From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linda Knippers Subject: Re: [PATCH] acpi, nfit: fix acpi_check_dsm() vs zero functions implemented Date: Mon, 27 Jun 2016 15:03:14 -0400 Message-ID: <9a95e65b-30ad-5175-db24-28d7603e513d@hpe.com> References: <146679026571.24395.11569929364936343871.stgit@dwillia2-desk3.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linux-nvdimm-bounces-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org Sender: "Linux-nvdimm" To: Dan Williams Cc: "Rafael J. Wysocki" , Linux ACPI , Len Brown , Xiao Guangrong , "linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org" List-Id: linux-acpi@vger.kernel.org On 6/27/2016 2:58 PM, Dan Williams wrote: > On Mon, Jun 27, 2016 at 11:47 AM, Linda Knippers wrote: >> >> >> On 6/27/2016 2:06 PM, Dan Williams wrote: >>> On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers 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