subject:"\[PATCH v8 4\/5\] nvdimm acpi\: build ACPI nvdimm devices"

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

2015-11-30 Thread Michael S. Tsirkin

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 
> ---
>  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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list:

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

2015-11-30 Thread Michael S. Tsirkin

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 
> ---
>  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 majord...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-30 Thread Xiao Guangrong




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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-30 Thread 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 
---
  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 4/5] nvdimm acpi: build ACPI nvdimm devices

2015-11-16 Thread Xiao Guangrong

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 
---
 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;
+
+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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

[PATCH v8 4/5] nvdimm acpi: build ACPI nvdimm devices

5 matches

Site Navigation

Mail list logo

Footer information