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.

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.

Cc: "Rafael J. Wysocki" <rjw(a)rjwysocki.net&gt; Cc: Len Brown <lenb(a)kernel.org&gt; Cc: Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism") Reported-by: Xiao Guangrong <guangrong.xiao(a)linux.intel.com&gt; Tested-by: Xiao Guangrong <guangrong.xiao(a)linux.intel.com&gt; Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; --- drivers/acpi/nfit.c | 6 +++--- drivers/acpi/utils.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 2215fc847fa9..32579a7b71d5 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -1131,11 +1131,11 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, /* * Until standardization materializes we need to consider up to 3 - * different command sets. Note, that checking for function0 (bit0) - * tells us if any commands are reachable through this uuid. + * different command sets. Note, that checking for zero functions + * tells us if any commands might be reachable through this uuid. */ for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_HPE2; i++) - if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) + if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 0)) break; /* limit the supported commands to those that are publicly documented */ diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 22c09952e177..b4de130f2d57 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -680,9 +680,6 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs) u64 mask = 0; union acpi_object *obj; - if (funcs == 0) - return false; - obj = acpi_evaluate_dsm(handle, uuid, rev, 0, NULL); if (!obj) return false; @@ -695,6 +692,9 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs) mask |= (((u64)obj->buffer.pointer[i]) << (i * 8)); ACPI_FREE(obj); + if (funcs == 0) + return true; + /* * Bit 0 indicates whether there's support for any functions other than * function 0 for the specified UUID and revision. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm(a)lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm

On 06/28/2016 02:58 AM, Dan Williams wrote: > 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. > Sorry. I do not know why you guys think QEMU does not support function 0. It already returns 0 to indicates "no functions are supported (other than function zero)".

-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Fri, Jun 24, 2016 at 10:44:25AM -0700, 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. Dan, This breaks calling DSM on HPE platforms and is a regression. E-mail context can be found at: https://lists.01.org/pipermail/linux-nvdimm/2016-June/006099.html The problem with this change is that it assumes that ACPI returning an object means that the UUID is supported on that platform. However, looking at ACPI v 6.1 section 9.1.1, the example for evaluating a _DSM shows that if the UUID is not supported at all, a zeroed out buffer of length 1 is returned: // // If not one of the UUIDs we recognize, then return a buffer // with bit 0 set to 0 indicating no functions supported. // return(Buffer(){0}) HPE firmware has been following this practice for a long long time. I suspect other manufacturer's firmware do so as well. The problem is that this creates an ambiguity and the linux code is no longer differentiating the case where the DSM/UUID is supported but only implements function 0 (the QEMU case you're trying to accommodate) and the case that the DSM/UUID is not supported at all. The result is that the code in acpi_nfit_add_dimm: for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_HPE2; i++) - if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) + if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 0)) break; always matches NVDIMM_FAMILY_INTEL. This is either because its is an Intel nvdimm, or because the unsupported UUID returns back a zeroed out buffer of length 1. As nfit_mem->family always equals NVDIMM_FAMILY_INTEL, no subsequent DSM call will succeed for NVDIMM_FAMILY_HPE1 or any other family. I don't have a fix as of yet, but wanted to make you aware of the problem.

> > 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. > > Cc: "Rafael J. Wysocki" <rjw(a)rjwysocki.net&gt; > Cc: Len Brown <lenb(a)kernel.org&gt; > Cc: Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; > Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism") > Reported-by: Xiao Guangrong <guangrong.xiao(a)linux.intel.com&gt; > Tested-by: Xiao Guangrong <guangrong.xiao(a)linux.intel.com&gt; > Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; > --- > drivers/acpi/nfit.c | 6 +++--- > drivers/acpi/utils.c | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 2215fc847fa9..32579a7b71d5 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -1131,11 +1131,11 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, > > /* > * Until standardization materializes we need to consider up to 3 > - * different command sets. Note, that checking for function0 (bit0) > - * tells us if any commands are reachable through this uuid. > + * different command sets. Note, that checking for zero functions > + * tells us if any commands might be reachable through this uuid. > */ > for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_HPE2; i++) > - if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) > + if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 0)) > break; At this point i will always == NVDIMM_FAMILY_INTEL. > > /* limit the supported commands to those that are publicly documented */ > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 22c09952e177..b4de130f2d57 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -680,9 +680,6 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs) > u64 mask = 0; > union acpi_object *obj; > > - if (funcs == 0) > - return false; > - > obj = acpi_evaluate_dsm(handle, uuid, rev, 0, NULL); > if (!obj) > return false; > @@ -695,6 +692,9 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs) > mask |= (((u64)obj->buffer.pointer[i]) << (i * 8)); > ACPI_FREE(obj); Unsupported UUID will get an object. A zeroed out buffer of length 1. So, acpi_check_dsm is saying supported when it is not. > > + if (funcs == 0) > + return true; > + > /* > * Bit 0 indicates whether there's support for any functions other than > * function 0 for the specified UUID and revision.

On Tue, Jul 19, 2016 at 11:52 AM, Dan Williams <dan.j.williams(a)intel.com&gt; wrote: > On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers(a)hpe.com&gt; wrote: >> On 7/19/2016 1:11 PM, Jerry Hoemann wrote: [..] >>> As nfit_mem->family always equals NVDIMM_FAMILY_INTEL, no subsequent >>> DSM call will succeed for NVDIMM_FAMILY_HPE1 or any other >>> family. >>> >>> I don't have a fix as of yet, but wanted to make you aware of >>> the problem. >> >> Could we try the all known UUIDs looking for one that returns a non-zero >> value? >> > > Yes, that seems like the way forward, and also make not finding a DSM > family non-fatal. Actually, all we need is that last bit... Jerry, Xiao, can you try the attached patch on top for v4.7-rc6 to see if it works in both HPe and QEMU 2.6 environments respectively?

From 367f4468b349a9ed22fc0e6382a52c18c6775472 Mon Sep 17 00:00:00 2001 From: Dan Williams <dan.j.williams(a)intel.com&gt; Date: Tue, 19 Jul 2016 12:32:39 -0700 Subject: [PATCH] nfit: make DIMM DSMs optional Commit 4995734e973a "acpi, nfit: fix acpi_check_dsm() vs zero functions implemented" attempted to fix a QEMU regression by supporting its usage of a zero-mask as a valid response to a DSM-family probe request. However, this behavior breaks HP platforms that return a zero-mask by default causing the probe to misidentify the DSM-family. Instead, the QEMU regression can be fixed by simply not requiring the DSM family to be identified. This effectively reverts commit 4995734e973a, and removes the DSM requirement from the init path. Cc: "Rafael J. Wysocki" <rafael(a)kernel.org&gt; Cc: Xiao Guangrong <guangrong.xiao(a)linux.intel.com&gt; Cc: Linda Knippers <linda.knippers(a)hpe.com&gt; Fixes: 4995734e973a ("acpi, nfit: fix acpi_check_dsm() vs zero functions implemented") Reported-by: Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; Signed-off-by: Dan Williams <dan.j.williams(a)intel.com&gt; --- drivers/acpi/nfit.c | 11 ++++++----- drivers/acpi/utils.c | 6 +++--- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index ac6ddcc080d4..1f0e06065ae6 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -1131,11 +1131,11 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, /* * Until standardization materializes we need to consider up to 3 - * different command sets. Note, that checking for zero functions - * tells us if any commands might be reachable through this uuid. + * different command sets. Note, that checking for function0 (bit0) + * tells us if any commands are reachable through this uuid. */ for (i = NVDIMM_FAMILY_INTEL; i <= NVDIMM_FAMILY_HPE2; i++) - if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 0)) + if (acpi_check_dsm(adev_dimm->handle, to_nfit_uuid(i), 1, 1)) break; /* limit the supported commands to those that are publicly documented */ @@ -1151,9 +1151,10 @@ static int acpi_nfit_add_dimm(struct acpi_nfit_desc *acpi_desc, if (disable_vendor_specific) dsm_mask &= ~(1 << 8); } else { - dev_err(dev, "unknown dimm command family\n"); + dev_dbg(dev, "unknown dimm command family\n"); nfit_mem->family = -1; - return force_enable_dimms ? 0 : -ENODEV; + /* DSMs are optional, continue loading the driver... */ + return 0; } uuid = to_nfit_uuid(nfit_mem->family); diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index b4de130f2d57..22c09952e177 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -680,6 +680,9 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs) u64 mask = 0; union acpi_object *obj; + if (funcs == 0) + return false; + obj = acpi_evaluate_dsm(handle, uuid, rev, 0, NULL); if (!obj) return false; @@ -692,9 +695,6 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs) mask |= (((u64)obj->buffer.pointer[i]) << (i * 8)); ACPI_FREE(obj); - if (funcs == 0) - return true; - /* * Bit 0 indicates whether there's support for any functions other than * function 0 for the specified UUID and revision. -- 2.5.5