Re: [PATCH] acpi, nfit: fix acpi_check_dsm() vs zero functions implemented

On Mon, Jun 27, 2016 at 11:47 AM, Linda Knippers <linda.knippers(a)hpe.com&gt; wrote: > > > On 6/27/2016 2:06 PM, Dan Williams wrote: >> On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers(a)hpe.com&gt; wrote: >>> >>> On 6/24/2016 1:44 PM, Dan Williams wrote: >>>> QEMU 2.6 implements nascent support for nvdimm DSMs. Depending on >>>> configuration it may only implement the function0 dsm to indicate that >>>> no other DSMs are available. Commit 31eca76ba2fc "nfit, libnvdimm: >>>> limited/whitelisted dimm command marshaling mechanism" breaks QEMU, but >>>> QEMU is spec compliant. Per the spec the way to indicate that no >>>> functions are supported is: >>>> >>>> If Function Index is zero, the return is a buffer containing one bit >>>> for each function index, starting with zero. Bit 0 indicates whether >>>> there is support for any functions other than function 0 for the >>>> specified UUID and Revision ID. If set to zero, no functions are >>>> supported (other than function zero) for the specified UUID and >>>> Revision ID. >>> >>> The rest of that paragraph is: >>> >>> If set to one, at least one additional function is supported. For all other bits >>> in the buffer, a bit is set to zero to indicate if that function index is not >>> supported for the specific UUID and Revision ID. (For example, bit 1 set to 0 >>> indicates that function index 1 is not supported for the specific UUID and >>> Revision ID.) >>> >>>> >>>> Update the nfit driver to determine the family (interface UUID) without >>>> requiring the implementation to define any other functions, i.e. >>>> short-circuit acpi_check_dsm() to succeed per the spec. The nfit driver >>>> appears to be the only user passing funcs==0 to acpi_check_dsm(), so >>>> this behavior change of the common routine should be limited to the >>>> probing done by the nfit driver. >>> >>> I don't understand why we're special casing this to support QEMU only when >>> there are no DSM functions supported. If we want to implement the >>> spec and support function zero, I think we should support it correctly. >>> That means returning the correct value for all spec compliant callers, >>> even when there are functions that are supported. >> >> QEMU 2.6 already shipped, so whatever we do we should not regress >> those binaries. The QEMU behavior could be argued as not spec >> compliant, but they've implemented enough of function0 to answer the >> "which family" probe. > > How would you argue that? acpi_evaluate_dsm() returns data instead of an error. >> Yes, if an implementation supports function0 it >> should say so in the returned bitmask, > > But in other places you explicitly prevent function 0 from > being executed. Right, for the whitelist-filtered result to userspace, but this patch is about the initial kernel probe. >> but by the time we've >> determined that function0 is "not supported" we've already >> successfully executed a function0 request. > > If they advertise a _DSM, I think they have to support function 0. They should, but didn't. Kernel v4.6 works with qemu 2.6, kernel v4.7 does not, so the kernel needs to be fixed.