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.knipp...@hpe.com>
> > wrote:
> > >
> > >
> > > On 6/27/2016 2:06 PM, Dan Williams wrote:
> > > > On Mon, Jun 27, 2016 at 10:40 AM, Linda Knippers
> > > > <linda.knipp...@hpe.com> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
