Re: [Qemu-devel] [PATCH v8 4/5] nvdimm acpi: build ACPI nvdimm devices

[Xiao Guangrong]

On 11/30/2015 06:30 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 16, 2015 at 06:51:02PM +0800, Xiao Guangrong wrote:
> > NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices
> >
> > There is a root device under \_SB and specified NVDIMM devices are under the
> > root device. Each NVDIMM device has _ADR which returns its handle used to
> > associate MEMDEV structure in NFIT
> >
> > Currently, we do not support any function on _DSM, that means, NVDIMM
> > label data has not been supported yet
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >   hw/acpi/nvdimm.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 85 insertions(+)
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 98c004d..abe0daa 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -367,6 +367,90 @@ static void nvdimm_build_nfit(GSList *device_list, GArray 
> > *table_offsets,
> >       g_array_free(structures, true);
> >   }
> >
> > +static void nvdimm_build_common_dsm(Aml *root_dev)
> > +{
> > +    Aml *method, *ifctx, *function;
> > +    uint8_t byte_list[1];
> > +
> > +    method = aml_method("NCAL", 4);
>
> This "NCAL" needs a define as it's used
> in multiple places. It's really just a DSM
> implementation, right? Reflect this in the macro
> name.

Yes, it is a common DSM method used by both root device and nvdimm devices.
I will do it like this:

#define NVDIMM_COMMON_DSM       "NCAL"

> > +    {
>
> What's this doing?

It just a reminder that the code containing in this braces is a DSM body like
a C function. However, i do not have strong opinion on it, will drop this style
if you dislike it.

> > +        function = aml_arg(2);
> > +
> > +        /*
> > +         * function 0 is called to inquire what functions are supported by
> > +         * OSPM
> > +         */
> > +        ifctx = aml_if(aml_equal(function, aml_int(0)));
> > +        byte_list[0] = 0 /* No function Supported */;
> > +        aml_append(ifctx, aml_return(aml_buffer(1, byte_list)));
> > +        aml_append(method, ifctx);
> > +
> > +        /* No function is supported yet. */
> > +        byte_list[0] = 1 /* Not Supported */;
> > +        aml_append(method, aml_return(aml_buffer(1, byte_list)));
> > +    }
> > +    aml_append(root_dev, method);
> > +}
> > +
> > +static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
> > +{
> > +    for (; device_list; device_list = device_list->next) {
> > +        DeviceState *dev = device_list->data;
> > +        int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> > +                                           NULL);
> > +        uint32_t handle = nvdimm_slot_to_handle(slot);
> > +        Aml *nvdimm_dev, *method;
> > +
> > +        nvdimm_dev = aml_device("NV%02X", slot);
> > +        aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle)));
> > +
> > +        method = aml_method("_DSM", 4);
> > +        {
> > +            aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0),
> > +                       aml_arg(1), aml_arg(2), aml_arg(3))));
> > +        }
> > +        aml_append(nvdimm_dev, method);
> > +
> > +        aml_append(root_dev, nvdimm_dev);
> > +    }
> > +}
> > +
> > +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
> > +                              GArray *table_data, GArray *linker)
> > +{
> > +    Aml *ssdt, *sb_scope, *dev, *method;
> > +
> > +    acpi_add_table(table_offsets, table_data);
> > +
> > +    ssdt = init_aml_allocator();
> > +    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> > +
> > +    sb_scope = aml_scope("\\_SB");
> > +
> > +    dev = aml_device("NVDR");
> > +    aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
>
> Pls add a comment explaining that ACPI0012 is NVDIMM root device.

Okay, will add these comment:

/*
 * NVDIMM is introduced in ACPI 6.0 9.20 NVDIMM Devices which defines an NVDIMM
 * root device under _SB scope with a _HID of “ACPI0012”. For each NVDIMM 
present
 * or intended to be supported by platform, platform firmware also exposes an 
ACPI
 * Namespace Device under the root device.
 */

> Also - this will now appear for all users, e.g.
> windows guests will prompt users for a driver.
> Not nice if user didn't actually ask for nvdimm.
>
> A simple solution is to default this functionality
> to off by default.

Okay, will disable nvdimm on default in the next version.

> > +
> > +    nvdimm_build_common_dsm(dev);
> > +    method = aml_method("_DSM", 4);
> > +    {
> > +        aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0),
> > +                   aml_arg(1), aml_arg(2), aml_arg(3))));
> > +    }
>
> Some duplication here, move above to a sub-function please.

Okay, will add a function named nvdimm_build_device_dsm() to do these
things.

Thanks!