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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <guangrong.xiao@linux.intel.com>
> ---
>  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.

> +    {

What's this doing?

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

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.

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

> +    aml_append(dev, method);
> +
> +    nvdimm_build_nvdimm_devices(device_list, dev);
> +
> +    aml_append(sb_scope, dev);
> +
> +    aml_append(ssdt, sb_scope);
> +    /* copy AML table into ACPI tables blob and patch header there */
> +    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> +    build_header(linker, table_data,
> +        (void *)(table_data->data + table_data->len - ssdt->buf->len),
> +        "SSDT", ssdt->buf->len, 1, "NVDIMM");
> +    free_aml_allocator();
> +}
> +
>  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>                         GArray *linker)
>  {
> @@ -378,5 +462,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
>          return;
>      }
>      nvdimm_build_nfit(device_list, table_offsets, table_data, linker);
> +    nvdimm_build_ssdt(device_list, table_offsets, table_data, linker);
>      g_slist_free(device_list);
>  }
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html