Re: [PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Igor Mammedov]

On Wed, 21 Oct 2015 21:32:38 +0800
Xiao Guangrong  wrote:

> 
> 
> On 10/21/2015 06:49 PM, Stefan Hajnoczi wrote:
> > On Wed, Oct 21, 2015 at 12:26:35AM +0800, Xiao Guangrong wrote:
> >>
> >>
> >> On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote:
> >>> On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
>  +exit:
>  +/* Write our output result to dsm memory. */
>  +((dsm_out *)dsm_ram_addr)->len = out->len;
> >>>
> >>> Missing byteswap?
> >>>
> >>> I thought you were going to remove this field because it wasn't needed
> >>> by the guest.
> >>>
> >>
> >> The @len is the size of _DSM result buffer, for example, for the function 
> >> of
> >> DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for
> >> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL 
> >> code
> >> how much size of memory we need to return to the _DSM caller.
> >>
> >> In _DSM code, it's handled like this:
> >>
> >> "RLEN" is @len, “OBUF” is the left memory in DSM page.
> >>
> >>  /* get @len*/
> >>  aml_append(method, aml_store(aml_name("RLEN"), aml_local(6)));
> >>  /* @len << 3 to get bits. */
> >>  aml_append(method, aml_store(aml_shiftleft(aml_local(6),
> >> aml_int(3)), aml_local(6)));
> >>
> >>  /* get @len << 3 bits from OBUF, and return it to the caller. */
> >>  aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
> >>  aml_local(6) , "OBUF"));
> >>
> >> Since @len is our internally used, it's not return to guest, so i did not 
> >> do
> >> byteswap here.
> >
> > I am not familiar with the ACPI details, but I think this emits bytecode
> > that will be run by the guest's ACPI interpreter?
> >
> > You still need to define the endianness of fields since QEMU and the
> > guest could have different endianness.
> >
> > In other words, will the following work if a big-endian ppc host is
> > running a little-endian x86 guest?
> >
> >((dsm_out *)dsm_ram_addr)->len = out->len;
> >
> 
> Er... If we do byteswap in QEMU then it is also needed in ASL code, however,
> ASL lacks this kind of instruction.  I guess ACPI interpreter is smart enough
> to change value to Littel-Endian for all 2 bytes / 4 bytes / 8 bytes accesses
> 
> I will do the change in next version, thanks for you pointing it out, Stefan!
According to ACPI spec integers encoded as little endian,
so QEMU needs to convert fields accessible by OSPM to it
(i.e. do cpu_to_le())

> 
> --
> 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 v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Xiao Guangrong]

On 10/21/2015 06:49 PM, Stefan Hajnoczi wrote:

On Wed, Oct 21, 2015 at 12:26:35AM +0800, Xiao Guangrong wrote:



On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote:

On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:

+exit:
+/* Write our output result to dsm memory. */
+((dsm_out *)dsm_ram_addr)->len = out->len;


Missing byteswap?

I thought you were going to remove this field because it wasn't needed
by the guest.



The @len is the size of _DSM result buffer, for example, for the function of
DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for
DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code
how much size of memory we need to return to the _DSM caller.

In _DSM code, it's handled like this:

"RLEN" is @len, “OBUF” is the left memory in DSM page.

 /* get @len*/
 aml_append(method, aml_store(aml_name("RLEN"), aml_local(6)));
 /* @len << 3 to get bits. */
 aml_append(method, aml_store(aml_shiftleft(aml_local(6),
aml_int(3)), aml_local(6)));

 /* get @len << 3 bits from OBUF, and return it to the caller. */
 aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
 aml_local(6) , "OBUF"));

Since @len is our internally used, it's not return to guest, so i did not do
byteswap here.


I am not familiar with the ACPI details, but I think this emits bytecode
that will be run by the guest's ACPI interpreter?

You still need to define the endianness of fields since QEMU and the
guest could have different endianness.

In other words, will the following work if a big-endian ppc host is
running a little-endian x86 guest?

   ((dsm_out *)dsm_ram_addr)->len = out->len;



Er... If we do byteswap in QEMU then it is also needed in ASL code, however,
ASL lacks this kind of instruction.  I guess ACPI interpreter is smart enough
to change value to Littel-Endian for all 2 bytes / 4 bytes / 8 bytes accesses

I will do the change in next version, thanks for you pointing it out, Stefan!

--
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 v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Stefan Hajnoczi]

On Wed, Oct 21, 2015 at 12:26:35AM +0800, Xiao Guangrong wrote:
> 
> 
> On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote:
> >On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
> >>+exit:
> >>+/* Write our output result to dsm memory. */
> >>+((dsm_out *)dsm_ram_addr)->len = out->len;
> >
> >Missing byteswap?
> >
> >I thought you were going to remove this field because it wasn't needed
> >by the guest.
> >
> 
> The @len is the size of _DSM result buffer, for example, for the function of
> DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for
> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code
> how much size of memory we need to return to the _DSM caller.
> 
> In _DSM code, it's handled like this:
> 
> "RLEN" is @len, “OBUF” is the left memory in DSM page.
> 
> /* get @len*/
> aml_append(method, aml_store(aml_name("RLEN"), aml_local(6)));
> /* @len << 3 to get bits. */
> aml_append(method, aml_store(aml_shiftleft(aml_local(6),
>aml_int(3)), aml_local(6)));
> 
> /* get @len << 3 bits from OBUF, and return it to the caller. */
> aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
> aml_local(6) , "OBUF"));
> 
> Since @len is our internally used, it's not return to guest, so i did not do
> byteswap here.

I am not familiar with the ACPI details, but I think this emits bytecode
that will be run by the guest's ACPI interpreter?

You still need to define the endianness of fields since QEMU and the
guest could have different endianness.

In other words, will the following work if a big-endian ppc host is
running a little-endian x86 guest?

  ((dsm_out *)dsm_ram_addr)->len = out->len;

Stefan
--
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 v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Xiao Guangrong]

On 10/21/2015 12:26 AM, Xiao Guangrong wrote:



On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote:

On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:

+exit:
+/* Write our output result to dsm memory. */
+((dsm_out *)dsm_ram_addr)->len = out->len;


Missing byteswap?

I thought you were going to remove this field because it wasn't needed
by the guest.



The @len is the size of _DSM result buffer, for example, for the function of
DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for
DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code


Sorry, s/DSM_DEV_FUN_NAMESPACE_LABEL_SIZE/DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA


how much size of memory we need to return to the _DSM caller.

In _DSM code, it's handled like this:

"RLEN" is @len, “OBUF” is the left memory in DSM page.

 /* get @len*/
 aml_append(method, aml_store(aml_name("RLEN"), aml_local(6)));
 /* @len << 3 to get bits. */
 aml_append(method, aml_store(aml_shiftleft(aml_local(6),
aml_int(3)), aml_local(6)));

 /* get @len << 3 bits from OBUF, and return it to the caller. */
 aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
 aml_local(6) , "OBUF"));

Since @len is our internally used, it's not return to guest, so i did not do
byteswap here.

--
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 v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Michael S. Tsirkin]

On Tue, Oct 20, 2015 at 04:51:49PM +0100, Stefan Hajnoczi wrote:
> On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
> > +exit:
> > +/* Write our output result to dsm memory. */
> > +((dsm_out *)dsm_ram_addr)->len = out->len;
> 
> Missing byteswap?


That's why I'd prefer no structures, using build_append_int_noprefix,
unless the structure is used outside ACPI as well.

> I thought you were going to remove this field because it wasn't needed
> by the guest.
--
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 v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Stefan Hajnoczi]

On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
> +exit:
> +/* Write our output result to dsm memory. */
> +((dsm_out *)dsm_ram_addr)->len = out->len;

Missing byteswap?

I thought you were going to remove this field because it wasn't needed
by the guest.
--
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 v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Xiao Guangrong]

On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote:

On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:

+exit:
+/* Write our output result to dsm memory. */
+((dsm_out *)dsm_ram_addr)->len = out->len;


Missing byteswap?

I thought you were going to remove this field because it wasn't needed
by the guest.



The @len is the size of _DSM result buffer, for example, for the function of
DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for
DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code
how much size of memory we need to return to the _DSM caller.

In _DSM code, it's handled like this:

"RLEN" is @len, “OBUF” is the left memory in DSM page.

/* get @len*/
aml_append(method, aml_store(aml_name("RLEN"), aml_local(6)));
/* @len << 3 to get bits. */
aml_append(method, aml_store(aml_shiftleft(aml_local(6),
   aml_int(3)), aml_local(6)));

/* get @len << 3 bits from OBUF, and return it to the caller. */
aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
aml_local(6) , "OBUF"));

Since @len is our internally used, it's not return to guest, so i did not do
byteswap here.
--
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 v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Michael S. Tsirkin]

On Mon, Oct 19, 2015 at 12:39:24PM +0800, Xiao Guangrong wrote:
> 
> 
> On 10/19/2015 02:05 AM, Michael S. Tsirkin wrote:
> >On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
> >>__DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method)
> >>
> >>Function 0 is a query function. We do not support any function on root
> >>device and only 3 functions are support for NVDIMM device,
> >>DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and
> >>DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow to
> >>access device's Label Namespace
> >>
> >>Signed-off-by: Xiao Guangrong 
> >>---
> >>  hw/acpi/nvdimm.c | 184 
> >> ++-
> >>  1 file changed, 182 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> >>index b211b8b..37fea1c 100644
> >>--- a/hw/acpi/nvdimm.c
> >>+++ b/hw/acpi/nvdimm.c
> >>@@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
> >>  return nvdimm_slot_to_spa_index(slot) + 1;
> >>  }
> >>
> >>+static NVDIMMDevice
> >>+*nvdimm_get_device_by_handle(GSList *list, uint32_t handle)
> >>+{
> >>+for (; list; list = list->next) {
> >>+NVDIMMDevice *nvdimm = list->data;
> >>+int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
> >>+   NULL);
> >>+
> >>+if (nvdimm_slot_to_handle(slot) == handle) {
> >>+return nvdimm;
> >>+}
> >>+}
> >>+
> >>+return NULL;
> >>+}
> >>+
> >>  /*
> >>   * Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range
> >>   * Structure
> >>@@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures, 
> >>GArray *table_offsets,
> >>  /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt */
> >>  #define NOTIFY_VALUE  0x99
> >
> >Again, please prefix everything consistently.
> 
> Okay, will do. Sorry for i missed it.
> 
> >
> >>
> >>+enum {
> >>+DSM_FUN_IMPLEMENTED = 0,
> >>+
> >>+/* NVDIMM Root Device Functions */
> >>+DSM_ROOT_DEV_FUN_ARS_CAP = 1,
> >>+DSM_ROOT_DEV_FUN_ARS_START = 2,
> >>+DSM_ROOT_DEV_FUN_ARS_QUERY = 3,
> >>+
> >>+/* NVDIMM Device (non-root) Functions */
> >>+DSM_DEV_FUN_SMART = 1,
> >>+DSM_DEV_FUN_SMART_THRESHOLD = 2,
> >>+DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3,
> >>+DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4,
> >>+DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5,
> >>+DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6,
> >>+DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7,
> >>+DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8,
> >>+DSM_DEV_FUN_VENDOR_SPECIFIC = 9,
> >>+};
> >
> >Does FUN stand for "function"? FUNC or FN is probably better.
> >
> 
> Yes.
> 
> >Please list exact names as they appear in spec so
> >they can be searched for.
> 
> The spec reference was at where this _FUN_ is used, eg:
> 
> /*
>  * Please refer to DSM specification 4.4.1 Get Namespace Label Size
>  * (Function Index 4).
>  *
>  * It gets the size of Namespace Label data area and the max data size
>  * that Get/Set Namespace Label Data functions can transfer.
>  */
> static void nvdimm_dsm_func_label_size(NVDIMMDevice *nvdimm, GArray *out)
> 
> I will follow your ‘single use’ comments below, these definitions will
> be dropped, the code will be like this:
> 
> switch (function) {
> case 4 /* DSM Spec 4.4.1 Get Namespace Label Size Get Namespace Label Size. 
> */:

If it's the same spec, you don't have to repeat it:

/* Encode function according to DSM Spec rev 1.0 */
> switch (function) {
> case 4 /* 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:

same for chapter etc.

> nvdimm_dsm_func_label_size();
> case ...
> ...
> };
> 
> >
> >
> >
> >>+
> >>+enum {
> >>+/* Common return status codes. */
> >>+DSM_STATUS_SUCCESS = 0,   /* Success */
> >>+DSM_STATUS_NOT_SUPPORTED = 1, /* Not Supported */
> >>+
> >>+/* NVDIMM Root Device _DSM function return status codes*/
> >>+DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2,/* Invalid Input Parameters 
> >>*/
> >>+DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific
> >>+Error */
> >>+
> >>+/* NVDIMM Device (non-root) _DSM function return status codes*/
> >>+DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2,  /* Non-Existing Memory 
> >>Device */
> >>+DSM_DEV_STATUS_INVALID_PARAS = 3, /* Invalid Input Parameters 
> >>*/
> >>+DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 4, /* Vendor Specific Error */
> >>+};
> >>+
> >>+/* Current revision supported by DSM specification is 1. */
> >>+#define DSM_REVISION(1)
> >>+
> >>+/*
> >>+ * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): Return
> >>+ * Value Information:
> >
> >Drop "please refer to".
> 
> Okay.
> 
> >
> >>+ *   if set to zero, no functions are supported (other than function zero)

Re: [PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Xiao Guangrong]

On 10/19/2015 03:06 PM, Michael S. Tsirkin wrote:

On Mon, Oct 19, 2015 at 12:39:24PM +0800, Xiao Guangrong wrote:



On 10/19/2015 02:05 AM, Michael S. Tsirkin wrote:

On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:

__DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method)

Function 0 is a query function. We do not support any function on root
device and only 3 functions are support for NVDIMM device,
DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and
DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow to
access device's Label Namespace

Signed-off-by: Xiao Guangrong 
---
  hw/acpi/nvdimm.c | 184 ++-
  1 file changed, 182 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index b211b8b..37fea1c 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
  return nvdimm_slot_to_spa_index(slot) + 1;
  }

+static NVDIMMDevice
+*nvdimm_get_device_by_handle(GSList *list, uint32_t handle)
+{
+for (; list; list = list->next) {
+NVDIMMDevice *nvdimm = list->data;
+int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
+   NULL);
+
+if (nvdimm_slot_to_handle(slot) == handle) {
+return nvdimm;
+}
+}
+
+return NULL;
+}
+
  /*
   * Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range
   * Structure
@@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures, GArray 
*table_offsets,
  /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt */
  #define NOTIFY_VALUE  0x99


Again, please prefix everything consistently.


Okay, will do. Sorry for i missed it.





+enum {
+DSM_FUN_IMPLEMENTED = 0,
+
+/* NVDIMM Root Device Functions */
+DSM_ROOT_DEV_FUN_ARS_CAP = 1,
+DSM_ROOT_DEV_FUN_ARS_START = 2,
+DSM_ROOT_DEV_FUN_ARS_QUERY = 3,
+
+/* NVDIMM Device (non-root) Functions */
+DSM_DEV_FUN_SMART = 1,
+DSM_DEV_FUN_SMART_THRESHOLD = 2,
+DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3,
+DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4,
+DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5,
+DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6,
+DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7,
+DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8,
+DSM_DEV_FUN_VENDOR_SPECIFIC = 9,
+};


Does FUN stand for "function"? FUNC or FN is probably better.



Yes.


Please list exact names as they appear in spec so
they can be searched for.


The spec reference was at where this _FUN_ is used, eg:

/*
  * Please refer to DSM specification 4.4.1 Get Namespace Label Size
  * (Function Index 4).
  *
  * It gets the size of Namespace Label data area and the max data size
  * that Get/Set Namespace Label Data functions can transfer.
  */
static void nvdimm_dsm_func_label_size(NVDIMMDevice *nvdimm, GArray *out)

I will follow your ‘single use’ comments below, these definitions will
be dropped, the code will be like this:

switch (function) {
case 4 /* DSM Spec 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:


If it's the same spec, you don't have to repeat it:

/* Encode function according to DSM Spec rev 1.0 */

switch (function) {
 case 4 /* 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:


same for chapter etc.


Okay.




nvdimm_dsm_func_label_size();
case ...
...
};






+
+enum {
+/* Common return status codes. */
+DSM_STATUS_SUCCESS = 0,   /* Success */
+DSM_STATUS_NOT_SUPPORTED = 1, /* Not Supported */
+
+/* NVDIMM Root Device _DSM function return status codes*/
+DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2,/* Invalid Input Parameters */
+DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific
+Error */
+
+/* NVDIMM Device (non-root) _DSM function return status codes*/
+DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2,  /* Non-Existing Memory Device */
+DSM_DEV_STATUS_INVALID_PARAS = 3, /* Invalid Input Parameters */
+DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 4, /* Vendor Specific Error */
+};
+
+/* Current revision supported by DSM specification is 1. */
+#define DSM_REVISION(1)
+
+/*
+ * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): Return
+ * Value Information:


Drop "please refer to".


Okay.




+ *   if set to zero, no functions are supported (other than function zero)
+ *   for the specified UUID and Revision ID. If set to one, at least one
+ *   additional function is supported.
+ */
+
+/* do not support any function on root. */
+#define ROOT_SUPPORT_FUN (0ULL)


Needs a name that implies the comment somehow.


+#define DIMM_SUPPORT_FUN((1 << DSM_FUN_IMPLEMENTED)   \
+   | (1 << DSM_DEV_FUN_NAMESPACE_LABEL_SIZE)  

Re: [PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Xiao Guangrong]

On 10/19/2015 02:05 AM, Michael S. Tsirkin wrote:

On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:

__DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method)

Function 0 is a query function. We do not support any function on root
device and only 3 functions are support for NVDIMM device,
DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and
DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow to
access device's Label Namespace

Signed-off-by: Xiao Guangrong 
---
  hw/acpi/nvdimm.c | 184 ++-
  1 file changed, 182 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index b211b8b..37fea1c 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
  return nvdimm_slot_to_spa_index(slot) + 1;
  }

+static NVDIMMDevice
+*nvdimm_get_device_by_handle(GSList *list, uint32_t handle)
+{
+for (; list; list = list->next) {
+NVDIMMDevice *nvdimm = list->data;
+int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
+   NULL);
+
+if (nvdimm_slot_to_handle(slot) == handle) {
+return nvdimm;
+}
+}
+
+return NULL;
+}
+
  /*
   * Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range
   * Structure
@@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures, GArray 
*table_offsets,
  /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt */
  #define NOTIFY_VALUE  0x99


Again, please prefix everything consistently.


Okay, will do. Sorry for i missed it.





+enum {
+DSM_FUN_IMPLEMENTED = 0,
+
+/* NVDIMM Root Device Functions */
+DSM_ROOT_DEV_FUN_ARS_CAP = 1,
+DSM_ROOT_DEV_FUN_ARS_START = 2,
+DSM_ROOT_DEV_FUN_ARS_QUERY = 3,
+
+/* NVDIMM Device (non-root) Functions */
+DSM_DEV_FUN_SMART = 1,
+DSM_DEV_FUN_SMART_THRESHOLD = 2,
+DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3,
+DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4,
+DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5,
+DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6,
+DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7,
+DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8,
+DSM_DEV_FUN_VENDOR_SPECIFIC = 9,
+};


Does FUN stand for "function"? FUNC or FN is probably better.



Yes.


Please list exact names as they appear in spec so
they can be searched for.


The spec reference was at where this _FUN_ is used, eg:

/*
 * Please refer to DSM specification 4.4.1 Get Namespace Label Size
 * (Function Index 4).
 *
 * It gets the size of Namespace Label data area and the max data size
 * that Get/Set Namespace Label Data functions can transfer.
 */
static void nvdimm_dsm_func_label_size(NVDIMMDevice *nvdimm, GArray *out)

I will follow your ‘single use’ comments below, these definitions will
be dropped, the code will be like this:

switch (function) {
case 4 /* DSM Spec 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:
nvdimm_dsm_func_label_size();
case ...
...
};






+
+enum {
+/* Common return status codes. */
+DSM_STATUS_SUCCESS = 0,   /* Success */
+DSM_STATUS_NOT_SUPPORTED = 1, /* Not Supported */
+
+/* NVDIMM Root Device _DSM function return status codes*/
+DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2,/* Invalid Input Parameters */
+DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific
+Error */
+
+/* NVDIMM Device (non-root) _DSM function return status codes*/
+DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2,  /* Non-Existing Memory Device */
+DSM_DEV_STATUS_INVALID_PARAS = 3, /* Invalid Input Parameters */
+DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 4, /* Vendor Specific Error */
+};
+
+/* Current revision supported by DSM specification is 1. */
+#define DSM_REVISION(1)
+
+/*
+ * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): Return
+ * Value Information:


Drop "please refer to".


Okay.




+ *   if set to zero, no functions are supported (other than function zero)
+ *   for the specified UUID and Revision ID. If set to one, at least one
+ *   additional function is supported.
+ */
+
+/* do not support any function on root. */
+#define ROOT_SUPPORT_FUN (0ULL)


Needs a name that implies the comment somehow.


+#define DIMM_SUPPORT_FUN((1 << DSM_FUN_IMPLEMENTED)   \
+   | (1 << DSM_DEV_FUN_NAMESPACE_LABEL_SIZE)  \
+   | (1 << DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA)  \
+   | (1 << DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA))
+


I think it's best to just drop these macros.
There's a single point of use - just add a comment there
explaining what does it mean.


Okay. Good to me.


You will be able to drop all _FUN_ macros too.

Re: [PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Michael S. Tsirkin]

On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
> __DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method)
> 
> Function 0 is a query function. We do not support any function on root
> device and only 3 functions are support for NVDIMM device,
> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and
> DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow to
> access device's Label Namespace
> 
> Signed-off-by: Xiao Guangrong 
> ---
>  hw/acpi/nvdimm.c | 184 
> ++-
>  1 file changed, 182 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index b211b8b..37fea1c 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
>  return nvdimm_slot_to_spa_index(slot) + 1;
>  }
>  
> +static NVDIMMDevice
> +*nvdimm_get_device_by_handle(GSList *list, uint32_t handle)
> +{
> +for (; list; list = list->next) {
> +NVDIMMDevice *nvdimm = list->data;
> +int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
> +   NULL);
> +
> +if (nvdimm_slot_to_handle(slot) == handle) {
> +return nvdimm;
> +}
> +}
> +
> +return NULL;
> +}
> +
>  /*
>   * Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range
>   * Structure
> @@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures, GArray 
> *table_offsets,
>  /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt */
>  #define NOTIFY_VALUE  0x99

Again, please prefix everything consistently.

>  
> +enum {
> +DSM_FUN_IMPLEMENTED = 0,
> +
> +/* NVDIMM Root Device Functions */
> +DSM_ROOT_DEV_FUN_ARS_CAP = 1,
> +DSM_ROOT_DEV_FUN_ARS_START = 2,
> +DSM_ROOT_DEV_FUN_ARS_QUERY = 3,
> +
> +/* NVDIMM Device (non-root) Functions */
> +DSM_DEV_FUN_SMART = 1,
> +DSM_DEV_FUN_SMART_THRESHOLD = 2,
> +DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3,
> +DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4,
> +DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5,
> +DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6,
> +DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7,
> +DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8,
> +DSM_DEV_FUN_VENDOR_SPECIFIC = 9,
> +};

Does FUN stand for "function"? FUNC or FN is probably better.

Please list exact names as they appear in spec so
they can be searched for.



> +
> +enum {
> +/* Common return status codes. */
> +DSM_STATUS_SUCCESS = 0,   /* Success */
> +DSM_STATUS_NOT_SUPPORTED = 1, /* Not Supported */
> +
> +/* NVDIMM Root Device _DSM function return status codes*/
> +DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2,/* Invalid Input Parameters */
> +DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific
> +Error */
> +
> +/* NVDIMM Device (non-root) _DSM function return status codes*/
> +DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2,  /* Non-Existing Memory Device 
> */
> +DSM_DEV_STATUS_INVALID_PARAS = 3, /* Invalid Input Parameters */
> +DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 4, /* Vendor Specific Error */
> +};
> +
> +/* Current revision supported by DSM specification is 1. */
> +#define DSM_REVISION(1)
> +
> +/*
> + * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): Return
> + * Value Information:

Drop "please refer to".

> + *   if set to zero, no functions are supported (other than function zero)
> + *   for the specified UUID and Revision ID. If set to one, at least one
> + *   additional function is supported.
> + */
> +
> +/* do not support any function on root. */
> +#define ROOT_SUPPORT_FUN (0ULL)

Needs a name that implies the comment somehow.

> +#define DIMM_SUPPORT_FUN((1 << DSM_FUN_IMPLEMENTED)   \
> +   | (1 << DSM_DEV_FUN_NAMESPACE_LABEL_SIZE)  \
> +   | (1 << DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA)  \
> +   | (1 << DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA))
> +

I think it's best to just drop these macros.
There's a single point of use - just add a comment there
explaining what does it mean.
You will be able to drop all _FUN_ macros too.


>  struct dsm_in {
>  uint32_t handle;
>  uint32_t revision;
> @@ -420,6 +490,11 @@ struct dsm_in {
>  } QEMU_PACKED;
>  typedef struct dsm_in dsm_in;
>  
> +struct cmd_out_implemented {
> +uint64_t cmd_list;
> +};
> +typedef struct cmd_out_implemented cmd_out_implemented;
> +
>  struct dsm_out {
>  /* the size of buffer filled by QEMU. */
>  uint32_t len;
> @@ -434,12 +509,115 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned 
> size)
>  return 0;
>  }
>  
> +static void nvdimm_dsm_write_status(GArray *out, uint32_t status)
> +{
> +/* 

[PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function

[Xiao Guangrong]

__DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method)

Function 0 is a query function. We do not support any function on root
device and only 3 functions are support for NVDIMM device,
DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and
DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow to
access device's Label Namespace

Signed-off-by: Xiao Guangrong 
---
 hw/acpi/nvdimm.c | 184 ++-
 1 file changed, 182 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index b211b8b..37fea1c 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
 return nvdimm_slot_to_spa_index(slot) + 1;
 }
 
+static NVDIMMDevice
+*nvdimm_get_device_by_handle(GSList *list, uint32_t handle)
+{
+for (; list; list = list->next) {
+NVDIMMDevice *nvdimm = list->data;
+int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
+   NULL);
+
+if (nvdimm_slot_to_handle(slot) == handle) {
+return nvdimm;
+}
+}
+
+return NULL;
+}
+
 /*
  * Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range
  * Structure
@@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures, GArray 
*table_offsets,
 /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt */
 #define NOTIFY_VALUE  0x99
 
+enum {
+DSM_FUN_IMPLEMENTED = 0,
+
+/* NVDIMM Root Device Functions */
+DSM_ROOT_DEV_FUN_ARS_CAP = 1,
+DSM_ROOT_DEV_FUN_ARS_START = 2,
+DSM_ROOT_DEV_FUN_ARS_QUERY = 3,
+
+/* NVDIMM Device (non-root) Functions */
+DSM_DEV_FUN_SMART = 1,
+DSM_DEV_FUN_SMART_THRESHOLD = 2,
+DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3,
+DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4,
+DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5,
+DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6,
+DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7,
+DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8,
+DSM_DEV_FUN_VENDOR_SPECIFIC = 9,
+};
+
+enum {
+/* Common return status codes. */
+DSM_STATUS_SUCCESS = 0,   /* Success */
+DSM_STATUS_NOT_SUPPORTED = 1, /* Not Supported */
+
+/* NVDIMM Root Device _DSM function return status codes*/
+DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2,/* Invalid Input Parameters */
+DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific
+Error */
+
+/* NVDIMM Device (non-root) _DSM function return status codes*/
+DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2,  /* Non-Existing Memory Device */
+DSM_DEV_STATUS_INVALID_PARAS = 3, /* Invalid Input Parameters */
+DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 4, /* Vendor Specific Error */
+};
+
+/* Current revision supported by DSM specification is 1. */
+#define DSM_REVISION(1)
+
+/*
+ * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): Return
+ * Value Information:
+ *   if set to zero, no functions are supported (other than function zero)
+ *   for the specified UUID and Revision ID. If set to one, at least one
+ *   additional function is supported.
+ */
+
+/* do not support any function on root. */
+#define ROOT_SUPPORT_FUN (0ULL)
+#define DIMM_SUPPORT_FUN((1 << DSM_FUN_IMPLEMENTED)   \
+   | (1 << DSM_DEV_FUN_NAMESPACE_LABEL_SIZE)  \
+   | (1 << DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA)  \
+   | (1 << DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA))
+
 struct dsm_in {
 uint32_t handle;
 uint32_t revision;
@@ -420,6 +490,11 @@ struct dsm_in {
 } QEMU_PACKED;
 typedef struct dsm_in dsm_in;
 
+struct cmd_out_implemented {
+uint64_t cmd_list;
+};
+typedef struct cmd_out_implemented cmd_out_implemented;
+
 struct dsm_out {
 /* the size of buffer filled by QEMU. */
 uint32_t len;
@@ -434,12 +509,115 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
 return 0;
 }
 
+static void nvdimm_dsm_write_status(GArray *out, uint32_t status)
+{
+/* status locates in the first 4 bytes in the dsm memory. */
+assert(!out->len);
+
+status = cpu_to_le32(status);
+g_array_append_vals(out, , sizeof(status));
+}
+
+static void nvdimm_dsm_write_root(dsm_in *in, GArray *out)
+{
+uint32_t status = DSM_STATUS_NOT_SUPPORTED;
+
+/* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method) */
+if (in->function == DSM_FUN_IMPLEMENTED) {
+uint64_t cmd_list = cpu_to_le64(ROOT_SUPPORT_FUN);
+
+g_array_append_vals(out, _list, sizeof(cmd_list));
+return;
+}
+
+nvdimm_debug("Return status %#x.\n", status);
+nvdimm_dsm_write_status(out, status);
+}
+
+static void nvdimm_dsm_write_nvdimm(dsm_in *in, GArray *out)
+{
+GSList *list =