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

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

[Dan Williams]

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-LthD3rsA81gm4RdzfppkhA@public.gmane.org>
Cc: Len Brown <lenb-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Jerry Hoemann <jerry.hoemann-ZPxbGqLxI0U@public.gmane.org>
Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism")
Reported-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Tested-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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.

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

[Rafael J. Wysocki]

On Fri, Jun 24, 2016 at 7:44 PM, Dan Williams <dan.j.williams@intel.com> 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.
>
> 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@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Jerry Hoemann <jerry.hoemann@hpe.com>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism")
> Reported-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Tested-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

ACK

> ---
>  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.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linda Knippers]

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.

-- ljk
> 
> Cc: "Rafael J. Wysocki" <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org>
> Cc: Len Brown <lenb-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Jerry Hoemann <jerry.hoemann-ZPxbGqLxI0U@public.gmane.org>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism")
> Reported-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Tested-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
> 

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

[Dan Williams]

On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> 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. Yes, if an implementation supports function0 it
should say so in the returned bitmask, but by the time we've
determined that function0 is "not supported" we've already
successfully executed a function0 request.

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

[Linda Knippers]

On 6/27/2016 2:06 PM, Dan Williams wrote:
> On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> 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?

> 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.

> 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.

-- ljk

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

[Dan Williams]

On Mon, Jun 27, 2016 at 11:47 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> wrote:
>
>
> On 6/27/2016 2:06 PM, Dan Williams wrote:
>> On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> 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.

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

[Linda Knippers]

On 6/27/2016 2:58 PM, Dan Williams wrote:
> On Mon, Jun 27, 2016 at 11:47 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> wrote:
>>
>>
>> On 6/27/2016 2:06 PM, Dan Williams wrote:
>>> On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> 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.

I'm not actually arguing against this fix.  I'm arguing that we should
support function 0 more generally.

-- ljk

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

[Xiao Guangrong]

On 06/28/2016 02:58 AM, Dan Williams wrote:
> On Mon, Jun 27, 2016 at 11:47 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> wrote:
>>
>>
>> On 6/27/2016 2:06 PM, Dan Williams wrote:
>>> On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> 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)".

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

[Jerry Hoemann]

On Tue, Jun 28, 2016 at 05:37:45PM +0800, Xiao Guangrong wrote:
> 
> 
> On 06/28/2016 02:58 AM, Dan Williams wrote:
> > On Mon, Jun 27, 2016 at 11:47 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> wrote:
> > > 
> > > 
> > > On 6/27/2016 2:06 PM, Dan Williams wrote:
> > > > On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> 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)".
> 

  The currently upstream version of acpi_check_dsm doesn't handle this
  case,  it assumes at least one function other than zero is also being
  returned (as it assumes bit 0 is set.)

  Instead of the proposed change we should have something like:

index 22c0995..3ecf462 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -702,6 +702,9 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, u64 rev, u64 funcs)
        if ((mask & 0x1) && (mask & funcs) == funcs)
                return true;
 
+       if ((mask == 0) && (funcs == 1))
+               return true;
+
        return false;
 }
 EXPORT_SYMBOL(acpi_check_dsm);


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

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

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

[Jerry Hoemann]

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-LthD3rsA81gm4RdzfppkhA@public.gmane.org>
> Cc: Len Brown <lenb-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Jerry Hoemann <jerry.hoemann-ZPxbGqLxI0U@public.gmane.org>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism")
> Reported-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Tested-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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.



-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

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

[Linda Knippers]

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-LthD3rsA81gm4RdzfppkhA@public.gmane.org>
>> Cc: Len Brown <lenb-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Jerry Hoemann <jerry.hoemann-ZPxbGqLxI0U@public.gmane.org>
>> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command marshaling mechanism")
>> Reported-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> Tested-by: Xiao Guangrong <guangrong.xiao-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  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.
> 
> 
> 

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

[Dan Williams]

On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> wrote:
>
>
> 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?
>

Yes, that seems like the way forward, and also make not finding a DSM
family non-fatal.

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

[Jerry Hoemann]

On Tue, Jul 19, 2016 at 11:52:53AM -0700, Dan Williams wrote:
> On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> wrote:
> >
> >
> > 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?
> >
> 
> Yes, that seems like the way forward, and also make not finding a DSM
> family non-fatal.

Reverting this change would address for HPE, but I do not fully understand
the nature of the problem this change was attempting to address.

Can you expand a bit?

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

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

[Dan Williams]

On Tue, Jul 19, 2016 at 11:52 AM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers-ZPxbGqLxI0U@public.gmane.org> 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?

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

[Jerry Hoemann]

On Tue, Jul 19, 2016 at 01:01:16PM -0700, Dan Williams wrote:
> On Tue, Jul 19, 2016 at 11:52 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers@hpe.com> 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?

Dan,

I applied this patch on top of the SLES 12 sp2 kernel I was testing
with last night.

The proposed patch below works for HPE nvdimms.

Jerry


> From 367f4468b349a9ed22fc0e6382a52c18c6775472 Mon Sep 17 00:00:00 2001
> From: Dan Williams <dan.j.williams@intel.com>
> 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@kernel.org>
> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> Cc: Linda Knippers <linda.knippers@hpe.com>
> Fixes: 4995734e973a ("acpi, nfit: fix acpi_check_dsm() vs zero functions implemented")
> Reported-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  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
> 


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

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

[Dan Williams]

On Tue, Jul 19, 2016 at 3:46 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> On Tue, Jul 19, 2016 at 01:01:16PM -0700, Dan Williams wrote:
>> On Tue, Jul 19, 2016 at 11:52 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>> > On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers@hpe.com> 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?
>
> Dan,
>
> I applied this patch on top of the SLES 12 sp2 kernel I was testing
> with last night.
>
> The proposed patch below works for HPE nvdimms.

Thanks Jerry, I'll add your Tested-by and push it for v4.7-final after
Xiao has a chance to confirm.

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

[Dan Williams]

On Tue, Jul 19, 2016 at 3:53 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, Jul 19, 2016 at 3:46 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>> On Tue, Jul 19, 2016 at 01:01:16PM -0700, Dan Williams wrote:
>>> On Tue, Jul 19, 2016 at 11:52 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>>> > On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers@hpe.com> 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?
>>
>> Dan,
>>
>> I applied this patch on top of the SLES 12 sp2 kernel I was testing
>> with last night.
>>
>> The proposed patch below works for HPE nvdimms.
>
> Thanks Jerry, I'll add your Tested-by and push it for v4.7-final after
> Xiao has a chance to confirm.

I was able to verify this on QEMU 2.6.

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

[Xiao Guangrong]

On 07/21/2016 06:49 AM, Dan Williams wrote:
> On Tue, Jul 19, 2016 at 3:53 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Tue, Jul 19, 2016 at 3:46 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
>>> On Tue, Jul 19, 2016 at 01:01:16PM -0700, Dan Williams wrote:
>>>> On Tue, Jul 19, 2016 at 11:52 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>>>>> On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers@hpe.com> 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?
>>>
>>> Dan,
>>>
>>> I applied this patch on top of the SLES 12 sp2 kernel I was testing
>>> with last night.
>>>
>>> The proposed patch below works for HPE nvdimms.
>>
>> Thanks Jerry, I'll add your Tested-by and push it for v4.7-final after
>> Xiao has a chance to confirm.
>
> I was able to verify this on QEMU 2.6.
>

Thank you, Dan. :)

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

[Johannes Thumshirn]

On Tue, Jul 19, 2016 at 01:01:16PM -0700, Dan Williams wrote:
> On Tue, Jul 19, 2016 at 11:52 AM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> > On Tue, Jul 19, 2016 at 11:50 AM, Linda Knippers <linda.knippers-vdxHd4ltRiQ@public.gmane.orgm> 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?
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

Hi Dan,

Somehow the mailinglist dropped the patch attachment and patchwork didn't
pick it up either.

As you have a Tested-by by Jerry and Xiao, can you appliy it to your git so
downstream distros can pick it up?

Thanks a lot,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn-l3A5Bk7waGM@public.gmane.org                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850