On 7/19/2016 1:11 PM, Jerry Hoemann wrote: > 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. Could we try the all known UUIDs looking for one that returns a non-zero value? -- ljk > > > > >> >> 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@xxxxxxxxxxxxx> >> Cc: Len Brown <lenb@xxxxxxxxxx> >> Cc: Jerry Hoemann <jerry.hoemann@xxxxxxx> >> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism") >> Reported-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> >> Tested-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> >> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> >> --- >> 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. > > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
