[v8,4/5] nvdimm acpi: build ACPI nvdimm devices
diff mbox

Message ID 1447671063-92665-5-git-send-email-guangrong.xiao@linux.intel.com
State New
Headers show

Commit Message

Xiao Guangrong Nov. 16, 2015, 10:51 a.m. UTC
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(+)

Comments

Michael S. Tsirkin Nov. 30, 2015, 10:30 a.m. UTC | #1
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
--
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
Michael S. Tsirkin Nov. 30, 2015, 10:32 a.m. UTC | #2
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

Forgot to mention:

Pls put spec info in code comments near
relevant functions, not just the log.

> 
> 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);
> +    {
> +        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;

So why don't we skip this completely if device list is empty?

> +
> +    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")));
> +
> +    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))));
> +    }
> +    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
--
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
Xiao Guangrong Nov. 30, 2015, 12:21 p.m. UTC | #3
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 <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.
>

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!
--
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
Xiao Guangrong Nov. 30, 2015, 12:31 p.m. UTC | #4
On 11/30/2015 06:32 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
>
> Forgot to mention:
>
> Pls put spec info in code comments near
> relevant functions, not just the log.
>

Sure, good to me.

>> +
>> +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
>> +                              GArray *table_data, GArray *linker)
>> +{
>> +    Aml *ssdt, *sb_scope, *dev, *method;
>
> So why don't we skip this completely if device list is empty?

Yes, it is exactly what we did:

  void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
                         GArray *linker)
  {
      GSList *device_list;

      /* no NVDIMM device is plugged. */
      device_list = nvdimm_get_plugged_device_list();
      if (!device_list) {
          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);
  }
--
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

Patch
diff mbox

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);
+    {
+        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")));
+
+    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))));
+    }
+    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);
 }