[qemu PATCH v4 0/4] support NFIT platform capabilities

[qemu PATCH v4 0/4] support NFIT platform capabilities

[Ross Zwisler]

Changes since v3:
 * Updated the text in docs/nvdimm.txt to make it clear that the value
   being passed in on the command line in an integer made up of various
   bit fields. (Rob Elliott)
 
 * Updated the "Highest Valid Capability" byte to be dynamic based on
   the highest valid bit in the user's input. (Rob Elliott)

---

The first 2 patches in this series clean up some things I noticed while
coding.

Patch 3 adds support for the new Platform Capabilities Structure, which
was added to the NFIT in ACPI 6.2 Errata A.  We add a machine command
line option "nvdimm-cap":

    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2

which allows the user to pass in a value for this structure.  When such
a value is passed in we will generate the new NFIT subtable.

Patch 4 adds code to the "make check" self test infrastructure so that
we generate the new Platform Capabilities Structure, and adds it to the
expected NFIT output so that we test for it.

Ross Zwisler (4):
  nvdimm: fix typo in label-size definition
  tests/.gitignore: add entry for generated file
  nvdimm, acpi: support NFIT platform capabilities
  ACPI testing: test NFIT platform capabilities

 docs/nvdimm.txt                       |  27 ++++++++++++++++++++
 hw/acpi/nvdimm.c                      |  45 +++++++++++++++++++++++++++++++---
 hw/i386/pc.c                          |  31 +++++++++++++++++++++++
 hw/mem/nvdimm.c                       |   2 +-
 include/hw/i386/pc.h                  |   1 +
 include/hw/mem/nvdimm.h               |   7 +++++-
 tests/.gitignore                      |   1 +
 tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
 tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
 tests/bios-tables-test.c              |   2 +-
 10 files changed, 109 insertions(+), 7 deletions(-)

-- 
2.14.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[Qemu-devel] [qemu PATCH v4 0/4] support NFIT platform capabilities

[Ross Zwisler]

Changes since v3:
 * Updated the text in docs/nvdimm.txt to make it clear that the value
   being passed in on the command line in an integer made up of various
   bit fields. (Rob Elliott)
 
 * Updated the "Highest Valid Capability" byte to be dynamic based on
   the highest valid bit in the user's input. (Rob Elliott)

---

The first 2 patches in this series clean up some things I noticed while
coding.

Patch 3 adds support for the new Platform Capabilities Structure, which
was added to the NFIT in ACPI 6.2 Errata A.  We add a machine command
line option "nvdimm-cap":

    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2

which allows the user to pass in a value for this structure.  When such
a value is passed in we will generate the new NFIT subtable.

Patch 4 adds code to the "make check" self test infrastructure so that
we generate the new Platform Capabilities Structure, and adds it to the
expected NFIT output so that we test for it.

Ross Zwisler (4):
  nvdimm: fix typo in label-size definition
  tests/.gitignore: add entry for generated file
  nvdimm, acpi: support NFIT platform capabilities
  ACPI testing: test NFIT platform capabilities

 docs/nvdimm.txt                       |  27 ++++++++++++++++++++
 hw/acpi/nvdimm.c                      |  45 +++++++++++++++++++++++++++++++---
 hw/i386/pc.c                          |  31 +++++++++++++++++++++++
 hw/mem/nvdimm.c                       |   2 +-
 include/hw/i386/pc.h                  |   1 +
 include/hw/mem/nvdimm.h               |   7 +++++-
 tests/.gitignore                      |   1 +
 tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
 tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
 tests/bios-tables-test.c              |   2 +-
 10 files changed, 109 insertions(+), 7 deletions(-)

-- 
2.14.3

[qemu PATCH v4 1/4] nvdimm: fix typo in label-size definition

[Ross Zwisler]

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Fixes: commit da6789c27c2e ("nvdimm: add a macro for property "label-size"")
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/mem/nvdimm.c         | 2 +-
 include/hw/mem/nvdimm.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index acb656b672..4087aca25e 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -89,7 +89,7 @@ static void nvdimm_set_unarmed(Object *obj, bool value, Error **errp)
 
 static void nvdimm_init(Object *obj)
 {
-    object_property_add(obj, NVDIMM_LABLE_SIZE_PROP, "int",
+    object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
                         nvdimm_get_label_size, nvdimm_set_label_size, NULL,
                         NULL, NULL);
     object_property_add_bool(obj, NVDIMM_UNARMED_PROP,
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 7fd87c4e1c..74c60332e1 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -48,7 +48,7 @@
 #define NVDIMM_GET_CLASS(obj) OBJECT_GET_CLASS(NVDIMMClass, (obj), \
                                                TYPE_NVDIMM)
 
-#define NVDIMM_LABLE_SIZE_PROP "label-size"
+#define NVDIMM_LABEL_SIZE_PROP "label-size"
 #define NVDIMM_UNARMED_PROP    "unarmed"
 
 struct NVDIMMDevice {
-- 
2.14.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[Qemu-devel] [qemu PATCH v4 1/4] nvdimm: fix typo in label-size definition

[Ross Zwisler]

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Fixes: commit da6789c27c2e ("nvdimm: add a macro for property "label-size"")
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/mem/nvdimm.c         | 2 +-
 include/hw/mem/nvdimm.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index acb656b672..4087aca25e 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -89,7 +89,7 @@ static void nvdimm_set_unarmed(Object *obj, bool value, Error **errp)
 
 static void nvdimm_init(Object *obj)
 {
-    object_property_add(obj, NVDIMM_LABLE_SIZE_PROP, "int",
+    object_property_add(obj, NVDIMM_LABEL_SIZE_PROP, "int",
                         nvdimm_get_label_size, nvdimm_set_label_size, NULL,
                         NULL, NULL);
     object_property_add_bool(obj, NVDIMM_UNARMED_PROP,
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 7fd87c4e1c..74c60332e1 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -48,7 +48,7 @@
 #define NVDIMM_GET_CLASS(obj) OBJECT_GET_CLASS(NVDIMMClass, (obj), \
                                                TYPE_NVDIMM)
 
-#define NVDIMM_LABLE_SIZE_PROP "label-size"
+#define NVDIMM_LABEL_SIZE_PROP "label-size"
 #define NVDIMM_UNARMED_PROP    "unarmed"
 
 struct NVDIMMDevice {
-- 
2.14.3

[qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Ross Zwisler]

After a "make check" we end up with the following:

$ git status
On branch master
Your branch is up-to-date with 'origin/master'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	tests/test-block-backend

nothing added to commit but untracked files present (use "git add" to track)

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Fixes: commit ad0df3e0fdac ("block: test blk_aio_flush() with blk->root == NULL")
Cc: Kevin Wolf <kwolf@redhat.com>
---
 tests/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/.gitignore b/tests/.gitignore
index fb62d2299b..2bc61a9a58 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -21,6 +21,7 @@ test-base64
 test-bdrv-drain
 test-bitops
 test-bitcnt
+test-block-backend
 test-blockjob
 test-blockjob-txn
 test-bufferiszero
-- 
2.14.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[Qemu-devel] [qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Ross Zwisler]

After a "make check" we end up with the following:

$ git status
On branch master
Your branch is up-to-date with 'origin/master'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	tests/test-block-backend

nothing added to commit but untracked files present (use "git add" to track)

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Fixes: commit ad0df3e0fdac ("block: test blk_aio_flush() with blk->root == NULL")
Cc: Kevin Wolf <kwolf@redhat.com>
---
 tests/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/.gitignore b/tests/.gitignore
index fb62d2299b..2bc61a9a58 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -21,6 +21,7 @@ test-base64
 test-bdrv-drain
 test-bitops
 test-bitcnt
+test-block-backend
 test-blockjob
 test-blockjob-txn
 test-bufferiszero
-- 
2.14.3

Re: [Qemu-devel] [qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Eric Blake]

On 05/21/2018 11:32 AM, Ross Zwisler wrote:
> After a "make check" we end up with the following:
> 
> $ git status
> On branch master
> Your branch is up-to-date with 'origin/master'.
> 
> Untracked files:
>    (use "git add <file>..." to include in what will be committed)
> 
> 	tests/test-block-backend
> 
> nothing added to commit but untracked files present (use "git add" to track)
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Fixes: commit ad0df3e0fdac ("block: test blk_aio_flush() with blk->root == NULL")
> Cc: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/.gitignore | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index fb62d2299b..2bc61a9a58 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -21,6 +21,7 @@ test-base64
>   test-bdrv-drain
>   test-bitops
>   test-bitcnt
> +test-block-backend
>   test-blockjob
>   test-blockjob-txn
>   test-bufferiszero
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Eric Blake]

On 05/21/2018 11:32 AM, Ross Zwisler wrote:
> After a "make check" we end up with the following:
> 
> $ git status
> On branch master
> Your branch is up-to-date with 'origin/master'.
> 
> Untracked files:
>    (use "git add <file>..." to include in what will be committed)
> 
> 	tests/test-block-backend
> 
> nothing added to commit but untracked files present (use "git add" to track)
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Fixes: commit ad0df3e0fdac ("block: test blk_aio_flush() with blk->root == NULL")
> Cc: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/.gitignore | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index fb62d2299b..2bc61a9a58 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -21,6 +21,7 @@ test-base64
>   test-bdrv-drain
>   test-bitops
>   test-bitcnt
> +test-block-backend
>   test-blockjob
>   test-blockjob-txn
>   test-bufferiszero
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Philippe Mathieu-Daudé]

On 05/21/2018 01:41 PM, Eric Blake wrote:
> On 05/21/2018 11:32 AM, Ross Zwisler wrote:
>> After a "make check" we end up with the following:
>>
>> $ git status
>> On branch master
>> Your branch is up-to-date with 'origin/master'.
>>
>> Untracked files:
>>    (use "git add <file>..." to include in what will be committed)
>>
>>     tests/test-block-backend
>>
>> nothing added to commit but untracked files present (use "git add" to
>> track)
>>
>> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Fixes: commit ad0df3e0fdac ("block: test blk_aio_flush() with
>> blk->root == NULL")
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   tests/.gitignore | 1 +
>>   1 file changed, 1 insertion(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index fb62d2299b..2bc61a9a58 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -21,6 +21,7 @@ test-base64
>>   test-bdrv-drain
>>   test-bitops
>>   test-bitcnt
>> +test-block-backend
>>   test-blockjob
>>   test-blockjob-txn
>>   test-bufferiszero

What about using gitignore negated pattern in tests/?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Philippe Mathieu-Daudé]

On 05/21/2018 01:41 PM, Eric Blake wrote:
> On 05/21/2018 11:32 AM, Ross Zwisler wrote:
>> After a "make check" we end up with the following:
>>
>> $ git status
>> On branch master
>> Your branch is up-to-date with 'origin/master'.
>>
>> Untracked files:
>>    (use "git add <file>..." to include in what will be committed)
>>
>>     tests/test-block-backend
>>
>> nothing added to commit but untracked files present (use "git add" to
>> track)
>>
>> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Fixes: commit ad0df3e0fdac ("block: test blk_aio_flush() with
>> blk->root == NULL")
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   tests/.gitignore | 1 +
>>   1 file changed, 1 insertion(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index fb62d2299b..2bc61a9a58 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -21,6 +21,7 @@ test-base64
>>   test-bdrv-drain
>>   test-bitops
>>   test-bitcnt
>> +test-block-backend
>>   test-blockjob
>>   test-blockjob-txn
>>   test-bufferiszero

What about using gitignore negated pattern in tests/?

Re: [Qemu-devel] [qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Eric Blake]

On 05/21/2018 12:32 PM, Philippe Mathieu-Daudé wrote:

>>>
>>>      tests/test-block-backend
>>>

>>> +test-block-backend
>>>    test-blockjob
>>>    test-blockjob-txn
>>>    test-bufferiszero
> 
> What about using gitignore negated pattern in tests/?

Or, what we've threatened to do in the past: rename all unit tests to 
the pattern *-test instead of test-*, as a suffix is a lot easier to 
exclude via glob than a prefix.  And while we're renaming things, sort 
tests into separate subdirectories according to whether they are run as 
part of 'make check-unit' or 'make check-qtest'.  But until someone does 
that work, tweaking the .gitignore for individual tests as they keep 
getting added is no worse than what we've been doing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 2/4] tests/.gitignore: add entry for generated file

[Eric Blake]

On 05/21/2018 12:32 PM, Philippe Mathieu-Daudé wrote:

>>>
>>>      tests/test-block-backend
>>>

>>> +test-block-backend
>>>    test-blockjob
>>>    test-blockjob-txn
>>>    test-bufferiszero
> 
> What about using gitignore negated pattern in tests/?

Or, what we've threatened to do in the past: rename all unit tests to 
the pattern *-test instead of test-*, as a suffix is a lot easier to 
exclude via glob than a prefix.  And while we're renaming things, sort 
tests into separate subdirectories according to whether they are run as 
part of 'make check-unit' or 'make check-qtest'.  But until someone does 
that work, tweaking the .gitignore for individual tests as they keep 
getting added is no worse than what we've been doing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

Add a machine command line option to allow the user to control the Platform
Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
Structure was added in ACPI 6.2 Errata A.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 docs/nvdimm.txt         | 27 +++++++++++++++++++++++++++
 hw/acpi/nvdimm.c        | 45 +++++++++++++++++++++++++++++++++++++++++----
 hw/i386/pc.c            | 31 +++++++++++++++++++++++++++++++
 include/hw/i386/pc.h    |  1 +
 include/hw/mem/nvdimm.h |  5 +++++
 5 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index e903d8bb09..8b48fb4633 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -153,3 +153,30 @@ guest NVDIMM region mapping structure.  This unarmed flag indicates
 guest software that this vNVDIMM device contains a region that cannot
 accept persistent writes. In result, for example, the guest Linux
 NVDIMM driver, marks such vNVDIMM device as read-only.
+
+Platform Capabilities
+---------------------
+
+ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
+which allows the platform to communicate what features it supports related to
+NVDIMM data durability.  Users can provide a capabilities value to a guest via
+the optional "nvdimm-cap" machine command line option:
+
+    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
+
+This "nvdimm-cap" field is an integer, and is the combined value of the
+various capability bits defined in table 5-137 of the ACPI 6.2 Errata A spec.
+
+Here is a quick summary of the three bits that are defined as of that spec:
+
+Bit[0] - CPU Cache Flush to NVDIMM Durability on Power Loss Capable.
+Bit[1] - Memory Controller Flush to NVDIMM Durability on Power Loss Capable.
+         Note: If bit 0 is set to 1 then this bit shall be set to 1 as well.
+Bit[2] - Byte Addressable Persistent Memory Hardware Mirroring Capable.
+
+So, a "nvdimm-cap" value of 2 would mean that the platform supports Memory
+Controller Flush on Power Loss, a value of 3 would mean that the platform
+supports CPU Cache Flush and Memory Controller Flush on Power Loss, etc.
+
+For a complete list of the flags available and for more detailed descriptions,
+please consult the ACPI spec.
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 59d6e4254c..87e4280c71 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -169,6 +169,21 @@ struct NvdimmNfitControlRegion {
 } QEMU_PACKED;
 typedef struct NvdimmNfitControlRegion NvdimmNfitControlRegion;
 
+/*
+ * NVDIMM Platform Capabilities Structure
+ *
+ * Defined in section 5.2.25.9 of ACPI 6.2 Errata A, September 2017
+ */
+struct NvdimmNfitPlatformCaps {
+    uint16_t type;
+    uint16_t length;
+    uint8_t highest_cap;
+    uint8_t reserved[3];
+    uint32_t capabilities;
+    uint8_t reserved2[4];
+} QEMU_PACKED;
+typedef struct NvdimmNfitPlatformCaps NvdimmNfitPlatformCaps;
+
 /*
  * Module serial number is a unique number for each device. We use the
  * slot id of NVDIMM device to generate this number so that each device
@@ -351,7 +366,23 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
                                          JEDEC Annex L Release 3. */);
 }
 
-static GArray *nvdimm_build_device_structure(void)
+/*
+ * ACPI 6.2 Errata A: 5.2.25.9 NVDIMM Platform Capabilities Structure
+ */
+static void
+nvdimm_build_structure_caps(GArray *structures, uint32_t capabilities)
+{
+    NvdimmNfitPlatformCaps *nfit_caps;
+
+    nfit_caps = acpi_data_push(structures, sizeof(*nfit_caps));
+
+    nfit_caps->type = cpu_to_le16(7 /* NVDIMM Platform Capabilities */);
+    nfit_caps->length = cpu_to_le16(sizeof(*nfit_caps));
+    nfit_caps->highest_cap = 31 - clz32(capabilities);
+    nfit_caps->capabilities = cpu_to_le32(capabilities);
+}
+
+static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
 {
     GSList *device_list = nvdimm_get_device_list();
     GArray *structures = g_array_new(false, true /* clear */, 1);
@@ -373,6 +404,10 @@ static GArray *nvdimm_build_device_structure(void)
     }
     g_slist_free(device_list);
 
+    if (state->capabilities) {
+        nvdimm_build_structure_caps(structures, state->capabilities);
+    }
+
     return structures;
 }
 
@@ -381,16 +416,18 @@ static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
     fit_buf->fit = g_array_new(false, true /* clear */, 1);
 }
 
-static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
+static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
 {
+    NvdimmFitBuffer *fit_buf = &state->fit_buf;
+
     g_array_free(fit_buf->fit, true);
-    fit_buf->fit = nvdimm_build_device_structure();
+    fit_buf->fit = nvdimm_build_device_structure(state);
     fit_buf->dirty = true;
 }
 
 void nvdimm_plug(AcpiNVDIMMState *state)
 {
-    nvdimm_build_fit_buffer(&state->fit_buf);
+    nvdimm_build_fit_buffer(state);
 }
 
 static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d768930d02..1b2684c549 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2182,6 +2182,33 @@ static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
     pcms->acpi_nvdimm_state.is_enabled = value;
 }
 
+static void pc_machine_get_nvdimm_capabilities(Object *obj, Visitor *v,
+                                               const char *name, void *opaque,
+                                               Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    uint32_t value = pcms->acpi_nvdimm_state.capabilities;
+
+    visit_type_uint32(v, name, &value, errp);
+}
+
+static void pc_machine_set_nvdimm_capabilities(Object *obj, Visitor *v,
+                                               const char *name, void *opaque,
+                                               Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    Error *error = NULL;
+    uint32_t value;
+
+    visit_type_uint32(v, name, &value, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    pcms->acpi_nvdimm_state.capabilities = value;
+}
+
 static bool pc_machine_get_smbus(Object *obj, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
@@ -2395,6 +2422,10 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
         pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
 
+    object_class_property_add(oc, PC_MACHINE_NVDIMM_CAP, "uint32",
+        pc_machine_get_nvdimm_capabilities,
+        pc_machine_set_nvdimm_capabilities, NULL, NULL, &error_abort);
+
     object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
         pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 2a98e3ad68..e9b22f929c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -76,6 +76,7 @@ struct PCMachineState {
 #define PC_MACHINE_VMPORT           "vmport"
 #define PC_MACHINE_SMM              "smm"
 #define PC_MACHINE_NVDIMM           "nvdimm"
+#define PC_MACHINE_NVDIMM_CAP       "nvdimm-cap"
 #define PC_MACHINE_SMBUS            "smbus"
 #define PC_MACHINE_SATA             "sata"
 #define PC_MACHINE_PIT              "pit"
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 74c60332e1..3c82751bab 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -134,6 +134,11 @@ struct AcpiNVDIMMState {
 
     /* the IO region used by OSPM to transfer control to QEMU. */
     MemoryRegion io_mr;
+
+    /*
+     * Platform capabilities, section 5.2.25.9 of ACPI 6.2 Errata A
+     */
+    int32_t capabilities;
 };
 typedef struct AcpiNVDIMMState AcpiNVDIMMState;
 
-- 
2.14.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

Add a machine command line option to allow the user to control the Platform
Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
Structure was added in ACPI 6.2 Errata A.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 docs/nvdimm.txt         | 27 +++++++++++++++++++++++++++
 hw/acpi/nvdimm.c        | 45 +++++++++++++++++++++++++++++++++++++++++----
 hw/i386/pc.c            | 31 +++++++++++++++++++++++++++++++
 include/hw/i386/pc.h    |  1 +
 include/hw/mem/nvdimm.h |  5 +++++
 5 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index e903d8bb09..8b48fb4633 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -153,3 +153,30 @@ guest NVDIMM region mapping structure.  This unarmed flag indicates
 guest software that this vNVDIMM device contains a region that cannot
 accept persistent writes. In result, for example, the guest Linux
 NVDIMM driver, marks such vNVDIMM device as read-only.
+
+Platform Capabilities
+---------------------
+
+ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
+which allows the platform to communicate what features it supports related to
+NVDIMM data durability.  Users can provide a capabilities value to a guest via
+the optional "nvdimm-cap" machine command line option:
+
+    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
+
+This "nvdimm-cap" field is an integer, and is the combined value of the
+various capability bits defined in table 5-137 of the ACPI 6.2 Errata A spec.
+
+Here is a quick summary of the three bits that are defined as of that spec:
+
+Bit[0] - CPU Cache Flush to NVDIMM Durability on Power Loss Capable.
+Bit[1] - Memory Controller Flush to NVDIMM Durability on Power Loss Capable.
+         Note: If bit 0 is set to 1 then this bit shall be set to 1 as well.
+Bit[2] - Byte Addressable Persistent Memory Hardware Mirroring Capable.
+
+So, a "nvdimm-cap" value of 2 would mean that the platform supports Memory
+Controller Flush on Power Loss, a value of 3 would mean that the platform
+supports CPU Cache Flush and Memory Controller Flush on Power Loss, etc.
+
+For a complete list of the flags available and for more detailed descriptions,
+please consult the ACPI spec.
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 59d6e4254c..87e4280c71 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -169,6 +169,21 @@ struct NvdimmNfitControlRegion {
 } QEMU_PACKED;
 typedef struct NvdimmNfitControlRegion NvdimmNfitControlRegion;
 
+/*
+ * NVDIMM Platform Capabilities Structure
+ *
+ * Defined in section 5.2.25.9 of ACPI 6.2 Errata A, September 2017
+ */
+struct NvdimmNfitPlatformCaps {
+    uint16_t type;
+    uint16_t length;
+    uint8_t highest_cap;
+    uint8_t reserved[3];
+    uint32_t capabilities;
+    uint8_t reserved2[4];
+} QEMU_PACKED;
+typedef struct NvdimmNfitPlatformCaps NvdimmNfitPlatformCaps;
+
 /*
  * Module serial number is a unique number for each device. We use the
  * slot id of NVDIMM device to generate this number so that each device
@@ -351,7 +366,23 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
                                          JEDEC Annex L Release 3. */);
 }
 
-static GArray *nvdimm_build_device_structure(void)
+/*
+ * ACPI 6.2 Errata A: 5.2.25.9 NVDIMM Platform Capabilities Structure
+ */
+static void
+nvdimm_build_structure_caps(GArray *structures, uint32_t capabilities)
+{
+    NvdimmNfitPlatformCaps *nfit_caps;
+
+    nfit_caps = acpi_data_push(structures, sizeof(*nfit_caps));
+
+    nfit_caps->type = cpu_to_le16(7 /* NVDIMM Platform Capabilities */);
+    nfit_caps->length = cpu_to_le16(sizeof(*nfit_caps));
+    nfit_caps->highest_cap = 31 - clz32(capabilities);
+    nfit_caps->capabilities = cpu_to_le32(capabilities);
+}
+
+static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
 {
     GSList *device_list = nvdimm_get_device_list();
     GArray *structures = g_array_new(false, true /* clear */, 1);
@@ -373,6 +404,10 @@ static GArray *nvdimm_build_device_structure(void)
     }
     g_slist_free(device_list);
 
+    if (state->capabilities) {
+        nvdimm_build_structure_caps(structures, state->capabilities);
+    }
+
     return structures;
 }
 
@@ -381,16 +416,18 @@ static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
     fit_buf->fit = g_array_new(false, true /* clear */, 1);
 }
 
-static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
+static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
 {
+    NvdimmFitBuffer *fit_buf = &state->fit_buf;
+
     g_array_free(fit_buf->fit, true);
-    fit_buf->fit = nvdimm_build_device_structure();
+    fit_buf->fit = nvdimm_build_device_structure(state);
     fit_buf->dirty = true;
 }
 
 void nvdimm_plug(AcpiNVDIMMState *state)
 {
-    nvdimm_build_fit_buffer(&state->fit_buf);
+    nvdimm_build_fit_buffer(state);
 }
 
 static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d768930d02..1b2684c549 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2182,6 +2182,33 @@ static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
     pcms->acpi_nvdimm_state.is_enabled = value;
 }
 
+static void pc_machine_get_nvdimm_capabilities(Object *obj, Visitor *v,
+                                               const char *name, void *opaque,
+                                               Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    uint32_t value = pcms->acpi_nvdimm_state.capabilities;
+
+    visit_type_uint32(v, name, &value, errp);
+}
+
+static void pc_machine_set_nvdimm_capabilities(Object *obj, Visitor *v,
+                                               const char *name, void *opaque,
+                                               Error **errp)
+{
+    PCMachineState *pcms = PC_MACHINE(obj);
+    Error *error = NULL;
+    uint32_t value;
+
+    visit_type_uint32(v, name, &value, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    pcms->acpi_nvdimm_state.capabilities = value;
+}
+
 static bool pc_machine_get_smbus(Object *obj, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(obj);
@@ -2395,6 +2422,10 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
         pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
 
+    object_class_property_add(oc, PC_MACHINE_NVDIMM_CAP, "uint32",
+        pc_machine_get_nvdimm_capabilities,
+        pc_machine_set_nvdimm_capabilities, NULL, NULL, &error_abort);
+
     object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
         pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 2a98e3ad68..e9b22f929c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -76,6 +76,7 @@ struct PCMachineState {
 #define PC_MACHINE_VMPORT           "vmport"
 #define PC_MACHINE_SMM              "smm"
 #define PC_MACHINE_NVDIMM           "nvdimm"
+#define PC_MACHINE_NVDIMM_CAP       "nvdimm-cap"
 #define PC_MACHINE_SMBUS            "smbus"
 #define PC_MACHINE_SATA             "sata"
 #define PC_MACHINE_PIT              "pit"
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 74c60332e1..3c82751bab 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -134,6 +134,11 @@ struct AcpiNVDIMMState {
 
     /* the IO region used by OSPM to transfer control to QEMU. */
     MemoryRegion io_mr;
+
+    /*
+     * Platform capabilities, section 5.2.25.9 of ACPI 6.2 Errata A
+     */
+    int32_t capabilities;
 };
 typedef struct AcpiNVDIMMState AcpiNVDIMMState;
 
-- 
2.14.3

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> Add a machine command line option to allow the user to control the Platform
> Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> Structure was added in ACPI 6.2 Errata A.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>

I tried playing with it and encoding the capabilities is
quite awkward.

Can we add bits for specific capabilities instead of nvdimm-cap?

How about:

"cpu-flush-on-power-loss-cap"
"memory-flush-on-power-loss-cap"
"byte-addressable-mirroring-cap"

?



> ---
>  docs/nvdimm.txt         | 27 +++++++++++++++++++++++++++
>  hw/acpi/nvdimm.c        | 45 +++++++++++++++++++++++++++++++++++++++++----
>  hw/i386/pc.c            | 31 +++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h    |  1 +
>  include/hw/mem/nvdimm.h |  5 +++++
>  5 files changed, 105 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index e903d8bb09..8b48fb4633 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -153,3 +153,30 @@ guest NVDIMM region mapping structure.  This unarmed flag indicates
>  guest software that this vNVDIMM device contains a region that cannot
>  accept persistent writes. In result, for example, the guest Linux
>  NVDIMM driver, marks such vNVDIMM device as read-only.
> +
> +Platform Capabilities
> +---------------------
> +
> +ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
> +which allows the platform to communicate what features it supports related to
> +NVDIMM data durability.  Users can provide a capabilities value to a guest via
> +the optional "nvdimm-cap" machine command line option:
> +
> +    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> +
> +This "nvdimm-cap" field is an integer, and is the combined value of the
> +various capability bits defined in table 5-137 of the ACPI 6.2 Errata A spec.
> +
> +Here is a quick summary of the three bits that are defined as of that spec:
> +
> +Bit[0] - CPU Cache Flush to NVDIMM Durability on Power Loss Capable.
> +Bit[1] - Memory Controller Flush to NVDIMM Durability on Power Loss Capable.
> +         Note: If bit 0 is set to 1 then this bit shall be set to 1 as well.
> +Bit[2] - Byte Addressable Persistent Memory Hardware Mirroring Capable.
> +
> +So, a "nvdimm-cap" value of 2 would mean that the platform supports Memory
> +Controller Flush on Power Loss, a value of 3 would mean that the platform
> +supports CPU Cache Flush and Memory Controller Flush on Power Loss, etc.
> +
> +For a complete list of the flags available and for more detailed descriptions,
> +please consult the ACPI spec.
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 59d6e4254c..87e4280c71 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -169,6 +169,21 @@ struct NvdimmNfitControlRegion {
>  } QEMU_PACKED;
>  typedef struct NvdimmNfitControlRegion NvdimmNfitControlRegion;
>  
> +/*
> + * NVDIMM Platform Capabilities Structure
> + *
> + * Defined in section 5.2.25.9 of ACPI 6.2 Errata A, September 2017
> + */
> +struct NvdimmNfitPlatformCaps {
> +    uint16_t type;
> +    uint16_t length;
> +    uint8_t highest_cap;
> +    uint8_t reserved[3];
> +    uint32_t capabilities;
> +    uint8_t reserved2[4];
> +} QEMU_PACKED;
> +typedef struct NvdimmNfitPlatformCaps NvdimmNfitPlatformCaps;
> +
>  /*
>   * Module serial number is a unique number for each device. We use the
>   * slot id of NVDIMM device to generate this number so that each device
> @@ -351,7 +366,23 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
>                                           JEDEC Annex L Release 3. */);
>  }
>  
> -static GArray *nvdimm_build_device_structure(void)
> +/*
> + * ACPI 6.2 Errata A: 5.2.25.9 NVDIMM Platform Capabilities Structure
> + */
> +static void
> +nvdimm_build_structure_caps(GArray *structures, uint32_t capabilities)
> +{
> +    NvdimmNfitPlatformCaps *nfit_caps;
> +
> +    nfit_caps = acpi_data_push(structures, sizeof(*nfit_caps));
> +
> +    nfit_caps->type = cpu_to_le16(7 /* NVDIMM Platform Capabilities */);
> +    nfit_caps->length = cpu_to_le16(sizeof(*nfit_caps));
> +    nfit_caps->highest_cap = 31 - clz32(capabilities);
> +    nfit_caps->capabilities = cpu_to_le32(capabilities);
> +}
> +
> +static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
>  {
>      GSList *device_list = nvdimm_get_device_list();
>      GArray *structures = g_array_new(false, true /* clear */, 1);
> @@ -373,6 +404,10 @@ static GArray *nvdimm_build_device_structure(void)
>      }
>      g_slist_free(device_list);
>  
> +    if (state->capabilities) {
> +        nvdimm_build_structure_caps(structures, state->capabilities);
> +    }
> +
>      return structures;
>  }
>  
> @@ -381,16 +416,18 @@ static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
>      fit_buf->fit = g_array_new(false, true /* clear */, 1);
>  }
>  
> -static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
> +static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
>  {
> +    NvdimmFitBuffer *fit_buf = &state->fit_buf;
> +
>      g_array_free(fit_buf->fit, true);
> -    fit_buf->fit = nvdimm_build_device_structure();
> +    fit_buf->fit = nvdimm_build_device_structure(state);
>      fit_buf->dirty = true;
>  }
>  
>  void nvdimm_plug(AcpiNVDIMMState *state)
>  {
> -    nvdimm_build_fit_buffer(&state->fit_buf);
> +    nvdimm_build_fit_buffer(state);
>  }
>  
>  static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d768930d02..1b2684c549 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2182,6 +2182,33 @@ static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
>      pcms->acpi_nvdimm_state.is_enabled = value;
>  }
>  
> +static void pc_machine_get_nvdimm_capabilities(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    uint32_t value = pcms->acpi_nvdimm_state.capabilities;
> +
> +    visit_type_uint32(v, name, &value, errp);
> +}
> +
> +static void pc_machine_set_nvdimm_capabilities(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    Error *error = NULL;
> +    uint32_t value;
> +
> +    visit_type_uint32(v, name, &value, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    pcms->acpi_nvdimm_state.capabilities = value;
> +}
> +
>  static bool pc_machine_get_smbus(Object *obj, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> @@ -2395,6 +2422,10 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
>          pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
>  
> +    object_class_property_add(oc, PC_MACHINE_NVDIMM_CAP, "uint32",
> +        pc_machine_get_nvdimm_capabilities,
> +        pc_machine_set_nvdimm_capabilities, NULL, NULL, &error_abort);
> +
>      object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
>          pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 2a98e3ad68..e9b22f929c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -76,6 +76,7 @@ struct PCMachineState {
>  #define PC_MACHINE_VMPORT           "vmport"
>  #define PC_MACHINE_SMM              "smm"
>  #define PC_MACHINE_NVDIMM           "nvdimm"
> +#define PC_MACHINE_NVDIMM_CAP       "nvdimm-cap"
>  #define PC_MACHINE_SMBUS            "smbus"
>  #define PC_MACHINE_SATA             "sata"
>  #define PC_MACHINE_PIT              "pit"
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 74c60332e1..3c82751bab 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -134,6 +134,11 @@ struct AcpiNVDIMMState {
>  
>      /* the IO region used by OSPM to transfer control to QEMU. */
>      MemoryRegion io_mr;
> +
> +    /*
> +     * Platform capabilities, section 5.2.25.9 of ACPI 6.2 Errata A
> +     */
> +    int32_t capabilities;
>  };
>  typedef struct AcpiNVDIMMState AcpiNVDIMMState;
>  
> -- 
> 2.14.3
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> Add a machine command line option to allow the user to control the Platform
> Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> Structure was added in ACPI 6.2 Errata A.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>

I tried playing with it and encoding the capabilities is
quite awkward.

Can we add bits for specific capabilities instead of nvdimm-cap?

How about:

"cpu-flush-on-power-loss-cap"
"memory-flush-on-power-loss-cap"
"byte-addressable-mirroring-cap"

?



> ---
>  docs/nvdimm.txt         | 27 +++++++++++++++++++++++++++
>  hw/acpi/nvdimm.c        | 45 +++++++++++++++++++++++++++++++++++++++++----
>  hw/i386/pc.c            | 31 +++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h    |  1 +
>  include/hw/mem/nvdimm.h |  5 +++++
>  5 files changed, 105 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index e903d8bb09..8b48fb4633 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -153,3 +153,30 @@ guest NVDIMM region mapping structure.  This unarmed flag indicates
>  guest software that this vNVDIMM device contains a region that cannot
>  accept persistent writes. In result, for example, the guest Linux
>  NVDIMM driver, marks such vNVDIMM device as read-only.
> +
> +Platform Capabilities
> +---------------------
> +
> +ACPI 6.2 Errata A added support for a new Platform Capabilities Structure
> +which allows the platform to communicate what features it supports related to
> +NVDIMM data durability.  Users can provide a capabilities value to a guest via
> +the optional "nvdimm-cap" machine command line option:
> +
> +    -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> +
> +This "nvdimm-cap" field is an integer, and is the combined value of the
> +various capability bits defined in table 5-137 of the ACPI 6.2 Errata A spec.
> +
> +Here is a quick summary of the three bits that are defined as of that spec:
> +
> +Bit[0] - CPU Cache Flush to NVDIMM Durability on Power Loss Capable.
> +Bit[1] - Memory Controller Flush to NVDIMM Durability on Power Loss Capable.
> +         Note: If bit 0 is set to 1 then this bit shall be set to 1 as well.
> +Bit[2] - Byte Addressable Persistent Memory Hardware Mirroring Capable.
> +
> +So, a "nvdimm-cap" value of 2 would mean that the platform supports Memory
> +Controller Flush on Power Loss, a value of 3 would mean that the platform
> +supports CPU Cache Flush and Memory Controller Flush on Power Loss, etc.
> +
> +For a complete list of the flags available and for more detailed descriptions,
> +please consult the ACPI spec.
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 59d6e4254c..87e4280c71 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -169,6 +169,21 @@ struct NvdimmNfitControlRegion {
>  } QEMU_PACKED;
>  typedef struct NvdimmNfitControlRegion NvdimmNfitControlRegion;
>  
> +/*
> + * NVDIMM Platform Capabilities Structure
> + *
> + * Defined in section 5.2.25.9 of ACPI 6.2 Errata A, September 2017
> + */
> +struct NvdimmNfitPlatformCaps {
> +    uint16_t type;
> +    uint16_t length;
> +    uint8_t highest_cap;
> +    uint8_t reserved[3];
> +    uint32_t capabilities;
> +    uint8_t reserved2[4];
> +} QEMU_PACKED;
> +typedef struct NvdimmNfitPlatformCaps NvdimmNfitPlatformCaps;
> +
>  /*
>   * Module serial number is a unique number for each device. We use the
>   * slot id of NVDIMM device to generate this number so that each device
> @@ -351,7 +366,23 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev)
>                                           JEDEC Annex L Release 3. */);
>  }
>  
> -static GArray *nvdimm_build_device_structure(void)
> +/*
> + * ACPI 6.2 Errata A: 5.2.25.9 NVDIMM Platform Capabilities Structure
> + */
> +static void
> +nvdimm_build_structure_caps(GArray *structures, uint32_t capabilities)
> +{
> +    NvdimmNfitPlatformCaps *nfit_caps;
> +
> +    nfit_caps = acpi_data_push(structures, sizeof(*nfit_caps));
> +
> +    nfit_caps->type = cpu_to_le16(7 /* NVDIMM Platform Capabilities */);
> +    nfit_caps->length = cpu_to_le16(sizeof(*nfit_caps));
> +    nfit_caps->highest_cap = 31 - clz32(capabilities);
> +    nfit_caps->capabilities = cpu_to_le32(capabilities);
> +}
> +
> +static GArray *nvdimm_build_device_structure(AcpiNVDIMMState *state)
>  {
>      GSList *device_list = nvdimm_get_device_list();
>      GArray *structures = g_array_new(false, true /* clear */, 1);
> @@ -373,6 +404,10 @@ static GArray *nvdimm_build_device_structure(void)
>      }
>      g_slist_free(device_list);
>  
> +    if (state->capabilities) {
> +        nvdimm_build_structure_caps(structures, state->capabilities);
> +    }
> +
>      return structures;
>  }
>  
> @@ -381,16 +416,18 @@ static void nvdimm_init_fit_buffer(NvdimmFitBuffer *fit_buf)
>      fit_buf->fit = g_array_new(false, true /* clear */, 1);
>  }
>  
> -static void nvdimm_build_fit_buffer(NvdimmFitBuffer *fit_buf)
> +static void nvdimm_build_fit_buffer(AcpiNVDIMMState *state)
>  {
> +    NvdimmFitBuffer *fit_buf = &state->fit_buf;
> +
>      g_array_free(fit_buf->fit, true);
> -    fit_buf->fit = nvdimm_build_device_structure();
> +    fit_buf->fit = nvdimm_build_device_structure(state);
>      fit_buf->dirty = true;
>  }
>  
>  void nvdimm_plug(AcpiNVDIMMState *state)
>  {
> -    nvdimm_build_fit_buffer(&state->fit_buf);
> +    nvdimm_build_fit_buffer(state);
>  }
>  
>  static void nvdimm_build_nfit(AcpiNVDIMMState *state, GArray *table_offsets,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index d768930d02..1b2684c549 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2182,6 +2182,33 @@ static void pc_machine_set_nvdimm(Object *obj, bool value, Error **errp)
>      pcms->acpi_nvdimm_state.is_enabled = value;
>  }
>  
> +static void pc_machine_get_nvdimm_capabilities(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    uint32_t value = pcms->acpi_nvdimm_state.capabilities;
> +
> +    visit_type_uint32(v, name, &value, errp);
> +}
> +
> +static void pc_machine_set_nvdimm_capabilities(Object *obj, Visitor *v,
> +                                               const char *name, void *opaque,
> +                                               Error **errp)
> +{
> +    PCMachineState *pcms = PC_MACHINE(obj);
> +    Error *error = NULL;
> +    uint32_t value;
> +
> +    visit_type_uint32(v, name, &value, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    pcms->acpi_nvdimm_state.capabilities = value;
> +}
> +
>  static bool pc_machine_get_smbus(Object *obj, Error **errp)
>  {
>      PCMachineState *pcms = PC_MACHINE(obj);
> @@ -2395,6 +2422,10 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      object_class_property_add_bool(oc, PC_MACHINE_NVDIMM,
>          pc_machine_get_nvdimm, pc_machine_set_nvdimm, &error_abort);
>  
> +    object_class_property_add(oc, PC_MACHINE_NVDIMM_CAP, "uint32",
> +        pc_machine_get_nvdimm_capabilities,
> +        pc_machine_set_nvdimm_capabilities, NULL, NULL, &error_abort);
> +
>      object_class_property_add_bool(oc, PC_MACHINE_SMBUS,
>          pc_machine_get_smbus, pc_machine_set_smbus, &error_abort);
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 2a98e3ad68..e9b22f929c 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -76,6 +76,7 @@ struct PCMachineState {
>  #define PC_MACHINE_VMPORT           "vmport"
>  #define PC_MACHINE_SMM              "smm"
>  #define PC_MACHINE_NVDIMM           "nvdimm"
> +#define PC_MACHINE_NVDIMM_CAP       "nvdimm-cap"
>  #define PC_MACHINE_SMBUS            "smbus"
>  #define PC_MACHINE_SATA             "sata"
>  #define PC_MACHINE_PIT              "pit"
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index 74c60332e1..3c82751bab 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -134,6 +134,11 @@ struct AcpiNVDIMMState {
>  
>      /* the IO region used by OSPM to transfer control to QEMU. */
>      MemoryRegion io_mr;
> +
> +    /*
> +     * Platform capabilities, section 5.2.25.9 of ACPI 6.2 Errata A
> +     */
> +    int32_t capabilities;
>  };
>  typedef struct AcpiNVDIMMState AcpiNVDIMMState;
>  
> -- 
> 2.14.3

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > Add a machine command line option to allow the user to control the Platform
> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > Structure was added in ACPI 6.2 Errata A.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> I tried playing with it and encoding the capabilities is
> quite awkward.
> 
> Can we add bits for specific capabilities instead of nvdimm-cap?
> 
> How about:
> 
> "cpu-flush-on-power-loss-cap"
> "memory-flush-on-power-loss-cap"
> "byte-addressable-mirroring-cap"

Hmmm...I don't like that as much because:

a) It's very verbose.  Looking at my current qemu command line few other
   options require that many characters, and you'd commonly be defining more
   than one of these for a given VM.

b) It means that the QEMU will need to be updated if/when new flags are added,
   because we'll have to have new options for each flag.  The current
   implementation is more future-proof because you can specify any flags
   value you want.

However, if you feel strongly about this, I'll make the change.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > Add a machine command line option to allow the user to control the Platform
> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > Structure was added in ACPI 6.2 Errata A.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> I tried playing with it and encoding the capabilities is
> quite awkward.
> 
> Can we add bits for specific capabilities instead of nvdimm-cap?
> 
> How about:
> 
> "cpu-flush-on-power-loss-cap"
> "memory-flush-on-power-loss-cap"
> "byte-addressable-mirroring-cap"

Hmmm...I don't like that as much because:

a) It's very verbose.  Looking at my current qemu command line few other
   options require that many characters, and you'd commonly be defining more
   than one of these for a given VM.

b) It means that the QEMU will need to be updated if/when new flags are added,
   because we'll have to have new options for each flag.  The current
   implementation is more future-proof because you can specify any flags
   value you want.

However, if you feel strongly about this, I'll make the change.

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Dan Williams]

On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
>> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
>> > Add a machine command line option to allow the user to control the Platform
>> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
>> > Structure was added in ACPI 6.2 Errata A.
>> >
>> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>>
>> I tried playing with it and encoding the capabilities is
>> quite awkward.
>>
>> Can we add bits for specific capabilities instead of nvdimm-cap?
>>
>> How about:
>>
>> "cpu-flush-on-power-loss-cap"
>> "memory-flush-on-power-loss-cap"
>> "byte-addressable-mirroring-cap"
>
> Hmmm...I don't like that as much because:
>
> a) It's very verbose.  Looking at my current qemu command line few other
>    options require that many characters, and you'd commonly be defining more
>    than one of these for a given VM.
>
> b) It means that the QEMU will need to be updated if/when new flags are added,
>    because we'll have to have new options for each flag.  The current
>    implementation is more future-proof because you can specify any flags
>    value you want.
>
> However, if you feel strongly about this, I'll make the change.

Straw-man: Could we do something similar with what we are doing in ndctl?

enum ndctl_persistence_domain {
        PERSISTENCE_NONE = 0,
        PERSISTENCE_MEM_CTRL = 10,
        PERSISTENCE_CPU_CACHE = 20,
        PERSISTENCE_UNKNOWN = INT_MAX,
};

...and have the command line take a number where "10" and "20" are
supported today, but allows us to adapt to new persistence domains in
the future.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Dan Williams]

On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
>> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
>> > Add a machine command line option to allow the user to control the Platform
>> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
>> > Structure was added in ACPI 6.2 Errata A.
>> >
>> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>>
>> I tried playing with it and encoding the capabilities is
>> quite awkward.
>>
>> Can we add bits for specific capabilities instead of nvdimm-cap?
>>
>> How about:
>>
>> "cpu-flush-on-power-loss-cap"
>> "memory-flush-on-power-loss-cap"
>> "byte-addressable-mirroring-cap"
>
> Hmmm...I don't like that as much because:
>
> a) It's very verbose.  Looking at my current qemu command line few other
>    options require that many characters, and you'd commonly be defining more
>    than one of these for a given VM.
>
> b) It means that the QEMU will need to be updated if/when new flags are added,
>    because we'll have to have new options for each flag.  The current
>    implementation is more future-proof because you can specify any flags
>    value you want.
>
> However, if you feel strongly about this, I'll make the change.

Straw-man: Could we do something similar with what we are doing in ndctl?

enum ndctl_persistence_domain {
        PERSISTENCE_NONE = 0,
        PERSISTENCE_MEM_CTRL = 10,
        PERSISTENCE_CPU_CACHE = 20,
        PERSISTENCE_UNKNOWN = INT_MAX,
};

...and have the command line take a number where "10" and "20" are
supported today, but allows us to adapt to new persistence domains in
the future.

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> >> > Add a machine command line option to allow the user to control the Platform
> >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> >> > Structure was added in ACPI 6.2 Errata A.
> >> >
> >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> >>
> >> I tried playing with it and encoding the capabilities is
> >> quite awkward.
> >>
> >> Can we add bits for specific capabilities instead of nvdimm-cap?
> >>
> >> How about:
> >>
> >> "cpu-flush-on-power-loss-cap"
> >> "memory-flush-on-power-loss-cap"
> >> "byte-addressable-mirroring-cap"
> >
> > Hmmm...I don't like that as much because:
> >
> > a) It's very verbose.  Looking at my current qemu command line few other
> >    options require that many characters, and you'd commonly be defining more
> >    than one of these for a given VM.
> >
> > b) It means that the QEMU will need to be updated if/when new flags are added,
> >    because we'll have to have new options for each flag.  The current
> >    implementation is more future-proof because you can specify any flags
> >    value you want.
> >
> > However, if you feel strongly about this, I'll make the change.
> 
> Straw-man: Could we do something similar with what we are doing in ndctl?
> 
> enum ndctl_persistence_domain {
>         PERSISTENCE_NONE = 0,
>         PERSISTENCE_MEM_CTRL = 10,
>         PERSISTENCE_CPU_CACHE = 20,
>         PERSISTENCE_UNKNOWN = INT_MAX,
> };
> 
> ...and have the command line take a number where "10" and "20" are
> supported today, but allows us to adapt to new persistence domains in
> the future.

I'm fine with that except can we have symbolic names instead of numbers
on command line?

-- 
MST
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> >> > Add a machine command line option to allow the user to control the Platform
> >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> >> > Structure was added in ACPI 6.2 Errata A.
> >> >
> >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> >>
> >> I tried playing with it and encoding the capabilities is
> >> quite awkward.
> >>
> >> Can we add bits for specific capabilities instead of nvdimm-cap?
> >>
> >> How about:
> >>
> >> "cpu-flush-on-power-loss-cap"
> >> "memory-flush-on-power-loss-cap"
> >> "byte-addressable-mirroring-cap"
> >
> > Hmmm...I don't like that as much because:
> >
> > a) It's very verbose.  Looking at my current qemu command line few other
> >    options require that many characters, and you'd commonly be defining more
> >    than one of these for a given VM.
> >
> > b) It means that the QEMU will need to be updated if/when new flags are added,
> >    because we'll have to have new options for each flag.  The current
> >    implementation is more future-proof because you can specify any flags
> >    value you want.
> >
> > However, if you feel strongly about this, I'll make the change.
> 
> Straw-man: Could we do something similar with what we are doing in ndctl?
> 
> enum ndctl_persistence_domain {
>         PERSISTENCE_NONE = 0,
>         PERSISTENCE_MEM_CTRL = 10,
>         PERSISTENCE_CPU_CACHE = 20,
>         PERSISTENCE_UNKNOWN = INT_MAX,
> };
> 
> ...and have the command line take a number where "10" and "20" are
> supported today, but allows us to adapt to new persistence domains in
> the future.

I'm fine with that except can we have symbolic names instead of numbers
on command line?

-- 
MST

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > >> > Add a machine command line option to allow the user to control the Platform
> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > >> > Structure was added in ACPI 6.2 Errata A.
> > >> >
> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > >>
> > >> I tried playing with it and encoding the capabilities is
> > >> quite awkward.
> > >>
> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > >>
> > >> How about:
> > >>
> > >> "cpu-flush-on-power-loss-cap"
> > >> "memory-flush-on-power-loss-cap"
> > >> "byte-addressable-mirroring-cap"
> > >
> > > Hmmm...I don't like that as much because:
> > >
> > > a) It's very verbose.  Looking at my current qemu command line few other
> > >    options require that many characters, and you'd commonly be defining more
> > >    than one of these for a given VM.
> > >
> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > >    because we'll have to have new options for each flag.  The current
> > >    implementation is more future-proof because you can specify any flags
> > >    value you want.
> > >
> > > However, if you feel strongly about this, I'll make the change.
> > 
> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > 
> > enum ndctl_persistence_domain {
> >         PERSISTENCE_NONE = 0,
> >         PERSISTENCE_MEM_CTRL = 10,
> >         PERSISTENCE_CPU_CACHE = 20,
> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > };
> > 
> > ...and have the command line take a number where "10" and "20" are
> > supported today, but allows us to adapt to new persistence domains in
> > the future.
> 
> I'm fine with that except can we have symbolic names instead of numbers
> on command line?
> 
> -- 
> MST

Okay, we can move to the symbolic names.  Do you want them to be that long, or
would:

nvdimm-cap-cpu
nvdimm-cap-mem-ctrl
nvdimm-cap-mirroring

or something be better?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > >> > Add a machine command line option to allow the user to control the Platform
> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > >> > Structure was added in ACPI 6.2 Errata A.
> > >> >
> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > >>
> > >> I tried playing with it and encoding the capabilities is
> > >> quite awkward.
> > >>
> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > >>
> > >> How about:
> > >>
> > >> "cpu-flush-on-power-loss-cap"
> > >> "memory-flush-on-power-loss-cap"
> > >> "byte-addressable-mirroring-cap"
> > >
> > > Hmmm...I don't like that as much because:
> > >
> > > a) It's very verbose.  Looking at my current qemu command line few other
> > >    options require that many characters, and you'd commonly be defining more
> > >    than one of these for a given VM.
> > >
> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > >    because we'll have to have new options for each flag.  The current
> > >    implementation is more future-proof because you can specify any flags
> > >    value you want.
> > >
> > > However, if you feel strongly about this, I'll make the change.
> > 
> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > 
> > enum ndctl_persistence_domain {
> >         PERSISTENCE_NONE = 0,
> >         PERSISTENCE_MEM_CTRL = 10,
> >         PERSISTENCE_CPU_CACHE = 20,
> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > };
> > 
> > ...and have the command line take a number where "10" and "20" are
> > supported today, but allows us to adapt to new persistence domains in
> > the future.
> 
> I'm fine with that except can we have symbolic names instead of numbers
> on command line?
> 
> -- 
> MST

Okay, we can move to the symbolic names.  Do you want them to be that long, or
would:

nvdimm-cap-cpu
nvdimm-cap-mem-ctrl
nvdimm-cap-mirroring

or something be better?

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Dan Williams]

On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
>> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
>> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
>> > <ross.zwisler@linux.intel.com> wrote:
>> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
>> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
>> > >> > Add a machine command line option to allow the user to control the Platform
>> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
>> > >> > Structure was added in ACPI 6.2 Errata A.
>> > >> >
>> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>> > >>
>> > >> I tried playing with it and encoding the capabilities is
>> > >> quite awkward.
>> > >>
>> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
>> > >>
>> > >> How about:
>> > >>
>> > >> "cpu-flush-on-power-loss-cap"
>> > >> "memory-flush-on-power-loss-cap"
>> > >> "byte-addressable-mirroring-cap"
>> > >
>> > > Hmmm...I don't like that as much because:
>> > >
>> > > a) It's very verbose.  Looking at my current qemu command line few other
>> > >    options require that many characters, and you'd commonly be defining more
>> > >    than one of these for a given VM.
>> > >
>> > > b) It means that the QEMU will need to be updated if/when new flags are added,
>> > >    because we'll have to have new options for each flag.  The current
>> > >    implementation is more future-proof because you can specify any flags
>> > >    value you want.
>> > >
>> > > However, if you feel strongly about this, I'll make the change.
>> >
>> > Straw-man: Could we do something similar with what we are doing in ndctl?
>> >
>> > enum ndctl_persistence_domain {
>> >         PERSISTENCE_NONE = 0,
>> >         PERSISTENCE_MEM_CTRL = 10,
>> >         PERSISTENCE_CPU_CACHE = 20,
>> >         PERSISTENCE_UNKNOWN = INT_MAX,
>> > };
>> >
>> > ...and have the command line take a number where "10" and "20" are
>> > supported today, but allows us to adapt to new persistence domains in
>> > the future.
>>
>> I'm fine with that except can we have symbolic names instead of numbers
>> on command line?
>>
>> --
>> MST
>
> Okay, we can move to the symbolic names.  Do you want them to be that long, or
> would:
>
> nvdimm-cap-cpu
> nvdimm-cap-mem-ctrl
> nvdimm-cap-mirroring

Wait, why is mirroring part of this?

I was thinking this option would be:

    --persistence-domain={cpu,mem-ctrl}

...and try not to let ACPI specifics leak into the qemu command line
interface. For example PowerPC qemu could have a persistence domain
communicated via Open Firmware or some other mechanism.
>
> or something be better?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Dan Williams]

On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
>> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
>> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
>> > <ross.zwisler@linux.intel.com> wrote:
>> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
>> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
>> > >> > Add a machine command line option to allow the user to control the Platform
>> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
>> > >> > Structure was added in ACPI 6.2 Errata A.
>> > >> >
>> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>> > >>
>> > >> I tried playing with it and encoding the capabilities is
>> > >> quite awkward.
>> > >>
>> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
>> > >>
>> > >> How about:
>> > >>
>> > >> "cpu-flush-on-power-loss-cap"
>> > >> "memory-flush-on-power-loss-cap"
>> > >> "byte-addressable-mirroring-cap"
>> > >
>> > > Hmmm...I don't like that as much because:
>> > >
>> > > a) It's very verbose.  Looking at my current qemu command line few other
>> > >    options require that many characters, and you'd commonly be defining more
>> > >    than one of these for a given VM.
>> > >
>> > > b) It means that the QEMU will need to be updated if/when new flags are added,
>> > >    because we'll have to have new options for each flag.  The current
>> > >    implementation is more future-proof because you can specify any flags
>> > >    value you want.
>> > >
>> > > However, if you feel strongly about this, I'll make the change.
>> >
>> > Straw-man: Could we do something similar with what we are doing in ndctl?
>> >
>> > enum ndctl_persistence_domain {
>> >         PERSISTENCE_NONE = 0,
>> >         PERSISTENCE_MEM_CTRL = 10,
>> >         PERSISTENCE_CPU_CACHE = 20,
>> >         PERSISTENCE_UNKNOWN = INT_MAX,
>> > };
>> >
>> > ...and have the command line take a number where "10" and "20" are
>> > supported today, but allows us to adapt to new persistence domains in
>> > the future.
>>
>> I'm fine with that except can we have symbolic names instead of numbers
>> on command line?
>>
>> --
>> MST
>
> Okay, we can move to the symbolic names.  Do you want them to be that long, or
> would:
>
> nvdimm-cap-cpu
> nvdimm-cap-mem-ctrl
> nvdimm-cap-mirroring

Wait, why is mirroring part of this?

I was thinking this option would be:

    --persistence-domain={cpu,mem-ctrl}

...and try not to let ACPI specifics leak into the qemu command line
interface. For example PowerPC qemu could have a persistence domain
communicated via Open Firmware or some other mechanism.
>
> or something be better?

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> >> > <ross.zwisler@linux.intel.com> wrote:
> >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> >> > >> > Add a machine command line option to allow the user to control the Platform
> >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> >> > >> > Structure was added in ACPI 6.2 Errata A.
> >> > >> >
> >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> >> > >>
> >> > >> I tried playing with it and encoding the capabilities is
> >> > >> quite awkward.
> >> > >>
> >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> >> > >>
> >> > >> How about:
> >> > >>
> >> > >> "cpu-flush-on-power-loss-cap"
> >> > >> "memory-flush-on-power-loss-cap"
> >> > >> "byte-addressable-mirroring-cap"
> >> > >
> >> > > Hmmm...I don't like that as much because:
> >> > >
> >> > > a) It's very verbose.  Looking at my current qemu command line few other
> >> > >    options require that many characters, and you'd commonly be defining more
> >> > >    than one of these for a given VM.
> >> > >
> >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> >> > >    because we'll have to have new options for each flag.  The current
> >> > >    implementation is more future-proof because you can specify any flags
> >> > >    value you want.
> >> > >
> >> > > However, if you feel strongly about this, I'll make the change.
> >> >
> >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> >> >
> >> > enum ndctl_persistence_domain {
> >> >         PERSISTENCE_NONE = 0,
> >> >         PERSISTENCE_MEM_CTRL = 10,
> >> >         PERSISTENCE_CPU_CACHE = 20,
> >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> >> > };
> >> >
> >> > ...and have the command line take a number where "10" and "20" are
> >> > supported today, but allows us to adapt to new persistence domains in
> >> > the future.
> >>
> >> I'm fine with that except can we have symbolic names instead of numbers
> >> on command line?
> >>
> >> --
> >> MST
> >
> > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > would:
> >
> > nvdimm-cap-cpu
> > nvdimm-cap-mem-ctrl
> > nvdimm-cap-mirroring
> 
> Wait, why is mirroring part of this?
> 
> I was thinking this option would be:
> 
>     --persistence-domain={cpu,mem-ctrl}
> 
> ...and try not to let ACPI specifics leak into the qemu command line
> interface. For example PowerPC qemu could have a persistence domain
> communicated via Open Firmware or some other mechanism.

Sure, this seems fine, though we may want to throw an "nvdimm" in the name
somewhere so it's clear what the option affects.

nvdimm-persistence={cpu,mem-ctrl} maybe?

Michael, does this work for you?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> >> > <ross.zwisler@linux.intel.com> wrote:
> >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> >> > >> > Add a machine command line option to allow the user to control the Platform
> >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> >> > >> > Structure was added in ACPI 6.2 Errata A.
> >> > >> >
> >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> >> > >>
> >> > >> I tried playing with it and encoding the capabilities is
> >> > >> quite awkward.
> >> > >>
> >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> >> > >>
> >> > >> How about:
> >> > >>
> >> > >> "cpu-flush-on-power-loss-cap"
> >> > >> "memory-flush-on-power-loss-cap"
> >> > >> "byte-addressable-mirroring-cap"
> >> > >
> >> > > Hmmm...I don't like that as much because:
> >> > >
> >> > > a) It's very verbose.  Looking at my current qemu command line few other
> >> > >    options require that many characters, and you'd commonly be defining more
> >> > >    than one of these for a given VM.
> >> > >
> >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> >> > >    because we'll have to have new options for each flag.  The current
> >> > >    implementation is more future-proof because you can specify any flags
> >> > >    value you want.
> >> > >
> >> > > However, if you feel strongly about this, I'll make the change.
> >> >
> >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> >> >
> >> > enum ndctl_persistence_domain {
> >> >         PERSISTENCE_NONE = 0,
> >> >         PERSISTENCE_MEM_CTRL = 10,
> >> >         PERSISTENCE_CPU_CACHE = 20,
> >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> >> > };
> >> >
> >> > ...and have the command line take a number where "10" and "20" are
> >> > supported today, but allows us to adapt to new persistence domains in
> >> > the future.
> >>
> >> I'm fine with that except can we have symbolic names instead of numbers
> >> on command line?
> >>
> >> --
> >> MST
> >
> > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > would:
> >
> > nvdimm-cap-cpu
> > nvdimm-cap-mem-ctrl
> > nvdimm-cap-mirroring
> 
> Wait, why is mirroring part of this?
> 
> I was thinking this option would be:
> 
>     --persistence-domain={cpu,mem-ctrl}
> 
> ...and try not to let ACPI specifics leak into the qemu command line
> interface. For example PowerPC qemu could have a persistence domain
> communicated via Open Firmware or some other mechanism.

Sure, this seems fine, though we may want to throw an "nvdimm" in the name
somewhere so it's clear what the option affects.

nvdimm-persistence={cpu,mem-ctrl} maybe?

Michael, does this work for you?

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > >> > <ross.zwisler@linux.intel.com> wrote:
> > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > >> > >> > Add a machine command line option to allow the user to control the Platform
> > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > >> > >> >
> > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > >> > >>
> > >> > >> I tried playing with it and encoding the capabilities is
> > >> > >> quite awkward.
> > >> > >>
> > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > >> > >>
> > >> > >> How about:
> > >> > >>
> > >> > >> "cpu-flush-on-power-loss-cap"
> > >> > >> "memory-flush-on-power-loss-cap"
> > >> > >> "byte-addressable-mirroring-cap"
> > >> > >
> > >> > > Hmmm...I don't like that as much because:
> > >> > >
> > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > >> > >    options require that many characters, and you'd commonly be defining more
> > >> > >    than one of these for a given VM.
> > >> > >
> > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > >> > >    because we'll have to have new options for each flag.  The current
> > >> > >    implementation is more future-proof because you can specify any flags
> > >> > >    value you want.
> > >> > >
> > >> > > However, if you feel strongly about this, I'll make the change.
> > >> >
> > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > >> >
> > >> > enum ndctl_persistence_domain {
> > >> >         PERSISTENCE_NONE = 0,
> > >> >         PERSISTENCE_MEM_CTRL = 10,
> > >> >         PERSISTENCE_CPU_CACHE = 20,
> > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > >> > };
> > >> >
> > >> > ...and have the command line take a number where "10" and "20" are
> > >> > supported today, but allows us to adapt to new persistence domains in
> > >> > the future.
> > >>
> > >> I'm fine with that except can we have symbolic names instead of numbers
> > >> on command line?
> > >>
> > >> --
> > >> MST
> > >
> > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > would:
> > >
> > > nvdimm-cap-cpu
> > > nvdimm-cap-mem-ctrl
> > > nvdimm-cap-mirroring
> > 
> > Wait, why is mirroring part of this?
> > 
> > I was thinking this option would be:
> > 
> >     --persistence-domain={cpu,mem-ctrl}
> > 
> > ...and try not to let ACPI specifics leak into the qemu command line
> > interface. For example PowerPC qemu could have a persistence domain
> > communicated via Open Firmware or some other mechanism.
> 
> Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> somewhere so it's clear what the option affects.
> 
> nvdimm-persistence={cpu,mem-ctrl} maybe?
> 
> Michael, does this work for you?

Hmm...also, the version of these patches that used numeric values did go
upstream in QEMU.  So, do we need to leave that interface intact, and just add
a new one that uses symbolic names?  Or did you still just want to replace the
numeric one since it hasn't appeared in a QEMU release yet?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > >> > <ross.zwisler@linux.intel.com> wrote:
> > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > >> > >> > Add a machine command line option to allow the user to control the Platform
> > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > >> > >> >
> > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > >> > >>
> > >> > >> I tried playing with it and encoding the capabilities is
> > >> > >> quite awkward.
> > >> > >>
> > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > >> > >>
> > >> > >> How about:
> > >> > >>
> > >> > >> "cpu-flush-on-power-loss-cap"
> > >> > >> "memory-flush-on-power-loss-cap"
> > >> > >> "byte-addressable-mirroring-cap"
> > >> > >
> > >> > > Hmmm...I don't like that as much because:
> > >> > >
> > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > >> > >    options require that many characters, and you'd commonly be defining more
> > >> > >    than one of these for a given VM.
> > >> > >
> > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > >> > >    because we'll have to have new options for each flag.  The current
> > >> > >    implementation is more future-proof because you can specify any flags
> > >> > >    value you want.
> > >> > >
> > >> > > However, if you feel strongly about this, I'll make the change.
> > >> >
> > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > >> >
> > >> > enum ndctl_persistence_domain {
> > >> >         PERSISTENCE_NONE = 0,
> > >> >         PERSISTENCE_MEM_CTRL = 10,
> > >> >         PERSISTENCE_CPU_CACHE = 20,
> > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > >> > };
> > >> >
> > >> > ...and have the command line take a number where "10" and "20" are
> > >> > supported today, but allows us to adapt to new persistence domains in
> > >> > the future.
> > >>
> > >> I'm fine with that except can we have symbolic names instead of numbers
> > >> on command line?
> > >>
> > >> --
> > >> MST
> > >
> > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > would:
> > >
> > > nvdimm-cap-cpu
> > > nvdimm-cap-mem-ctrl
> > > nvdimm-cap-mirroring
> > 
> > Wait, why is mirroring part of this?
> > 
> > I was thinking this option would be:
> > 
> >     --persistence-domain={cpu,mem-ctrl}
> > 
> > ...and try not to let ACPI specifics leak into the qemu command line
> > interface. For example PowerPC qemu could have a persistence domain
> > communicated via Open Firmware or some other mechanism.
> 
> Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> somewhere so it's clear what the option affects.
> 
> nvdimm-persistence={cpu,mem-ctrl} maybe?
> 
> Michael, does this work for you?

Hmm...also, the version of these patches that used numeric values did go
upstream in QEMU.  So, do we need to leave that interface intact, and just add
a new one that uses symbolic names?  Or did you still just want to replace the
numeric one since it hasn't appeared in a QEMU release yet?

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > <ross.zwisler@linux.intel.com> wrote:
> > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > >> > <ross.zwisler@linux.intel.com> wrote:
> > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > >> > >> >
> > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > >> > >>
> > > >> > >> I tried playing with it and encoding the capabilities is
> > > >> > >> quite awkward.
> > > >> > >>
> > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > >> > >>
> > > >> > >> How about:
> > > >> > >>
> > > >> > >> "cpu-flush-on-power-loss-cap"
> > > >> > >> "memory-flush-on-power-loss-cap"
> > > >> > >> "byte-addressable-mirroring-cap"
> > > >> > >
> > > >> > > Hmmm...I don't like that as much because:
> > > >> > >
> > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > >> > >    options require that many characters, and you'd commonly be defining more
> > > >> > >    than one of these for a given VM.
> > > >> > >
> > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > >> > >    because we'll have to have new options for each flag.  The current
> > > >> > >    implementation is more future-proof because you can specify any flags
> > > >> > >    value you want.
> > > >> > >
> > > >> > > However, if you feel strongly about this, I'll make the change.
> > > >> >
> > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > >> >
> > > >> > enum ndctl_persistence_domain {
> > > >> >         PERSISTENCE_NONE = 0,
> > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > >> > };
> > > >> >
> > > >> > ...and have the command line take a number where "10" and "20" are
> > > >> > supported today, but allows us to adapt to new persistence domains in
> > > >> > the future.
> > > >>
> > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > >> on command line?
> > > >>
> > > >> --
> > > >> MST
> > > >
> > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > would:
> > > >
> > > > nvdimm-cap-cpu
> > > > nvdimm-cap-mem-ctrl
> > > > nvdimm-cap-mirroring
> > > 
> > > Wait, why is mirroring part of this?
> > > 
> > > I was thinking this option would be:
> > > 
> > >     --persistence-domain={cpu,mem-ctrl}
> > > 
> > > ...and try not to let ACPI specifics leak into the qemu command line
> > > interface. For example PowerPC qemu could have a persistence domain
> > > communicated via Open Firmware or some other mechanism.
> > 
> > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > somewhere so it's clear what the option affects.
> > 
> > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > 
> > Michael, does this work for you?
> 
> Hmm...also, the version of these patches that used numeric values did go
> upstream in QEMU.  So, do we need to leave that interface intact, and just add
> a new one that uses symbolic names?  Or did you still just want to replace the
> numeric one since it hasn't appeared in a QEMU release yet?

I guess another alternative would be to just add symbolic name options to the
existing command line option, i.e. allow all of these:

nvdimm-cap=3		# CPU cache
nvdimm-cap=cpu		# CPU cache
nvdimm-cap=2		# memory controller
nvdimm-cap=mem-ctrl	# memory controller

And just have them as aliases.  This retains backwards compatibility with
what is already there, allows for other numeric values without QEMU updates if
other bits are defined (though we are still tightly tied to ACPI in this
case), and provides a symbolic name which is easier to use.

Thoughts?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Ross Zwisler]

On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > <ross.zwisler@linux.intel.com> wrote:
> > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > >> > <ross.zwisler@linux.intel.com> wrote:
> > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > >> > >> >
> > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > >> > >>
> > > >> > >> I tried playing with it and encoding the capabilities is
> > > >> > >> quite awkward.
> > > >> > >>
> > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > >> > >>
> > > >> > >> How about:
> > > >> > >>
> > > >> > >> "cpu-flush-on-power-loss-cap"
> > > >> > >> "memory-flush-on-power-loss-cap"
> > > >> > >> "byte-addressable-mirroring-cap"
> > > >> > >
> > > >> > > Hmmm...I don't like that as much because:
> > > >> > >
> > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > >> > >    options require that many characters, and you'd commonly be defining more
> > > >> > >    than one of these for a given VM.
> > > >> > >
> > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > >> > >    because we'll have to have new options for each flag.  The current
> > > >> > >    implementation is more future-proof because you can specify any flags
> > > >> > >    value you want.
> > > >> > >
> > > >> > > However, if you feel strongly about this, I'll make the change.
> > > >> >
> > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > >> >
> > > >> > enum ndctl_persistence_domain {
> > > >> >         PERSISTENCE_NONE = 0,
> > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > >> > };
> > > >> >
> > > >> > ...and have the command line take a number where "10" and "20" are
> > > >> > supported today, but allows us to adapt to new persistence domains in
> > > >> > the future.
> > > >>
> > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > >> on command line?
> > > >>
> > > >> --
> > > >> MST
> > > >
> > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > would:
> > > >
> > > > nvdimm-cap-cpu
> > > > nvdimm-cap-mem-ctrl
> > > > nvdimm-cap-mirroring
> > > 
> > > Wait, why is mirroring part of this?
> > > 
> > > I was thinking this option would be:
> > > 
> > >     --persistence-domain={cpu,mem-ctrl}
> > > 
> > > ...and try not to let ACPI specifics leak into the qemu command line
> > > interface. For example PowerPC qemu could have a persistence domain
> > > communicated via Open Firmware or some other mechanism.
> > 
> > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > somewhere so it's clear what the option affects.
> > 
> > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > 
> > Michael, does this work for you?
> 
> Hmm...also, the version of these patches that used numeric values did go
> upstream in QEMU.  So, do we need to leave that interface intact, and just add
> a new one that uses symbolic names?  Or did you still just want to replace the
> numeric one since it hasn't appeared in a QEMU release yet?

I guess another alternative would be to just add symbolic name options to the
existing command line option, i.e. allow all of these:

nvdimm-cap=3		# CPU cache
nvdimm-cap=cpu		# CPU cache
nvdimm-cap=2		# memory controller
nvdimm-cap=mem-ctrl	# memory controller

And just have them as aliases.  This retains backwards compatibility with
what is already there, allows for other numeric values without QEMU updates if
other bits are defined (though we are still tightly tied to ACPI in this
case), and provides a symbolic name which is easier to use.

Thoughts?

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 11:08:49AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> > On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > > <ross.zwisler@linux.intel.com> wrote:
> > > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > > >> > <ross.zwisler@linux.intel.com> wrote:
> > > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > > >> > >> >
> > > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > > >> > >>
> > > > >> > >> I tried playing with it and encoding the capabilities is
> > > > >> > >> quite awkward.
> > > > >> > >>
> > > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > > >> > >>
> > > > >> > >> How about:
> > > > >> > >>
> > > > >> > >> "cpu-flush-on-power-loss-cap"
> > > > >> > >> "memory-flush-on-power-loss-cap"
> > > > >> > >> "byte-addressable-mirroring-cap"
> > > > >> > >
> > > > >> > > Hmmm...I don't like that as much because:
> > > > >> > >
> > > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > > >> > >    options require that many characters, and you'd commonly be defining more
> > > > >> > >    than one of these for a given VM.
> > > > >> > >
> > > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > > >> > >    because we'll have to have new options for each flag.  The current
> > > > >> > >    implementation is more future-proof because you can specify any flags
> > > > >> > >    value you want.
> > > > >> > >
> > > > >> > > However, if you feel strongly about this, I'll make the change.
> > > > >> >
> > > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > > >> >
> > > > >> > enum ndctl_persistence_domain {
> > > > >> >         PERSISTENCE_NONE = 0,
> > > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > > >> > };
> > > > >> >
> > > > >> > ...and have the command line take a number where "10" and "20" are
> > > > >> > supported today, but allows us to adapt to new persistence domains in
> > > > >> > the future.
> > > > >>
> > > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > > >> on command line?
> > > > >>
> > > > >> --
> > > > >> MST
> > > > >
> > > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > > would:
> > > > >
> > > > > nvdimm-cap-cpu
> > > > > nvdimm-cap-mem-ctrl
> > > > > nvdimm-cap-mirroring
> > > > 
> > > > Wait, why is mirroring part of this?
> > > > 
> > > > I was thinking this option would be:
> > > > 
> > > >     --persistence-domain={cpu,mem-ctrl}
> > > > 
> > > > ...and try not to let ACPI specifics leak into the qemu command line
> > > > interface. For example PowerPC qemu could have a persistence domain
> > > > communicated via Open Firmware or some other mechanism.
> > > 
> > > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > > somewhere so it's clear what the option affects.
> > > 
> > > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > > 
> > > Michael, does this work for you?
> > 
> > Hmm...also, the version of these patches that used numeric values did go
> > upstream in QEMU.  So, do we need to leave that interface intact, and just add
> > a new one that uses symbolic names?  Or did you still just want to replace the
> > numeric one since it hasn't appeared in a QEMU release yet?
> 
> I guess another alternative would be to just add symbolic name options to the
> existing command line option, i.e. allow all of these:
> 
> nvdimm-cap=3		# CPU cache
> nvdimm-cap=cpu		# CPU cache
> nvdimm-cap=2		# memory controller
> nvdimm-cap=mem-ctrl	# memory controller
> 
> And just have them as aliases.  This retains backwards compatibility with
> what is already there, allows for other numeric values without QEMU updates if
> other bits are defined (though we are still tightly tied to ACPI in this
> case), and provides a symbolic name which is easier to use.
> 
> Thoughts?

I'm inclined to say let's just keep the symbolic names.
Less of a chance users configure something incorrectly,
it somehow kind of works and then we get stuck with working
around these bugs.

-- 
MST
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 11:08:49AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> > On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > > <ross.zwisler@linux.intel.com> wrote:
> > > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > > >> > <ross.zwisler@linux.intel.com> wrote:
> > > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > > >> > >> >
> > > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > > >> > >>
> > > > >> > >> I tried playing with it and encoding the capabilities is
> > > > >> > >> quite awkward.
> > > > >> > >>
> > > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > > >> > >>
> > > > >> > >> How about:
> > > > >> > >>
> > > > >> > >> "cpu-flush-on-power-loss-cap"
> > > > >> > >> "memory-flush-on-power-loss-cap"
> > > > >> > >> "byte-addressable-mirroring-cap"
> > > > >> > >
> > > > >> > > Hmmm...I don't like that as much because:
> > > > >> > >
> > > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > > >> > >    options require that many characters, and you'd commonly be defining more
> > > > >> > >    than one of these for a given VM.
> > > > >> > >
> > > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > > >> > >    because we'll have to have new options for each flag.  The current
> > > > >> > >    implementation is more future-proof because you can specify any flags
> > > > >> > >    value you want.
> > > > >> > >
> > > > >> > > However, if you feel strongly about this, I'll make the change.
> > > > >> >
> > > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > > >> >
> > > > >> > enum ndctl_persistence_domain {
> > > > >> >         PERSISTENCE_NONE = 0,
> > > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > > >> > };
> > > > >> >
> > > > >> > ...and have the command line take a number where "10" and "20" are
> > > > >> > supported today, but allows us to adapt to new persistence domains in
> > > > >> > the future.
> > > > >>
> > > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > > >> on command line?
> > > > >>
> > > > >> --
> > > > >> MST
> > > > >
> > > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > > would:
> > > > >
> > > > > nvdimm-cap-cpu
> > > > > nvdimm-cap-mem-ctrl
> > > > > nvdimm-cap-mirroring
> > > > 
> > > > Wait, why is mirroring part of this?
> > > > 
> > > > I was thinking this option would be:
> > > > 
> > > >     --persistence-domain={cpu,mem-ctrl}
> > > > 
> > > > ...and try not to let ACPI specifics leak into the qemu command line
> > > > interface. For example PowerPC qemu could have a persistence domain
> > > > communicated via Open Firmware or some other mechanism.
> > > 
> > > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > > somewhere so it's clear what the option affects.
> > > 
> > > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > > 
> > > Michael, does this work for you?
> > 
> > Hmm...also, the version of these patches that used numeric values did go
> > upstream in QEMU.  So, do we need to leave that interface intact, and just add
> > a new one that uses symbolic names?  Or did you still just want to replace the
> > numeric one since it hasn't appeared in a QEMU release yet?
> 
> I guess another alternative would be to just add symbolic name options to the
> existing command line option, i.e. allow all of these:
> 
> nvdimm-cap=3		# CPU cache
> nvdimm-cap=cpu		# CPU cache
> nvdimm-cap=2		# memory controller
> nvdimm-cap=mem-ctrl	# memory controller
> 
> And just have them as aliases.  This retains backwards compatibility with
> what is already there, allows for other numeric values without QEMU updates if
> other bits are defined (though we are still tightly tied to ACPI in this
> case), and provides a symbolic name which is easier to use.
> 
> Thoughts?

I'm inclined to say let's just keep the symbolic names.
Less of a chance users configure something incorrectly,
it somehow kind of works and then we get stuck with working
around these bugs.

-- 
MST

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Igor Mammedov]

On Wed, 6 Jun 2018 22:06:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jun 06, 2018 at 11:08:49AM -0600, Ross Zwisler wrote:
> > On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:  
> > > On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:  
> > > > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:  
> > > > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > > > <ross.zwisler@linux.intel.com> wrote:  
> > > > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:  
> > > > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:  
> > > > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > > > >> > <ross.zwisler@linux.intel.com> wrote:  
> > > > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:  
> > > > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:  
> > > > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > > > >> > >> >
> > > > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>  
> > > > > >> > >>
> > > > > >> > >> I tried playing with it and encoding the capabilities is
> > > > > >> > >> quite awkward.
> > > > > >> > >>
> > > > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > > > >> > >>
> > > > > >> > >> How about:
> > > > > >> > >>
> > > > > >> > >> "cpu-flush-on-power-loss-cap"
> > > > > >> > >> "memory-flush-on-power-loss-cap"
> > > > > >> > >> "byte-addressable-mirroring-cap"  
> > > > > >> > >
> > > > > >> > > Hmmm...I don't like that as much because:
> > > > > >> > >
> > > > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > > > >> > >    options require that many characters, and you'd commonly be defining more
> > > > > >> > >    than one of these for a given VM.
> > > > > >> > >
> > > > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > > > >> > >    because we'll have to have new options for each flag.  The current
> > > > > >> > >    implementation is more future-proof because you can specify any flags
> > > > > >> > >    value you want.
> > > > > >> > >
> > > > > >> > > However, if you feel strongly about this, I'll make the change.  
> > > > > >> >
> > > > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > > > >> >
> > > > > >> > enum ndctl_persistence_domain {
> > > > > >> >         PERSISTENCE_NONE = 0,
> > > > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > > > >> > };
> > > > > >> >
> > > > > >> > ...and have the command line take a number where "10" and "20" are
> > > > > >> > supported today, but allows us to adapt to new persistence domains in
> > > > > >> > the future.  
> > > > > >>
> > > > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > > > >> on command line?
> > > > > >>
> > > > > >> --
> > > > > >> MST  
> > > > > >
> > > > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > > > would:
> > > > > >
> > > > > > nvdimm-cap-cpu
> > > > > > nvdimm-cap-mem-ctrl
> > > > > > nvdimm-cap-mirroring  
> > > > > 
> > > > > Wait, why is mirroring part of this?
> > > > > 
> > > > > I was thinking this option would be:
> > > > > 
> > > > >     --persistence-domain={cpu,mem-ctrl}
> > > > > 
> > > > > ...and try not to let ACPI specifics leak into the qemu command line
> > > > > interface. For example PowerPC qemu could have a persistence domain
> > > > > communicated via Open Firmware or some other mechanism.  
> > > > 
> > > > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > > > somewhere so it's clear what the option affects.
> > > > 
> > > > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > > > 
> > > > Michael, does this work for you?  
> > > 
> > > Hmm...also, the version of these patches that used numeric values did go
> > > upstream in QEMU.  So, do we need to leave that interface intact, and just add
> > > a new one that uses symbolic names?  Or did you still just want to replace the
> > > numeric one since it hasn't appeared in a QEMU release yet?  
> > 
> > I guess another alternative would be to just add symbolic name options to the
> > existing command line option, i.e. allow all of these:
> > 
> > nvdimm-cap=3		# CPU cache
> > nvdimm-cap=cpu		# CPU cache
> > nvdimm-cap=2		# memory controller
> > nvdimm-cap=mem-ctrl	# memory controller
> > 
> > And just have them as aliases.  This retains backwards compatibility with
> > what is already there, allows for other numeric values without QEMU updates if
> > other bits are defined (though we are still tightly tied to ACPI in this
> > case), and provides a symbolic name which is easier to use.
> > 
> > Thoughts?  
> 
> I'm inclined to say let's just keep the symbolic names.
> Less of a chance users configure something incorrectly,
> it somehow kind of works and then we get stuck with working
> around these bugs.
We used numeric values before (OSPM status) in QMP which is ACPI spec defined,
so we probably should do so in this case as well, as far as there is clear
explanation for user where values come from. We can have symbolic aliases,
but it would be just extra burden to maintain (so I'd prefer not to have it).


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Igor Mammedov]

On Wed, 6 Jun 2018 22:06:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jun 06, 2018 at 11:08:49AM -0600, Ross Zwisler wrote:
> > On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:  
> > > On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:  
> > > > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:  
> > > > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > > > <ross.zwisler@linux.intel.com> wrote:  
> > > > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:  
> > > > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:  
> > > > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > > > >> > <ross.zwisler@linux.intel.com> wrote:  
> > > > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:  
> > > > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:  
> > > > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > > > >> > >> >
> > > > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>  
> > > > > >> > >>
> > > > > >> > >> I tried playing with it and encoding the capabilities is
> > > > > >> > >> quite awkward.
> > > > > >> > >>
> > > > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > > > >> > >>
> > > > > >> > >> How about:
> > > > > >> > >>
> > > > > >> > >> "cpu-flush-on-power-loss-cap"
> > > > > >> > >> "memory-flush-on-power-loss-cap"
> > > > > >> > >> "byte-addressable-mirroring-cap"  
> > > > > >> > >
> > > > > >> > > Hmmm...I don't like that as much because:
> > > > > >> > >
> > > > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > > > >> > >    options require that many characters, and you'd commonly be defining more
> > > > > >> > >    than one of these for a given VM.
> > > > > >> > >
> > > > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > > > >> > >    because we'll have to have new options for each flag.  The current
> > > > > >> > >    implementation is more future-proof because you can specify any flags
> > > > > >> > >    value you want.
> > > > > >> > >
> > > > > >> > > However, if you feel strongly about this, I'll make the change.  
> > > > > >> >
> > > > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > > > >> >
> > > > > >> > enum ndctl_persistence_domain {
> > > > > >> >         PERSISTENCE_NONE = 0,
> > > > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > > > >> > };
> > > > > >> >
> > > > > >> > ...and have the command line take a number where "10" and "20" are
> > > > > >> > supported today, but allows us to adapt to new persistence domains in
> > > > > >> > the future.  
> > > > > >>
> > > > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > > > >> on command line?
> > > > > >>
> > > > > >> --
> > > > > >> MST  
> > > > > >
> > > > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > > > would:
> > > > > >
> > > > > > nvdimm-cap-cpu
> > > > > > nvdimm-cap-mem-ctrl
> > > > > > nvdimm-cap-mirroring  
> > > > > 
> > > > > Wait, why is mirroring part of this?
> > > > > 
> > > > > I was thinking this option would be:
> > > > > 
> > > > >     --persistence-domain={cpu,mem-ctrl}
> > > > > 
> > > > > ...and try not to let ACPI specifics leak into the qemu command line
> > > > > interface. For example PowerPC qemu could have a persistence domain
> > > > > communicated via Open Firmware or some other mechanism.  
> > > > 
> > > > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > > > somewhere so it's clear what the option affects.
> > > > 
> > > > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > > > 
> > > > Michael, does this work for you?  
> > > 
> > > Hmm...also, the version of these patches that used numeric values did go
> > > upstream in QEMU.  So, do we need to leave that interface intact, and just add
> > > a new one that uses symbolic names?  Or did you still just want to replace the
> > > numeric one since it hasn't appeared in a QEMU release yet?  
> > 
> > I guess another alternative would be to just add symbolic name options to the
> > existing command line option, i.e. allow all of these:
> > 
> > nvdimm-cap=3		# CPU cache
> > nvdimm-cap=cpu		# CPU cache
> > nvdimm-cap=2		# memory controller
> > nvdimm-cap=mem-ctrl	# memory controller
> > 
> > And just have them as aliases.  This retains backwards compatibility with
> > what is already there, allows for other numeric values without QEMU updates if
> > other bits are defined (though we are still tightly tied to ACPI in this
> > case), and provides a symbolic name which is easier to use.
> > 
> > Thoughts?  
> 
> I'm inclined to say let's just keep the symbolic names.
> Less of a chance users configure something incorrectly,
> it somehow kind of works and then we get stuck with working
> around these bugs.
We used numeric values before (OSPM status) in QMP which is ACPI spec defined,
so we probably should do so in this case as well, as far as there is clear
explanation for user where values come from. We can have symbolic aliases,
but it would be just extra burden to maintain (so I'd prefer not to have it).

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > <ross.zwisler@linux.intel.com> wrote:
> > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > >> > <ross.zwisler@linux.intel.com> wrote:
> > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > >> > >> >
> > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > >> > >>
> > > >> > >> I tried playing with it and encoding the capabilities is
> > > >> > >> quite awkward.
> > > >> > >>
> > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > >> > >>
> > > >> > >> How about:
> > > >> > >>
> > > >> > >> "cpu-flush-on-power-loss-cap"
> > > >> > >> "memory-flush-on-power-loss-cap"
> > > >> > >> "byte-addressable-mirroring-cap"
> > > >> > >
> > > >> > > Hmmm...I don't like that as much because:
> > > >> > >
> > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > >> > >    options require that many characters, and you'd commonly be defining more
> > > >> > >    than one of these for a given VM.
> > > >> > >
> > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > >> > >    because we'll have to have new options for each flag.  The current
> > > >> > >    implementation is more future-proof because you can specify any flags
> > > >> > >    value you want.
> > > >> > >
> > > >> > > However, if you feel strongly about this, I'll make the change.
> > > >> >
> > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > >> >
> > > >> > enum ndctl_persistence_domain {
> > > >> >         PERSISTENCE_NONE = 0,
> > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > >> > };
> > > >> >
> > > >> > ...and have the command line take a number where "10" and "20" are
> > > >> > supported today, but allows us to adapt to new persistence domains in
> > > >> > the future.
> > > >>
> > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > >> on command line?
> > > >>
> > > >> --
> > > >> MST
> > > >
> > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > would:
> > > >
> > > > nvdimm-cap-cpu
> > > > nvdimm-cap-mem-ctrl
> > > > nvdimm-cap-mirroring
> > > 
> > > Wait, why is mirroring part of this?
> > > 
> > > I was thinking this option would be:
> > > 
> > >     --persistence-domain={cpu,mem-ctrl}
> > > 
> > > ...and try not to let ACPI specifics leak into the qemu command line
> > > interface. For example PowerPC qemu could have a persistence domain
> > > communicated via Open Firmware or some other mechanism.
> > 
> > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > somewhere so it's clear what the option affects.
> > 
> > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > 
> > Michael, does this work for you?
> 
> Hmm...also, the version of these patches that used numeric values did go
> upstream in QEMU.  So, do we need to leave that interface intact, and just add
> a new one that uses symbolic names?  Or did you still just want to replace the
> numeric one since it hasn't appeared in a QEMU release yet?

The later. No release => no stable APIs.

-- 
MST
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 11:00:33AM -0600, Ross Zwisler wrote:
> On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> > On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > > <ross.zwisler@linux.intel.com> wrote:
> > > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > > >> > <ross.zwisler@linux.intel.com> wrote:
> > > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > > >> > >> > Add a machine command line option to allow the user to control the Platform
> > > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > > >> > >> >
> > > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > >> > >>
> > > >> > >> I tried playing with it and encoding the capabilities is
> > > >> > >> quite awkward.
> > > >> > >>
> > > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > > >> > >>
> > > >> > >> How about:
> > > >> > >>
> > > >> > >> "cpu-flush-on-power-loss-cap"
> > > >> > >> "memory-flush-on-power-loss-cap"
> > > >> > >> "byte-addressable-mirroring-cap"
> > > >> > >
> > > >> > > Hmmm...I don't like that as much because:
> > > >> > >
> > > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > > >> > >    options require that many characters, and you'd commonly be defining more
> > > >> > >    than one of these for a given VM.
> > > >> > >
> > > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > > >> > >    because we'll have to have new options for each flag.  The current
> > > >> > >    implementation is more future-proof because you can specify any flags
> > > >> > >    value you want.
> > > >> > >
> > > >> > > However, if you feel strongly about this, I'll make the change.
> > > >> >
> > > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > > >> >
> > > >> > enum ndctl_persistence_domain {
> > > >> >         PERSISTENCE_NONE = 0,
> > > >> >         PERSISTENCE_MEM_CTRL = 10,
> > > >> >         PERSISTENCE_CPU_CACHE = 20,
> > > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > > >> > };
> > > >> >
> > > >> > ...and have the command line take a number where "10" and "20" are
> > > >> > supported today, but allows us to adapt to new persistence domains in
> > > >> > the future.
> > > >>
> > > >> I'm fine with that except can we have symbolic names instead of numbers
> > > >> on command line?
> > > >>
> > > >> --
> > > >> MST
> > > >
> > > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > > would:
> > > >
> > > > nvdimm-cap-cpu
> > > > nvdimm-cap-mem-ctrl
> > > > nvdimm-cap-mirroring
> > > 
> > > Wait, why is mirroring part of this?
> > > 
> > > I was thinking this option would be:
> > > 
> > >     --persistence-domain={cpu,mem-ctrl}
> > > 
> > > ...and try not to let ACPI specifics leak into the qemu command line
> > > interface. For example PowerPC qemu could have a persistence domain
> > > communicated via Open Firmware or some other mechanism.
> > 
> > Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> > somewhere so it's clear what the option affects.
> > 
> > nvdimm-persistence={cpu,mem-ctrl} maybe?
> > 
> > Michael, does this work for you?
> 
> Hmm...also, the version of these patches that used numeric values did go
> upstream in QEMU.  So, do we need to leave that interface intact, and just add
> a new one that uses symbolic names?  Or did you still just want to replace the
> numeric one since it hasn't appeared in a QEMU release yet?

The later. No release => no stable APIs.

-- 
MST

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > >> > <ross.zwisler@linux.intel.com> wrote:
> > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > >> > >> > Add a machine command line option to allow the user to control the Platform
> > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > >> > >> >
> > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > >> > >>
> > >> > >> I tried playing with it and encoding the capabilities is
> > >> > >> quite awkward.
> > >> > >>
> > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > >> > >>
> > >> > >> How about:
> > >> > >>
> > >> > >> "cpu-flush-on-power-loss-cap"
> > >> > >> "memory-flush-on-power-loss-cap"
> > >> > >> "byte-addressable-mirroring-cap"
> > >> > >
> > >> > > Hmmm...I don't like that as much because:
> > >> > >
> > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > >> > >    options require that many characters, and you'd commonly be defining more
> > >> > >    than one of these for a given VM.
> > >> > >
> > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > >> > >    because we'll have to have new options for each flag.  The current
> > >> > >    implementation is more future-proof because you can specify any flags
> > >> > >    value you want.
> > >> > >
> > >> > > However, if you feel strongly about this, I'll make the change.
> > >> >
> > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > >> >
> > >> > enum ndctl_persistence_domain {
> > >> >         PERSISTENCE_NONE = 0,
> > >> >         PERSISTENCE_MEM_CTRL = 10,
> > >> >         PERSISTENCE_CPU_CACHE = 20,
> > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > >> > };
> > >> >
> > >> > ...and have the command line take a number where "10" and "20" are
> > >> > supported today, but allows us to adapt to new persistence domains in
> > >> > the future.
> > >>
> > >> I'm fine with that except can we have symbolic names instead of numbers
> > >> on command line?
> > >>
> > >> --
> > >> MST
> > >
> > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > would:
> > >
> > > nvdimm-cap-cpu
> > > nvdimm-cap-mem-ctrl
> > > nvdimm-cap-mirroring
> > 
> > Wait, why is mirroring part of this?
> > 
> > I was thinking this option would be:
> > 
> >     --persistence-domain={cpu,mem-ctrl}
> > 
> > ...and try not to let ACPI specifics leak into the qemu command line
> > interface. For example PowerPC qemu could have a persistence domain
> > communicated via Open Firmware or some other mechanism.
> 
> Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> somewhere so it's clear what the option affects.
> 
> nvdimm-persistence={cpu,mem-ctrl} maybe?
> 
> Michael, does this work for you?

Sounds good to me.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 10:50:51AM -0600, Ross Zwisler wrote:
> On Tue, Jun 05, 2018 at 03:21:30PM -0700, Dan Williams wrote:
> > On Tue, Jun 5, 2018 at 3:07 PM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> > > On Tue, Jun 05, 2018 at 09:37:25PM +0300, Michael S. Tsirkin wrote:
> > >> On Tue, Jun 05, 2018 at 11:15:00AM -0700, Dan Williams wrote:
> > >> > On Tue, Jun 5, 2018 at 9:42 AM, Ross Zwisler
> > >> > <ross.zwisler@linux.intel.com> wrote:
> > >> > > On Tue, Jun 05, 2018 at 06:25:27PM +0300, Michael S. Tsirkin wrote:
> > >> > >> On Mon, May 21, 2018 at 10:32:02AM -0600, Ross Zwisler wrote:
> > >> > >> > Add a machine command line option to allow the user to control the Platform
> > >> > >> > Capabilities Structure in the virtualized NFIT.  This Platform Capabilities
> > >> > >> > Structure was added in ACPI 6.2 Errata A.
> > >> > >> >
> > >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > >> > >>
> > >> > >> I tried playing with it and encoding the capabilities is
> > >> > >> quite awkward.
> > >> > >>
> > >> > >> Can we add bits for specific capabilities instead of nvdimm-cap?
> > >> > >>
> > >> > >> How about:
> > >> > >>
> > >> > >> "cpu-flush-on-power-loss-cap"
> > >> > >> "memory-flush-on-power-loss-cap"
> > >> > >> "byte-addressable-mirroring-cap"
> > >> > >
> > >> > > Hmmm...I don't like that as much because:
> > >> > >
> > >> > > a) It's very verbose.  Looking at my current qemu command line few other
> > >> > >    options require that many characters, and you'd commonly be defining more
> > >> > >    than one of these for a given VM.
> > >> > >
> > >> > > b) It means that the QEMU will need to be updated if/when new flags are added,
> > >> > >    because we'll have to have new options for each flag.  The current
> > >> > >    implementation is more future-proof because you can specify any flags
> > >> > >    value you want.
> > >> > >
> > >> > > However, if you feel strongly about this, I'll make the change.
> > >> >
> > >> > Straw-man: Could we do something similar with what we are doing in ndctl?
> > >> >
> > >> > enum ndctl_persistence_domain {
> > >> >         PERSISTENCE_NONE = 0,
> > >> >         PERSISTENCE_MEM_CTRL = 10,
> > >> >         PERSISTENCE_CPU_CACHE = 20,
> > >> >         PERSISTENCE_UNKNOWN = INT_MAX,
> > >> > };
> > >> >
> > >> > ...and have the command line take a number where "10" and "20" are
> > >> > supported today, but allows us to adapt to new persistence domains in
> > >> > the future.
> > >>
> > >> I'm fine with that except can we have symbolic names instead of numbers
> > >> on command line?
> > >>
> > >> --
> > >> MST
> > >
> > > Okay, we can move to the symbolic names.  Do you want them to be that long, or
> > > would:
> > >
> > > nvdimm-cap-cpu
> > > nvdimm-cap-mem-ctrl
> > > nvdimm-cap-mirroring
> > 
> > Wait, why is mirroring part of this?
> > 
> > I was thinking this option would be:
> > 
> >     --persistence-domain={cpu,mem-ctrl}
> > 
> > ...and try not to let ACPI specifics leak into the qemu command line
> > interface. For example PowerPC qemu could have a persistence domain
> > communicated via Open Firmware or some other mechanism.
> 
> Sure, this seems fine, though we may want to throw an "nvdimm" in the name
> somewhere so it's clear what the option affects.
> 
> nvdimm-persistence={cpu,mem-ctrl} maybe?
> 
> Michael, does this work for you?

Sounds good to me.

RE: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Elliott, Robert (Persistent Memory)]

> > Okay, we can move to the symbolic names.  Do you want them to be that
> long, or
> > would:
> >
> > nvdimm-cap-cpu
> > nvdimm-cap-mem-ctrl
> > nvdimm-cap-mirroring
> 
> Wait, why is mirroring part of this?

This data structure is intended to report any kind of platform capability, not 
just platform persistence capabilities.

We could add a short symbolic name to the definitions in ACPI that matches
the ones selected for this command line option, if that'll help people
find the right names to use.

I recommend mc rather than mem-ctrl to keep dashes as special.


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Elliott, Robert (Persistent Memory)]

> > Okay, we can move to the symbolic names.  Do you want them to be that
> long, or
> > would:
> >
> > nvdimm-cap-cpu
> > nvdimm-cap-mem-ctrl
> > nvdimm-cap-mirroring
> 
> Wait, why is mirroring part of this?

This data structure is intended to report any kind of platform capability, not 
just platform persistence capabilities.

We could add a short symbolic name to the definitions in ACPI that matches
the ones selected for this command line option, if that'll help people
find the right names to use.

I recommend mc rather than mem-ctrl to keep dashes as special.

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Dan Williams]

On Wed, Jun 6, 2018 at 4:20 PM, Elliott, Robert (Persistent Memory)
<elliott@hpe.com> wrote:
>
>> > Okay, we can move to the symbolic names.  Do you want them to be that
>> long, or
>> > would:
>> >
>> > nvdimm-cap-cpu
>> > nvdimm-cap-mem-ctrl
>> > nvdimm-cap-mirroring
>>
>> Wait, why is mirroring part of this?
>
> This data structure is intended to report any kind of platform capability, not
> just platform persistence capabilities.

Yes, but here's nothing actionable that a qemu guest OS can do with
that mirroring information, so there's no need at this time to add cli
cruft and code to support it.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Dan Williams]

On Wed, Jun 6, 2018 at 4:20 PM, Elliott, Robert (Persistent Memory)
<elliott@hpe.com> wrote:
>
>> > Okay, we can move to the symbolic names.  Do you want them to be that
>> long, or
>> > would:
>> >
>> > nvdimm-cap-cpu
>> > nvdimm-cap-mem-ctrl
>> > nvdimm-cap-mirroring
>>
>> Wait, why is mirroring part of this?
>
> This data structure is intended to report any kind of platform capability, not
> just platform persistence capabilities.

Yes, but here's nothing actionable that a qemu guest OS can do with
that mirroring information, so there's no need at this time to add cli
cruft and code to support it.

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 04:40:31PM -0700, Dan Williams wrote:
> On Wed, Jun 6, 2018 at 4:20 PM, Elliott, Robert (Persistent Memory)
> <elliott@hpe.com> wrote:
> >
> >> > Okay, we can move to the symbolic names.  Do you want them to be that
> >> long, or
> >> > would:
> >> >
> >> > nvdimm-cap-cpu
> >> > nvdimm-cap-mem-ctrl
> >> > nvdimm-cap-mirroring
> >>
> >> Wait, why is mirroring part of this?
> >
> > This data structure is intended to report any kind of platform capability, not
> > just platform persistence capabilities.
> 
> Yes, but here's nothing actionable that a qemu guest OS can do with
> that mirroring information, so there's no need at this time to add cli
> cruft and code to support it.

I agree.

-- 
MST
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 04:40:31PM -0700, Dan Williams wrote:
> On Wed, Jun 6, 2018 at 4:20 PM, Elliott, Robert (Persistent Memory)
> <elliott@hpe.com> wrote:
> >
> >> > Okay, we can move to the symbolic names.  Do you want them to be that
> >> long, or
> >> > would:
> >> >
> >> > nvdimm-cap-cpu
> >> > nvdimm-cap-mem-ctrl
> >> > nvdimm-cap-mirroring
> >>
> >> Wait, why is mirroring part of this?
> >
> > This data structure is intended to report any kind of platform capability, not
> > just platform persistence capabilities.
> 
> Yes, but here's nothing actionable that a qemu guest OS can do with
> that mirroring information, so there's no need at this time to add cli
> cruft and code to support it.

I agree.

-- 
MST

Re: [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 11:20:58PM +0000, Elliott, Robert (Persistent Memory) wrote:
> 
> > > Okay, we can move to the symbolic names.  Do you want them to be that
> > long, or
> > > would:
> > >
> > > nvdimm-cap-cpu
> > > nvdimm-cap-mem-ctrl
> > > nvdimm-cap-mirroring
> > 
> > Wait, why is mirroring part of this?
> 
> This data structure is intended to report any kind of platform capability, not 
> just platform persistence capabilities.
> 
> We could add a short symbolic name to the definitions in ACPI that matches
> the ones selected for this command line option, if that'll help people
> find the right names to use.
> 
> I recommend mc rather than mem-ctrl to keep dashes as special.
> 

I'm not sure it's a good idea:
dashes aren't special in qemu, and mc's way too brief.

-- 
MST
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 3/4] nvdimm, acpi: support NFIT platform capabilities

[Michael S. Tsirkin]

On Wed, Jun 06, 2018 at 11:20:58PM +0000, Elliott, Robert (Persistent Memory) wrote:
> 
> > > Okay, we can move to the symbolic names.  Do you want them to be that
> > long, or
> > > would:
> > >
> > > nvdimm-cap-cpu
> > > nvdimm-cap-mem-ctrl
> > > nvdimm-cap-mirroring
> > 
> > Wait, why is mirroring part of this?
> 
> This data structure is intended to report any kind of platform capability, not 
> just platform persistence capabilities.
> 
> We could add a short symbolic name to the definitions in ACPI that matches
> the ones selected for this command line option, if that'll help people
> find the right names to use.
> 
> I recommend mc rather than mem-ctrl to keep dashes as special.
> 

I'm not sure it's a good idea:
dashes aren't special in qemu, and mc's way too brief.

-- 
MST

[qemu PATCH v4 4/4] ACPI testing: test NFIT platform capabilities

[Ross Zwisler]

Add testing for the newly added NFIT Platform Capabilities Structure.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
 tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
 tests/bios-tables-test.c              |   2 +-
 3 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acpi-test-data/pc/NFIT.dimmpxm b/tests/acpi-test-data/pc/NFIT.dimmpxm
index 2bfc6c51f31c25a052803c494c933d4948fc0106..598d331b751cd3cb2137d431c1f34bb8957a0d31 100644
GIT binary patch
delta 35
lcmaFB_<@nj&&@OB0|NsCqsm0CYXa;H0t}2m9y1Vw005dH1-}3Q

delta 18
Zcmeys_<)hi&&@OB0RsaAqyI#%YXCU~1+M@A

diff --git a/tests/acpi-test-data/q35/NFIT.dimmpxm b/tests/acpi-test-data/q35/NFIT.dimmpxm
index 2bfc6c51f31c25a052803c494c933d4948fc0106..598d331b751cd3cb2137d431c1f34bb8957a0d31 100644
GIT binary patch
delta 35
lcmaFB_<@nj&&@OB0|NsCqsm0CYXa;H0t}2m9y1Vw005dH1-}3Q

delta 18
Zcmeys_<)hi&&@OB0RsaAqyI#%YXCU~1+M@A

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index bf3e193ae9..256d463cb8 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -830,7 +830,7 @@ static void test_acpi_tcg_dimm_pxm(const char *machine)
     memset(&data, 0, sizeof(data));
     data.machine = machine;
     data.variant = ".dimmpxm";
-    test_acpi_one(" -machine nvdimm=on"
+    test_acpi_one(" -machine nvdimm=on,nvdimm-cap=3"
                   " -smp 4,sockets=4"
                   " -m 128M,slots=3,maxmem=1G"
                   " -numa node,mem=32M,nodeid=0"
-- 
2.14.3

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[Qemu-devel] [qemu PATCH v4 4/4] ACPI testing: test NFIT platform capabilities

[Ross Zwisler]

Add testing for the newly added NFIT Platform Capabilities Structure.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
 tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
 tests/bios-tables-test.c              |   2 +-
 3 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/acpi-test-data/pc/NFIT.dimmpxm b/tests/acpi-test-data/pc/NFIT.dimmpxm
index 2bfc6c51f31c25a052803c494c933d4948fc0106..598d331b751cd3cb2137d431c1f34bb8957a0d31 100644
GIT binary patch
delta 35
lcmaFB_<@nj&&@OB0|NsCqsm0CYXa;H0t}2m9y1Vw005dH1-}3Q

delta 18
Zcmeys_<)hi&&@OB0RsaAqyI#%YXCU~1+M@A

diff --git a/tests/acpi-test-data/q35/NFIT.dimmpxm b/tests/acpi-test-data/q35/NFIT.dimmpxm
index 2bfc6c51f31c25a052803c494c933d4948fc0106..598d331b751cd3cb2137d431c1f34bb8957a0d31 100644
GIT binary patch
delta 35
lcmaFB_<@nj&&@OB0|NsCqsm0CYXa;H0t}2m9y1Vw005dH1-}3Q

delta 18
Zcmeys_<)hi&&@OB0RsaAqyI#%YXCU~1+M@A

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index bf3e193ae9..256d463cb8 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -830,7 +830,7 @@ static void test_acpi_tcg_dimm_pxm(const char *machine)
     memset(&data, 0, sizeof(data));
     data.machine = machine;
     data.variant = ".dimmpxm";
-    test_acpi_one(" -machine nvdimm=on"
+    test_acpi_one(" -machine nvdimm=on,nvdimm-cap=3"
                   " -smp 4,sockets=4"
                   " -m 128M,slots=3,maxmem=1G"
                   " -numa node,mem=32M,nodeid=0"
-- 
2.14.3

Re: [qemu PATCH v4 0/4] support NFIT platform capabilities

[Michael S. Tsirkin]

On Mon, May 21, 2018 at 10:31:59AM -0600, Ross Zwisler wrote:
> Changes since v3:
>  * Updated the text in docs/nvdimm.txt to make it clear that the value
>    being passed in on the command line in an integer made up of various
>    bit fields. (Rob Elliott)
>  
>  * Updated the "Highest Valid Capability" byte to be dynamic based on
>    the highest valid bit in the user's input. (Rob Elliott)

Igor could you review pls? I think your comments have been addressed.

> ---
> 
> The first 2 patches in this series clean up some things I noticed while
> coding.
> 
> Patch 3 adds support for the new Platform Capabilities Structure, which
> was added to the NFIT in ACPI 6.2 Errata A.  We add a machine command
> line option "nvdimm-cap":
> 
>     -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> 
> which allows the user to pass in a value for this structure.  When such
> a value is passed in we will generate the new NFIT subtable.
> 
> Patch 4 adds code to the "make check" self test infrastructure so that
> we generate the new Platform Capabilities Structure, and adds it to the
> expected NFIT output so that we test for it.
> 
> Ross Zwisler (4):
>   nvdimm: fix typo in label-size definition
>   tests/.gitignore: add entry for generated file
>   nvdimm, acpi: support NFIT platform capabilities
>   ACPI testing: test NFIT platform capabilities
> 
>  docs/nvdimm.txt                       |  27 ++++++++++++++++++++
>  hw/acpi/nvdimm.c                      |  45 +++++++++++++++++++++++++++++++---
>  hw/i386/pc.c                          |  31 +++++++++++++++++++++++
>  hw/mem/nvdimm.c                       |   2 +-
>  include/hw/i386/pc.h                  |   1 +
>  include/hw/mem/nvdimm.h               |   7 +++++-
>  tests/.gitignore                      |   1 +
>  tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
>  tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
>  tests/bios-tables-test.c              |   2 +-
>  10 files changed, 109 insertions(+), 7 deletions(-)
> 
> -- 
> 2.14.3
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 0/4] support NFIT platform capabilities

[Michael S. Tsirkin]

On Mon, May 21, 2018 at 10:31:59AM -0600, Ross Zwisler wrote:
> Changes since v3:
>  * Updated the text in docs/nvdimm.txt to make it clear that the value
>    being passed in on the command line in an integer made up of various
>    bit fields. (Rob Elliott)
>  
>  * Updated the "Highest Valid Capability" byte to be dynamic based on
>    the highest valid bit in the user's input. (Rob Elliott)

Igor could you review pls? I think your comments have been addressed.

> ---
> 
> The first 2 patches in this series clean up some things I noticed while
> coding.
> 
> Patch 3 adds support for the new Platform Capabilities Structure, which
> was added to the NFIT in ACPI 6.2 Errata A.  We add a machine command
> line option "nvdimm-cap":
> 
>     -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> 
> which allows the user to pass in a value for this structure.  When such
> a value is passed in we will generate the new NFIT subtable.
> 
> Patch 4 adds code to the "make check" self test infrastructure so that
> we generate the new Platform Capabilities Structure, and adds it to the
> expected NFIT output so that we test for it.
> 
> Ross Zwisler (4):
>   nvdimm: fix typo in label-size definition
>   tests/.gitignore: add entry for generated file
>   nvdimm, acpi: support NFIT platform capabilities
>   ACPI testing: test NFIT platform capabilities
> 
>  docs/nvdimm.txt                       |  27 ++++++++++++++++++++
>  hw/acpi/nvdimm.c                      |  45 +++++++++++++++++++++++++++++++---
>  hw/i386/pc.c                          |  31 +++++++++++++++++++++++
>  hw/mem/nvdimm.c                       |   2 +-
>  include/hw/i386/pc.h                  |   1 +
>  include/hw/mem/nvdimm.h               |   7 +++++-
>  tests/.gitignore                      |   1 +
>  tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
>  tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
>  tests/bios-tables-test.c              |   2 +-
>  10 files changed, 109 insertions(+), 7 deletions(-)
> 
> -- 
> 2.14.3

Re: [qemu PATCH v4 0/4] support NFIT platform capabilities

[Ross Zwisler]

On Fri, May 25, 2018 at 08:51:22PM +0300, Michael S. Tsirkin wrote:
> On Mon, May 21, 2018 at 10:31:59AM -0600, Ross Zwisler wrote:
> > Changes since v3:
> >  * Updated the text in docs/nvdimm.txt to make it clear that the value
> >    being passed in on the command line in an integer made up of various
> >    bit fields. (Rob Elliott)
> >  
> >  * Updated the "Highest Valid Capability" byte to be dynamic based on
> >    the highest valid bit in the user's input. (Rob Elliott)
> 
> Igor could you review pls? I think your comments have been addressed.

Ping?  Igor, can you pick this up?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 0/4] support NFIT platform capabilities

[Ross Zwisler]

On Fri, May 25, 2018 at 08:51:22PM +0300, Michael S. Tsirkin wrote:
> On Mon, May 21, 2018 at 10:31:59AM -0600, Ross Zwisler wrote:
> > Changes since v3:
> >  * Updated the text in docs/nvdimm.txt to make it clear that the value
> >    being passed in on the command line in an integer made up of various
> >    bit fields. (Rob Elliott)
> >  
> >  * Updated the "Highest Valid Capability" byte to be dynamic based on
> >    the highest valid bit in the user's input. (Rob Elliott)
> 
> Igor could you review pls? I think your comments have been addressed.

Ping?  Igor, can you pick this up?

Re: [qemu PATCH v4 0/4] support NFIT platform capabilities

[Ross Zwisler]

Ping on this series.  Rob, I think I've addressed all your feedback.  Can you
please verify?

Thanks,
- Ross

On Mon, May 21, 2018 at 10:31:59AM -0600, Ross Zwisler wrote:
> Changes since v3:
>  * Updated the text in docs/nvdimm.txt to make it clear that the value
>    being passed in on the command line in an integer made up of various
>    bit fields. (Rob Elliott)
>  
>  * Updated the "Highest Valid Capability" byte to be dynamic based on
>    the highest valid bit in the user's input. (Rob Elliott)
> 
> ---
> 
> The first 2 patches in this series clean up some things I noticed while
> coding.
> 
> Patch 3 adds support for the new Platform Capabilities Structure, which
> was added to the NFIT in ACPI 6.2 Errata A.  We add a machine command
> line option "nvdimm-cap":
> 
>     -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> 
> which allows the user to pass in a value for this structure.  When such
> a value is passed in we will generate the new NFIT subtable.
> 
> Patch 4 adds code to the "make check" self test infrastructure so that
> we generate the new Platform Capabilities Structure, and adds it to the
> expected NFIT output so that we test for it.
> 
> Ross Zwisler (4):
>   nvdimm: fix typo in label-size definition
>   tests/.gitignore: add entry for generated file
>   nvdimm, acpi: support NFIT platform capabilities
>   ACPI testing: test NFIT platform capabilities
> 
>  docs/nvdimm.txt                       |  27 ++++++++++++++++++++
>  hw/acpi/nvdimm.c                      |  45 +++++++++++++++++++++++++++++++---
>  hw/i386/pc.c                          |  31 +++++++++++++++++++++++
>  hw/mem/nvdimm.c                       |   2 +-
>  include/hw/i386/pc.h                  |   1 +
>  include/hw/mem/nvdimm.h               |   7 +++++-
>  tests/.gitignore                      |   1 +
>  tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
>  tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
>  tests/bios-tables-test.c              |   2 +-
>  10 files changed, 109 insertions(+), 7 deletions(-)
> 
> -- 
> 2.14.3
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 0/4] support NFIT platform capabilities

[Ross Zwisler]

Ping on this series.  Rob, I think I've addressed all your feedback.  Can you
please verify?

Thanks,
- Ross

On Mon, May 21, 2018 at 10:31:59AM -0600, Ross Zwisler wrote:
> Changes since v3:
>  * Updated the text in docs/nvdimm.txt to make it clear that the value
>    being passed in on the command line in an integer made up of various
>    bit fields. (Rob Elliott)
>  
>  * Updated the "Highest Valid Capability" byte to be dynamic based on
>    the highest valid bit in the user's input. (Rob Elliott)
> 
> ---
> 
> The first 2 patches in this series clean up some things I noticed while
> coding.
> 
> Patch 3 adds support for the new Platform Capabilities Structure, which
> was added to the NFIT in ACPI 6.2 Errata A.  We add a machine command
> line option "nvdimm-cap":
> 
>     -machine pc,accel=kvm,nvdimm,nvdimm-cap=2
> 
> which allows the user to pass in a value for this structure.  When such
> a value is passed in we will generate the new NFIT subtable.
> 
> Patch 4 adds code to the "make check" self test infrastructure so that
> we generate the new Platform Capabilities Structure, and adds it to the
> expected NFIT output so that we test for it.
> 
> Ross Zwisler (4):
>   nvdimm: fix typo in label-size definition
>   tests/.gitignore: add entry for generated file
>   nvdimm, acpi: support NFIT platform capabilities
>   ACPI testing: test NFIT platform capabilities
> 
>  docs/nvdimm.txt                       |  27 ++++++++++++++++++++
>  hw/acpi/nvdimm.c                      |  45 +++++++++++++++++++++++++++++++---
>  hw/i386/pc.c                          |  31 +++++++++++++++++++++++
>  hw/mem/nvdimm.c                       |   2 +-
>  include/hw/i386/pc.h                  |   1 +
>  include/hw/mem/nvdimm.h               |   7 +++++-
>  tests/.gitignore                      |   1 +
>  tests/acpi-test-data/pc/NFIT.dimmpxm  | Bin 224 -> 240 bytes
>  tests/acpi-test-data/q35/NFIT.dimmpxm | Bin 224 -> 240 bytes
>  tests/bios-tables-test.c              |   2 +-
>  10 files changed, 109 insertions(+), 7 deletions(-)
> 
> -- 
> 2.14.3
> 

RE: [qemu PATCH v4 0/4] support NFIT platform capabilities

[Elliott, Robert (Persistent Memory)]

> Ping on this series.  Rob, I think I've addressed all your feedback.
> Can you please verify?

I haven't tested it, but it reads OK.  I'm OK with just extending
the valid count for bits set to one for now; we can add a new
argument later if a need arises for extending it to express new bits
set to zero.


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [Qemu-devel] [qemu PATCH v4 0/4] support NFIT platform capabilities

[Elliott, Robert (Persistent Memory)]

> Ping on this series.  Rob, I think I've addressed all your feedback.
> Can you please verify?

I haven't tested it, but it reads OK.  I'm OK with just extending
the valid count for bits set to one for now; we can add a new
argument later if a need arises for extending it to express new bits
set to zero.