[Qemu-devel] [PATCH v4 0/9] Add support for VM Generation ID

[Qemu-devel] [PATCH v4 0/9] Add support for VM Generation ID

[ben]

From: Ben Warren <ben@skyportsystems.com>

This patch set adds support for passing a GUID to Windows guests.  It is a
re-implementation of previous patch sets written by Igor Mammedov et al, but
this time passing the GUID data as a fw_cfg blob.

This patch set has dependencies on new guest functionality, in particular the
support for a new linker-loader command and the ability to write back data
to QEMU over a DMA link.  Work is in flight in both SeaBIOS and OVMF to support this.
The SeaBIOS project is awaiting committing of this patch set before pulling in the
matching patches:
https://www.seabios.org/pipermail/seabios/2017-January/011097.html

v4->v3:
    - Rebased to top of tree.
    - Re-added document patch that was accidentally dropped from the last revision.
    - Added VMState functionality so that VGIA is restored properly.
    - Added Unit tests
v2->v3:
    - Added second writeable fw_cfg for storing the VM Generaiton ID
      address.  This uses a new linker-loader command for instructing the
      guest to write back the allocated address.  A patch for SeaBIOS has been
      submitted (https://www.seabios.org/pipermail/seabios/2017-January/011079.html)
      and the resulting binary will need to be pulled into QEMU once accepted.
    - Setting VM Generation ID by command line or qmp/hmp now accepts an "auto"
      value, whereby QEMU generates a random GUID.
    - Incorporated review comments from v2 mainly around code styling and AML syntax
    - Changed to use the E05 ACPI event instead of E00
v1->v2:
    - Removed "changed" boolean parameter as it is unneeded
    - Added ACPI Notify logic
    - Style changes to pass checkpatch.pl
    - Added support for dynamic sysbus to pc_piix boards

Ben Warren (7):
  ACPI: Add a function for building named qword entries
  linker-loader: Add new 'allocate and return address' cmd
  docs: VM Generation ID device description
  ACPI: Add Virtual Machine Generation ID support
  PC: Support dynamic sysbus on pc_i440fx
  tests: Move reusable ACPI macros into a new header file
  tests: Add unit tests for the VM Generation ID feature

Igor Mammedov (2):
  qmp/hmp: add query-vm-generation-id and 'info vm-generation-id'
    commands
  qmp/hmp: add set-vm-generation-id commands

 default-configs/i386-softmmu.mak     |   1 +
 default-configs/x86_64-softmmu.mak   |   1 +
 docs/specs/vmgenid.txt               |  40 ++++++
 hmp-commands-info.hx                 |  13 ++
 hmp-commands.hx                      |  13 ++
 hmp.c                                |  21 +++
 hmp.h                                |   2 +
 hw/acpi/Makefile.objs                |   1 +
 hw/acpi/aml-build.c                  |  28 ++++
 hw/acpi/bios-linker-loader.c         |  71 +++++++++-
 hw/acpi/vmgenid.c                    | 244 +++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c                 |   9 ++
 hw/i386/pc_piix.c                    |   1 +
 include/hw/acpi/acpi_dev_interface.h |   1 +
 include/hw/acpi/aml-build.h          |   4 +
 include/hw/acpi/bios-linker-loader.h |   7 +
 include/hw/acpi/vmgenid.h            |  32 +++++
 qapi-schema.json                     |  31 +++++
 stubs/Makefile.objs                  |   1 +
 stubs/vmgenid.c                      |  14 ++
 tests/Makefile.include               |   2 +
 tests/acpi-utils.h                   |  75 +++++++++++
 tests/bios-tables-test.c             |  72 +----------
 tests/vmgenid-test.c                 | 184 ++++++++++++++++++++++++++
 24 files changed, 794 insertions(+), 74 deletions(-)
 create mode 100644 docs/specs/vmgenid.txt
 create mode 100644 hw/acpi/vmgenid.c
 create mode 100644 include/hw/acpi/vmgenid.h
 create mode 100644 stubs/vmgenid.c
 create mode 100644 tests/acpi-utils.h
 create mode 100644 tests/vmgenid-test.c

-- 
2.7.4

[Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[ben]

From: Ben Warren <ben@skyportsystems.com>

This is initially used to patch a 64-bit address into the VM Generation ID SSDT

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  4 ++++
 2 files changed, 32 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b2a1e40..dc4edc2 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char *name_format, ...)
     return offset;
 }
 
+/*
+ * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a qword,
+ * and return the offset to 0x00000000 for runtime patching.
+ *
+ * Warning: runtime patching is best avoided. Only use this as
+ * a replacement for DataTableRegion (for guests that don't
+ * support it).
+ */
+int
+build_append_named_qword(GArray *array, const char *name_format, ...)
+{
+    int offset;
+    va_list ap;
+
+    build_append_byte(array, 0x08); /* NameOp */
+    va_start(ap, name_format);
+    build_append_namestringv(array, name_format, ap);
+    va_end(ap);
+
+    build_append_byte(array, 0x0E); /* QWordPrefix */
+
+    offset = array->len;
+    build_append_int_noprefix(array, 0x00000000, 8);
+    assert(array->len == offset + 8);
+
+    return offset;
+}
+
 static GPtrArray *alloc_list;
 
 static Aml *aml_alloc(void)
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 559326c..dbf63cf 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -385,6 +385,10 @@ int
 build_append_named_dword(GArray *array, const char *name_format, ...)
 GCC_FMT_ATTR(2, 3);
 
+int
+build_append_named_qword(GArray *array, const char *name_format, ...)
+GCC_FMT_ATTR(2, 3);
+
 void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
                        uint64_t len, int node, MemoryAffinityFlags flags);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

Hi Ben,

sorry about being late to reviewing this series. I hope I can now spend
more time on it.

- Please do not try to address my comments immediately. It's very
possible (even likely) that Igor, MST and myself could have different
opinions on things, so first please await agreement between your reviewers.

- I think you should have CC'd Igor and Michael directly. I'm adding
them to this reply; hopefully that will be enough for them to monitor
this series.

- I'll likely be unable to review everything with 100% coverage; so
addressing (or sufficiently refuting) my comments might not guarantee
that the next version will be merged at once.

With all that said:

On 01/25/17 02:43, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> This is initially used to patch a 64-bit address into the VM Generation ID SSDT

(1) I think this commit message line is overlong; I think we wrap at 74
chars or so. Not critical, but worth mentioning.

> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  4 ++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b2a1e40..dc4edc2 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char *name_format, ...)
>      return offset;
>  }
>  
> +/*
> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a qword,
> + * and return the offset to 0x00000000 for runtime patching.
> + *
> + * Warning: runtime patching is best avoided. Only use this as
> + * a replacement for DataTableRegion (for guests that don't
> + * support it).
> + */

(2) Since we're adding a qword (8-byte integer), the hexadecimal
constant should have 16 nibbles: 0x0000000000000000. (After copying the
comment from build_append_named_dword(), it should be actualized.)

(3) Normally the functions in this file that create AML operators carry
a note about the ACPI spec version and exact location that defines the
operator. I see that commit f20354910893 ("acpi: add
build_append_named_dword, returning an offset in buffer", 2016-03-01)
missed that too.

Unless this tradition has been willfully abandoned, I suggest that you
add the right reference here, and also (in retrospect) to
build_append_named_dword().

> +int
> +build_append_named_qword(GArray *array, const char *name_format, ...)
> +{
> +    int offset;
> +    va_list ap;
> +
> +    build_append_byte(array, 0x08); /* NameOp */
> +    va_start(ap, name_format);
> +    build_append_namestringv(array, name_format, ap);
> +    va_end(ap);
> +
> +    build_append_byte(array, 0x0E); /* QWordPrefix */
> +
> +    offset = array->len;
> +    build_append_int_noprefix(array, 0x00000000, 8);

(4) I guess the constant should be updated here too, for consistency
with the comment.

The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically,
because an error there would break the functionality immediately, and
you'd notice.)

Thanks!
Laszlo

> +    assert(array->len == offset + 8);
> +
> +    return offset;
> +}
> +
>  static GPtrArray *alloc_list;
>  
>  static Aml *aml_alloc(void)
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 559326c..dbf63cf 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -385,6 +385,10 @@ int
>  build_append_named_dword(GArray *array, const char *name_format, ...)
>  GCC_FMT_ATTR(2, 3);
>  
> +int
> +build_append_named_qword(GArray *array, const char *name_format, ...)
> +GCC_FMT_ATTR(2, 3);
> +
>  void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>                         uint64_t len, int node, MemoryAffinityFlags flags);
>  
> 

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Ben Warren]

[-- Attachment #1: Type: text/plain, Size: 4358 bytes --]

Hi Laszlo,

> On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> Hi Ben,
> 
> sorry about being late to reviewing this series. I hope I can now spend
> more time on it.
> 
> - Please do not try to address my comments immediately. It's very
> possible (even likely) that Igor, MST and myself could have different
> opinions on things, so first please await agreement between your reviewers.
> 
Thanks for the very detailed review.  I’ll give it a couple of days and then start work on the suggested changes.

> - I think you should have CC'd Igor and Michael directly. I'm adding
> them to this reply; hopefully that will be enough for them to monitor
> this series.
> 
> - I'll likely be unable to review everything with 100% coverage; so
> addressing (or sufficiently refuting) my comments might not guarantee
> that the next version will be merged at once.
> 
> With all that said:
> 
> On 01/25/17 02:43, ben@skyportsystems.com <mailto:ben@skyportsystems.com> wrote:
>> From: Ben Warren <ben@skyportsystems.com <mailto:ben@skyportsystems.com>>
>> 
>> This is initially used to patch a 64-bit address into the VM Generation ID SSDT
> 
> (1) I think this commit message line is overlong; I think we wrap at 74
> chars or so. Not critical, but worth mentioning.
> 
>> 
>> Signed-off-by: Ben Warren <ben@skyportsystems.com>
>> ---
>> hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
>> include/hw/acpi/aml-build.h |  4 ++++
>> 2 files changed, 32 insertions(+)
>> 
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index b2a1e40..dc4edc2 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char *name_format, ...)
>>     return offset;
>> }
>> 
>> +/*
>> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a qword,
>> + * and return the offset to 0x00000000 for runtime patching.
>> + *
>> + * Warning: runtime patching is best avoided. Only use this as
>> + * a replacement for DataTableRegion (for guests that don't
>> + * support it).
>> + */
> 
> (2) Since we're adding a qword (8-byte integer), the hexadecimal
> constant should have 16 nibbles: 0x0000000000000000. (After copying the
> comment from build_append_named_dword(), it should be actualized.)
> 
> (3) Normally the functions in this file that create AML operators carry
> a note about the ACPI spec version and exact location that defines the
> operator. I see that commit f20354910893 ("acpi: add
> build_append_named_dword, returning an offset in buffer", 2016-03-01)
> missed that too.
> 
> Unless this tradition has been willfully abandoned, I suggest that you
> add the right reference here, and also (in retrospect) to
> build_append_named_dword().
> 
>> +int
>> +build_append_named_qword(GArray *array, const char *name_format, ...)
>> +{
>> +    int offset;
>> +    va_list ap;
>> +
>> +    build_append_byte(array, 0x08); /* NameOp */
>> +    va_start(ap, name_format);
>> +    build_append_namestringv(array, name_format, ap);
>> +    va_end(ap);
>> +
>> +    build_append_byte(array, 0x0E); /* QWordPrefix */
>> +
>> +    offset = array->len;
>> +    build_append_int_noprefix(array, 0x00000000, 8);
> 
> (4) I guess the constant should be updated here too, for consistency
> with the comment.
> 
> The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically,
> because an error there would break the functionality immediately, and
> you'd notice.)
> 
> Thanks!
> Laszlo
> 
>> +    assert(array->len == offset + 8);
>> +
>> +    return offset;
>> +}
>> +
>> static GPtrArray *alloc_list;
>> 
>> static Aml *aml_alloc(void)
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 559326c..dbf63cf 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -385,6 +385,10 @@ int
>> build_append_named_dword(GArray *array, const char *name_format, ...)
>> GCC_FMT_ATTR(2, 3);
>> 
>> +int
>> +build_append_named_qword(GArray *array, const char *name_format, ...)
>> +GCC_FMT_ATTR(2, 3);
>> +
>> void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>                        uint64_t len, int node, MemoryAffinityFlags flags);

—Ben


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
> Hi Laszlo,
> 
> 
>     On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>     Hi Ben,
> 
>     sorry about being late to reviewing this series. I hope I can now spend
>     more time on it.
> 
>     - Please do not try to address my comments immediately. It's very
>     possible (even likely) that Igor, MST and myself could have different
>     opinions on things, so first please await agreement between your reviewers.
> 
> 
> Thanks for the very detailed review.  I’ll give it a couple of days and then
> start work on the suggested changes.
> 
> 
>     - I think you should have CC'd Igor and Michael directly. I'm adding
>     them to this reply; hopefully that will be enough for them to monitor
>     this series.
> 
>     - I'll likely be unable to review everything with 100% coverage; so
>     addressing (or sufficiently refuting) my comments might not guarantee
>     that the next version will be merged at once.
> 
>     With all that said:
> 
>     On 01/25/17 02:43, ben@skyportsystems.com wrote:
> 
>         From: Ben Warren <ben@skyportsystems.com>
> 
>         This is initially used to patch a 64-bit address into the VM Generation
>         ID SSDT
> 
> 
>     (1) I think this commit message line is overlong; I think we wrap at 74
>     chars or so. Not critical, but worth mentioning.
> 
> 
> 
>         Signed-off-by: Ben Warren <ben@skyportsystems.com>
>         ---
>         hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
>         include/hw/acpi/aml-build.h |  4 ++++
>         2 files changed, 32 insertions(+)
> 
>         diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>         index b2a1e40..dc4edc2 100644
>         --- a/hw/acpi/aml-build.c
>         +++ b/hw/acpi/aml-build.c
>         @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char
>         *name_format, ...)
>             return offset;
>         }
> 
>         +/*
>         + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a
>         qword,
>         + * and return the offset to 0x00000000 for runtime patching.
>         + *
>         + * Warning: runtime patching is best avoided. Only use this as
>         + * a replacement for DataTableRegion (for guests that don't
>         + * support it).
>         + */

only one comment: QWords first appeared in ACPI 2.0 and
XP does not like them. Not strictly a blocker as people can
avoid using the feature, but nice to have.
Will either UEFI or seabios allocate
memory outside 4G range? If not you do not need a qword.




> 
>     (2) Since we're adding a qword (8-byte integer), the hexadecimal
>     constant should have 16 nibbles: 0x0000000000000000. (After copying the
>     comment from build_append_named_dword(), it should be actualized.)
> 
>     (3) Normally the functions in this file that create AML operators carry
>     a note about the ACPI spec version and exact location that defines the
>     operator. I see that commit f20354910893 ("acpi: add
>     build_append_named_dword, returning an offset in buffer", 2016-03-01)
>     missed that too.
> 
>     Unless this tradition has been willfully abandoned, I suggest that you
>     add the right reference here, and also (in retrospect) to
>     build_append_named_dword().
> 
> 
>         +int
>         +build_append_named_qword(GArray *array, const char *name_format, ...)
>         +{
>         +    int offset;
>         +    va_list ap;
>         +
>         +    build_append_byte(array, 0x08); /* NameOp */
>         +    va_start(ap, name_format);
>         +    build_append_namestringv(array, name_format, ap);
>         +    va_end(ap);
>         +
>         +    build_append_byte(array, 0x0E); /* QWordPrefix */
>         +
>         +    offset = array->len;
>         +    build_append_int_noprefix(array, 0x00000000, 8);
> 
> 
>     (4) I guess the constant should be updated here too, for consistency
>     with the comment.
> 
>     The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically,
>     because an error there would break the functionality immediately, and
>     you'd notice.)
> 
>     Thanks!
>     Laszlo
> 
> 
>         +    assert(array->len == offset + 8);
>         +
>         +    return offset;
>         +}
>         +
>         static GPtrArray *alloc_list;
> 
>         static Aml *aml_alloc(void)
>         diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>         index 559326c..dbf63cf 100644
>         --- a/include/hw/acpi/aml-build.h
>         +++ b/include/hw/acpi/aml-build.h
>         @@ -385,6 +385,10 @@ int
>         build_append_named_dword(GArray *array, const char *name_format, ...)
>         GCC_FMT_ATTR(2, 3);
> 
>         +int
>         +build_append_named_qword(GArray *array, const char *name_format, ...)
>         +GCC_FMT_ATTR(2, 3);
>         +
>         void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>                                uint64_t len, int node, MemoryAffinityFlags
>         flags);
> 
> 
> —Ben
> 

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/25/17 19:35, Michael S. Tsirkin wrote:
> On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
>> Hi Laszlo,
>>
>>
>>     On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>     Hi Ben,
>>
>>     sorry about being late to reviewing this series. I hope I can now spend
>>     more time on it.
>>
>>     - Please do not try to address my comments immediately. It's very
>>     possible (even likely) that Igor, MST and myself could have different
>>     opinions on things, so first please await agreement between your reviewers.
>>
>>
>> Thanks for the very detailed review.  I’ll give it a couple of days and then
>> start work on the suggested changes.
>>
>>
>>     - I think you should have CC'd Igor and Michael directly. I'm adding
>>     them to this reply; hopefully that will be enough for them to monitor
>>     this series.
>>
>>     - I'll likely be unable to review everything with 100% coverage; so
>>     addressing (or sufficiently refuting) my comments might not guarantee
>>     that the next version will be merged at once.
>>
>>     With all that said:
>>
>>     On 01/25/17 02:43, ben@skyportsystems.com wrote:
>>
>>         From: Ben Warren <ben@skyportsystems.com>
>>
>>         This is initially used to patch a 64-bit address into the VM Generation
>>         ID SSDT
>>
>>
>>     (1) I think this commit message line is overlong; I think we wrap at 74
>>     chars or so. Not critical, but worth mentioning.
>>
>>
>>
>>         Signed-off-by: Ben Warren <ben@skyportsystems.com>
>>         ---
>>         hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
>>         include/hw/acpi/aml-build.h |  4 ++++
>>         2 files changed, 32 insertions(+)
>>
>>         diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>         index b2a1e40..dc4edc2 100644
>>         --- a/hw/acpi/aml-build.c
>>         +++ b/hw/acpi/aml-build.c
>>         @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char
>>         *name_format, ...)
>>             return offset;
>>         }
>>
>>         +/*
>>         + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a
>>         qword,
>>         + * and return the offset to 0x00000000 for runtime patching.
>>         + *
>>         + * Warning: runtime patching is best avoided. Only use this as
>>         + * a replacement for DataTableRegion (for guests that don't
>>         + * support it).
>>         + */
> 
> only one comment: QWords first appeared in ACPI 2.0 and
> XP does not like them. Not strictly a blocker as people can
> avoid using the feature, but nice to have.

Does XP have a driver for VMGENID?

If not, then I'd prefer to stick with the qword VGIA.

> Will either UEFI or seabios allocate
> memory outside 4G range? If not you do not need a qword.

Good point (assuming XP has a driver for VMGENID).

OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the
upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is
concerned, using a dword for the VGIA named object should be fine.
Accordingly, a 4-byte wide ADD_POINTER command should be used for
patching VGIA.

Considering the fw_cfg file that receives the address, and
COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those
stayed 8-byte wide, regardless of XP's support for VMGENID.


Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long
as "Hyper-V integration services" are installed:

https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx

    The virtual machine must be running a guest operating system that
    has support for the virtual machine generation identifier.
    Currently, these are the following.
    The following operating systems have native support for the virtual
    machine generation identifier.
      [...]

    The following operating can be used as the guest operating system
    if the Hyper-V integration services from Windows 8 or Windows
    Server 2012 are installed.

      [...]
      * Windows XP with Service Pack 3 (SP3)

Additionally, under
<https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>:

    Supported Windows client guest operating systems

    Windows XP with       [...] Install the integration  [...]
    Service Pack 3 (SP3)        services after you set
                                up the operating system
                                in the virtual machine.

This seems to be consistent with the VMGENID spec requirement that the
ADDR method return a package of two 32-bit integers, not a single 64-bit
one.

So, I agree with using a dword for VGIA (and a matching 4-byte wide
ADD_POINTER command).

But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
In the future we might introduce more allocation hints (for the "zone"
field) that would enable the firmware to allocate from the full 64-bit
address space.

NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses
16-bit and 32-bit modes.

Thanks!
Laszlo

> 
> 
> 
> 
>>
>>     (2) Since we're adding a qword (8-byte integer), the hexadecimal
>>     constant should have 16 nibbles: 0x0000000000000000. (After copying the
>>     comment from build_append_named_dword(), it should be actualized.)
>>
>>     (3) Normally the functions in this file that create AML operators carry
>>     a note about the ACPI spec version and exact location that defines the
>>     operator. I see that commit f20354910893 ("acpi: add
>>     build_append_named_dword, returning an offset in buffer", 2016-03-01)
>>     missed that too.
>>
>>     Unless this tradition has been willfully abandoned, I suggest that you
>>     add the right reference here, and also (in retrospect) to
>>     build_append_named_dword().
>>
>>
>>         +int
>>         +build_append_named_qword(GArray *array, const char *name_format, ...)
>>         +{
>>         +    int offset;
>>         +    va_list ap;
>>         +
>>         +    build_append_byte(array, 0x08); /* NameOp */
>>         +    va_start(ap, name_format);
>>         +    build_append_namestringv(array, name_format, ap);
>>         +    va_end(ap);
>>         +
>>         +    build_append_byte(array, 0x0E); /* QWordPrefix */
>>         +
>>         +    offset = array->len;
>>         +    build_append_int_noprefix(array, 0x00000000, 8);
>>
>>
>>     (4) I guess the constant should be updated here too, for consistency
>>     with the comment.
>>
>>     The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically,
>>     because an error there would break the functionality immediately, and
>>     you'd notice.)
>>
>>     Thanks!
>>     Laszlo
>>
>>
>>         +    assert(array->len == offset + 8);
>>         +
>>         +    return offset;
>>         +}
>>         +
>>         static GPtrArray *alloc_list;
>>
>>         static Aml *aml_alloc(void)
>>         diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>         index 559326c..dbf63cf 100644
>>         --- a/include/hw/acpi/aml-build.h
>>         +++ b/include/hw/acpi/aml-build.h
>>         @@ -385,6 +385,10 @@ int
>>         build_append_named_dword(GArray *array, const char *name_format, ...)
>>         GCC_FMT_ATTR(2, 3);
>>
>>         +int
>>         +build_append_named_qword(GArray *array, const char *name_format, ...)
>>         +GCC_FMT_ATTR(2, 3);
>>         +
>>         void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>                                uint64_t len, int node, MemoryAffinityFlags
>>         flags);
>>
>>
>> —Ben
>>
> 
> 

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Ben Warren]

[-- Attachment #1: Type: text/plain, Size: 9627 bytes --]


> On Jan 25, 2017, at 4:48 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 01/25/17 19:35, Michael S. Tsirkin wrote:
>> On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
>>> Hi Laszlo,
>>> 
>>> 
>>>    On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 
>>>    Hi Ben,
>>> 
>>>    sorry about being late to reviewing this series. I hope I can now spend
>>>    more time on it.
>>> 
>>>    - Please do not try to address my comments immediately. It's very
>>>    possible (even likely) that Igor, MST and myself could have different
>>>    opinions on things, so first please await agreement between your reviewers.
>>> 
>>> 
>>> Thanks for the very detailed review.  I’ll give it a couple of days and then
>>> start work on the suggested changes.
>>> 
>>> 
>>>    - I think you should have CC'd Igor and Michael directly. I'm adding
>>>    them to this reply; hopefully that will be enough for them to monitor
>>>    this series.
>>> 
>>>    - I'll likely be unable to review everything with 100% coverage; so
>>>    addressing (or sufficiently refuting) my comments might not guarantee
>>>    that the next version will be merged at once.
>>> 
>>>    With all that said:
>>> 
>>>    On 01/25/17 02:43, ben@skyportsystems.com wrote:
>>> 
>>>        From: Ben Warren <ben@skyportsystems.com>
>>> 
>>>        This is initially used to patch a 64-bit address into the VM Generation
>>>        ID SSDT
>>> 
>>> 
>>>    (1) I think this commit message line is overlong; I think we wrap at 74
>>>    chars or so. Not critical, but worth mentioning.
>>> 
>>> 
>>> 
>>>        Signed-off-by: Ben Warren <ben@skyportsystems.com>
>>>        ---
>>>        hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
>>>        include/hw/acpi/aml-build.h |  4 ++++
>>>        2 files changed, 32 insertions(+)
>>> 
>>>        diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>        index b2a1e40..dc4edc2 100644
>>>        --- a/hw/acpi/aml-build.c
>>>        +++ b/hw/acpi/aml-build.c
>>>        @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char
>>>        *name_format, ...)
>>>            return offset;
>>>        }
>>> 
>>>        +/*
>>>        + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a
>>>        qword,
>>>        + * and return the offset to 0x00000000 for runtime patching.
>>>        + *
>>>        + * Warning: runtime patching is best avoided. Only use this as
>>>        + * a replacement for DataTableRegion (for guests that don't
>>>        + * support it).
>>>        + */
>> 
>> only one comment: QWords first appeared in ACPI 2.0 and
>> XP does not like them. Not strictly a blocker as people can
>> avoid using the feature, but nice to have.
> 
> Does XP have a driver for VMGENID?
> 
> If not, then I'd prefer to stick with the qword VGIA.
> 
>> Will either UEFI or seabios allocate
>> memory outside 4G range? If not you do not need a qword.
> 
> Good point (assuming XP has a driver for VMGENID).
> 
> OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the
> upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is
> concerned, using a dword for the VGIA named object should be fine.
> Accordingly, a 4-byte wide ADD_POINTER command should be used for
> patching VGIA.
> 
> Considering the fw_cfg file that receives the address, and
> COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those
> stayed 8-byte wide, regardless of XP's support for VMGENID.
> 
> 
> Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long
> as "Hyper-V integration services" are installed:
> 
> https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx <https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx>
> 
>    The virtual machine must be running a guest operating system that
>    has support for the virtual machine generation identifier.
>    Currently, these are the following.
>    The following operating systems have native support for the virtual
>    machine generation identifier.
>      [...]
> 
>    The following operating can be used as the guest operating system
>    if the Hyper-V integration services from Windows 8 or Windows
>    Server 2012 are installed.
> 
>      [...]
>      * Windows XP with Service Pack 3 (SP3)
> 
> Additionally, under
> <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>>:
> 
>    Supported Windows client guest operating systems
> 
>    Windows XP with       [...] Install the integration  [...]
>    Service Pack 3 (SP3)        services after you set
>                                up the operating system
>                                in the virtual machine.
> 
> This seems to be consistent with the VMGENID spec requirement that the
> ADDR method return a package of two 32-bit integers, not a single 64-bit
> one.
> 
> So, I agree with using a dword for VGIA (and a matching 4-byte wide
> ADD_POINTER command).
> 
> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
> In the future we might introduce more allocation hints (for the "zone"
> field) that would enable the firmware to allocate from the full 64-bit
> address space.
> 
> NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses
> 16-bit and 32-bit modes.
> 
Right, it appears that the allocated address will always fit in 32 bits.  As mentioned, XP should be OK since the ADDR method returns a package of two 32-bit numbers.

I propose to still include this patch but touch up the comments as requested by Laszlo.  This way it will be in the toolbox for future users and has been tested.  I will also change VGIA to dword and hard code the AML to return ADDR[1] = 0.  

FYI: here’s an iasl dump from Windows 2012 Hyper-V in case you haven’t seen it.  Note that Microsoft uses E00 and violates the HID name length spec:

Scope (\_SB)
   {
       Device (GENC)
       {
           Name (_CID, "VM_Gen_Counter")  // _CID: Compatible ID
           Name (_HID, "Hyper_V_Gen_Counter_V1")  // _HID: Hardware ID
           Name (_UID, 0x00)  // _UID: Unique ID
           Name (_DDN, "VM_Gen_Counter")  // _DDN: DOS Device Name
           Method (ADDR, 0, NotSerialized)
           {
               Name (LPKG, Package (0x02)
               {
                   0x00, 
                   0x00
               })
               LPKG [0x00] = GCAL /* \GCAL */
               LPKG [0x01] = GCAH /* \GCAH */
               Return (LPKG) /* \_SB_.GENC.ADDR.LPKG */
           }
       }
   }

   Scope (\_GPE)
   {
       Method (_E00, 0, NotSerialized)  // _Exx: Edge-Triggered GPE
       {
           Notify (\_SB.GENC, 0x80) // Status Change
       }
   }

> Thanks!
> Laszlo
> 
>> 
>> 
>> 
>> 
>>> 
>>>    (2) Since we're adding a qword (8-byte integer), the hexadecimal
>>>    constant should have 16 nibbles: 0x0000000000000000. (After copying the
>>>    comment from build_append_named_dword(), it should be actualized.)
>>> 
>>>    (3) Normally the functions in this file that create AML operators carry
>>>    a note about the ACPI spec version and exact location that defines the
>>>    operator. I see that commit f20354910893 ("acpi: add
>>>    build_append_named_dword, returning an offset in buffer", 2016-03-01)
>>>    missed that too.
>>> 
>>>    Unless this tradition has been willfully abandoned, I suggest that you
>>>    add the right reference here, and also (in retrospect) to
>>>    build_append_named_dword().
>>> 
>>> 
>>>        +int
>>>        +build_append_named_qword(GArray *array, const char *name_format, ...)
>>>        +{
>>>        +    int offset;
>>>        +    va_list ap;
>>>        +
>>>        +    build_append_byte(array, 0x08); /* NameOp */
>>>        +    va_start(ap, name_format);
>>>        +    build_append_namestringv(array, name_format, ap);
>>>        +    va_end(ap);
>>>        +
>>>        +    build_append_byte(array, 0x0E); /* QWordPrefix */
>>>        +
>>>        +    offset = array->len;
>>>        +    build_append_int_noprefix(array, 0x00000000, 8);
>>> 
>>> 
>>>    (4) I guess the constant should be updated here too, for consistency
>>>    with the comment.
>>> 
>>>    The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically,
>>>    because an error there would break the functionality immediately, and
>>>    you'd notice.)
>>> 
>>>    Thanks!
>>>    Laszlo
>>> 
>>> 
>>>        +    assert(array->len == offset + 8);
>>>        +
>>>        +    return offset;
>>>        +}
>>>        +
>>>        static GPtrArray *alloc_list;
>>> 
>>>        static Aml *aml_alloc(void)
>>>        diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>>>        index 559326c..dbf63cf 100644
>>>        --- a/include/hw/acpi/aml-build.h
>>>        +++ b/include/hw/acpi/aml-build.h
>>>        @@ -385,6 +385,10 @@ int
>>>        build_append_named_dword(GArray *array, const char *name_format, ...)
>>>        GCC_FMT_ATTR(2, 3);
>>> 
>>>        +int
>>>        +build_append_named_qword(GArray *array, const char *name_format, ...)
>>>        +GCC_FMT_ATTR(2, 3);
>>>        +
>>>        void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>>                               uint64_t len, int node, MemoryAffinityFlags
>>>        flags);
>>> 
>>> 
>>> —Ben


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/26/17 06:35, Ben Warren wrote:
> 
>> On Jan 25, 2017, at 4:48 PM, Laszlo Ersek <lersek@redhat.com
>> <mailto:lersek@redhat.com>> wrote:
>>
>> On 01/25/17 19:35, Michael S. Tsirkin wrote:
>>> On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
>>>> Hi Laszlo,
>>>>
>>>>
>>>>    On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com
>>>> <mailto:lersek@redhat.com>> wrote:
>>>>
>>>>    Hi Ben,
>>>>
>>>>    sorry about being late to reviewing this series. I hope I can now
>>>> spend
>>>>    more time on it.
>>>>
>>>>    - Please do not try to address my comments immediately. It's very
>>>>    possible (even likely) that Igor, MST and myself could have different
>>>>    opinions on things, so first please await agreement between your
>>>> reviewers.
>>>>
>>>>
>>>> Thanks for the very detailed review.  I’ll give it a couple of days
>>>> and then
>>>> start work on the suggested changes.
>>>>
>>>>
>>>>    - I think you should have CC'd Igor and Michael directly. I'm adding
>>>>    them to this reply; hopefully that will be enough for them to monitor
>>>>    this series.
>>>>
>>>>    - I'll likely be unable to review everything with 100% coverage; so
>>>>    addressing (or sufficiently refuting) my comments might not guarantee
>>>>    that the next version will be merged at once.
>>>>
>>>>    With all that said:
>>>>
>>>>    On 01/25/17 02:43, ben@skyportsystems.com
>>>> <mailto:ben@skyportsystems.com> wrote:
>>>>
>>>>        From: Ben Warren <ben@skyportsystems.com
>>>> <mailto:ben@skyportsystems.com>>
>>>>
>>>>        This is initially used to patch a 64-bit address into the VM
>>>> Generation
>>>>        ID SSDT
>>>>
>>>>
>>>>    (1) I think this commit message line is overlong; I think we wrap
>>>> at 74
>>>>    chars or so. Not critical, but worth mentioning.
>>>>
>>>>
>>>>
>>>>        Signed-off-by: Ben Warren <ben@skyportsystems.com
>>>> <mailto:ben@skyportsystems.com>>
>>>>        ---
>>>>        hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
>>>>        include/hw/acpi/aml-build.h |  4 ++++
>>>>        2 files changed, 32 insertions(+)
>>>>
>>>>        diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>>        index b2a1e40..dc4edc2 100644
>>>>        --- a/hw/acpi/aml-build.c
>>>>        +++ b/hw/acpi/aml-build.c
>>>>        @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array,
>>>> const char
>>>>        *name_format, ...)
>>>>            return offset;
>>>>        }
>>>>
>>>>        +/*
>>>>        + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a
>>>>        qword,
>>>>        + * and return the offset to 0x00000000 for runtime patching.
>>>>        + *
>>>>        + * Warning: runtime patching is best avoided. Only use this as
>>>>        + * a replacement for DataTableRegion (for guests that don't
>>>>        + * support it).
>>>>        + */
>>>
>>> only one comment: QWords first appeared in ACPI 2.0 and
>>> XP does not like them. Not strictly a blocker as people can
>>> avoid using the feature, but nice to have.
>>
>> Does XP have a driver for VMGENID?
>>
>> If not, then I'd prefer to stick with the qword VGIA.
>>
>>> Will either UEFI or seabios allocate
>>> memory outside 4G range? If not you do not need a qword.
>>
>> Good point (assuming XP has a driver for VMGENID).
>>
>> OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the
>> upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is
>> concerned, using a dword for the VGIA named object should be fine.
>> Accordingly, a 4-byte wide ADD_POINTER command should be used for
>> patching VGIA.
>>
>> Considering the fw_cfg file that receives the address, and
>> COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those
>> stayed 8-byte wide, regardless of XP's support for VMGENID.
>>
>>
>> Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long
>> as "Hyper-V integration services" are installed:
>>
>> https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx
>>
>>    The virtual machine must be running a guest operating system that
>>    has support for the virtual machine generation identifier.
>>    Currently, these are the following.
>>    The following operating systems have native support for the virtual
>>    machine generation identifier.
>>      [...]
>>
>>    The following operating can be used as the guest operating system
>>    if the Hyper-V integration services from Windows 8 or Windows
>>    Server 2012 are installed.
>>
>>      [...]
>>      * Windows XP with Service Pack 3 (SP3)
>>
>> Additionally, under
>> <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>:
>>
>>    Supported Windows client guest operating systems
>>
>>    Windows XP with       [...] Install the integration  [...]
>>    Service Pack 3 (SP3)        services after you set
>>                                up the operating system
>>                                in the virtual machine.
>>
>> This seems to be consistent with the VMGENID spec requirement that the
>> ADDR method return a package of two 32-bit integers, not a single 64-bit
>> one.
>>
>> So, I agree with using a dword for VGIA (and a matching 4-byte wide
>> ADD_POINTER command).
>>
>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
>> In the future we might introduce more allocation hints (for the "zone"
>> field) that would enable the firmware to allocate from the full 64-bit
>> address space.
>>
>> NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses
>> 16-bit and 32-bit modes.
>>
> Right, it appears that the allocated address will always fit in 32 bits.
>  As mentioned, XP should be OK since the ADDR method returns a package
> of two 32-bit numbers.
> 
> I propose to still include this patch but touch up the comments as
> requested by Laszlo.  This way it will be in the toolbox for future
> users and has been tested.  I will also change VGIA to dword and hard
> code the AML to return ADDR[1] = 0.  

Sounds good!

> 
> FYI: here’s an iasl dump from Windows 2012 Hyper-V in case you haven’t
> seen it.  Note that Microsoft uses E00 and violates the HID name length
> spec:

:)

Thanks!
Laszlo

> Scope (\_SB)
>    {
>        Device (GENC)
>        {
>            Name (_CID, "VM_Gen_Counter")  // _CID: Compatible ID
>            Name (_HID, "Hyper_V_Gen_Counter_V1")  // _HID: Hardware ID
>            Name (_UID, 0x00)  // _UID: Unique ID
>            Name (_DDN, "VM_Gen_Counter")  // _DDN: DOS Device Name
>            Method (ADDR, 0, NotSerialized)
>            {
>                Name (LPKG, Package (0x02)
>                {
>                    0x00, 
>                    0x00
>                })
>                LPKG [0x00] = GCAL /* \GCAL */
>                LPKG [0x01] = GCAH /* \GCAH */
>                Return (LPKG) /* \_SB_.GENC.ADDR.LPKG */
>            }
>        }
>    }
> 
>    Scope (\_GPE)
>    {
>        Method (_E00, 0, NotSerialized)  // _Exx: Edge-Triggered GPE
>        {
>            Notify (\_SB.GENC, 0x80) // Status Change
>        }
>    }
> 
>> Thanks!
>> Laszlo
>>
>>>
>>>
>>>
>>>
>>>>
>>>>    (2) Since we're adding a qword (8-byte integer), the hexadecimal
>>>>    constant should have 16 nibbles: 0x0000000000000000. (After
>>>> copying the
>>>>    comment from build_append_named_dword(), it should be actualized.)
>>>>
>>>>    (3) Normally the functions in this file that create AML operators
>>>> carry
>>>>    a note about the ACPI spec version and exact location that
>>>> defines the
>>>>    operator. I see that commit f20354910893 ("acpi: add
>>>>    build_append_named_dword, returning an offset in buffer", 2016-03-01)
>>>>    missed that too.
>>>>
>>>>    Unless this tradition has been willfully abandoned, I suggest
>>>> that you
>>>>    add the right reference here, and also (in retrospect) to
>>>>    build_append_named_dword().
>>>>
>>>>
>>>>        +int
>>>>        +build_append_named_qword(GArray *array, const char
>>>> *name_format, ...)
>>>>        +{
>>>>        +    int offset;
>>>>        +    va_list ap;
>>>>        +
>>>>        +    build_append_byte(array, 0x08); /* NameOp */
>>>>        +    va_start(ap, name_format);
>>>>        +    build_append_namestringv(array, name_format, ap);
>>>>        +    va_end(ap);
>>>>        +
>>>>        +    build_append_byte(array, 0x0E); /* QWordPrefix */
>>>>        +
>>>>        +    offset = array->len;
>>>>        +    build_append_int_noprefix(array, 0x00000000, 8);
>>>>
>>>>
>>>>    (4) I guess the constant should be updated here too, for consistency
>>>>    with the comment.
>>>>
>>>>    The rest looks okay. (I didn't verify 0x0E == QWordPrefix
>>>> specifically,
>>>>    because an error there would break the functionality immediately, and
>>>>    you'd notice.)
>>>>
>>>>    Thanks!
>>>>    Laszlo
>>>>
>>>>
>>>>        +    assert(array->len == offset + 8);
>>>>        +
>>>>        +    return offset;
>>>>        +}
>>>>        +
>>>>        static GPtrArray *alloc_list;
>>>>
>>>>        static Aml *aml_alloc(void)
>>>>        diff --git a/include/hw/acpi/aml-build.h
>>>> b/include/hw/acpi/aml-build.h
>>>>        index 559326c..dbf63cf 100644
>>>>        --- a/include/hw/acpi/aml-build.h
>>>>        +++ b/include/hw/acpi/aml-build.h
>>>>        @@ -385,6 +385,10 @@ int
>>>>        build_append_named_dword(GArray *array, const char
>>>> *name_format, ...)
>>>>        GCC_FMT_ATTR(2, 3);
>>>>
>>>>        +int
>>>>        +build_append_named_qword(GArray *array, const char
>>>> *name_format, ...)
>>>>        +GCC_FMT_ATTR(2, 3);
>>>>        +
>>>>        void build_srat_memory(AcpiSratMemoryAffinity *numamem,
>>>> uint64_t base,
>>>>                               uint64_t len, int node,
>>>> MemoryAffinityFlags
>>>>        flags);
>>>>
>>>>
>>>> —Ben
> 

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
> On 01/25/17 19:35, Michael S. Tsirkin wrote:
> > On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
> >> Hi Laszlo,
> >>
> >>
> >>     On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >>     Hi Ben,
> >>
> >>     sorry about being late to reviewing this series. I hope I can now spend
> >>     more time on it.
> >>
> >>     - Please do not try to address my comments immediately. It's very
> >>     possible (even likely) that Igor, MST and myself could have different
> >>     opinions on things, so first please await agreement between your reviewers.
> >>
> >>
> >> Thanks for the very detailed review.  I’ll give it a couple of days and then
> >> start work on the suggested changes.
> >>
> >>
> >>     - I think you should have CC'd Igor and Michael directly. I'm adding
> >>     them to this reply; hopefully that will be enough for them to monitor
> >>     this series.
> >>
> >>     - I'll likely be unable to review everything with 100% coverage; so
> >>     addressing (or sufficiently refuting) my comments might not guarantee
> >>     that the next version will be merged at once.
> >>
> >>     With all that said:
> >>
> >>     On 01/25/17 02:43, ben@skyportsystems.com wrote:
> >>
> >>         From: Ben Warren <ben@skyportsystems.com>
> >>
> >>         This is initially used to patch a 64-bit address into the VM Generation
> >>         ID SSDT
> >>
> >>
> >>     (1) I think this commit message line is overlong; I think we wrap at 74
> >>     chars or so. Not critical, but worth mentioning.
> >>
> >>
> >>
> >>         Signed-off-by: Ben Warren <ben@skyportsystems.com>
> >>         ---
> >>         hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
> >>         include/hw/acpi/aml-build.h |  4 ++++
> >>         2 files changed, 32 insertions(+)
> >>
> >>         diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> >>         index b2a1e40..dc4edc2 100644
> >>         --- a/hw/acpi/aml-build.c
> >>         +++ b/hw/acpi/aml-build.c
> >>         @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char
> >>         *name_format, ...)
> >>             return offset;
> >>         }
> >>
> >>         +/*
> >>         + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a
> >>         qword,
> >>         + * and return the offset to 0x00000000 for runtime patching.
> >>         + *
> >>         + * Warning: runtime patching is best avoided. Only use this as
> >>         + * a replacement for DataTableRegion (for guests that don't
> >>         + * support it).
> >>         + */
> > 
> > only one comment: QWords first appeared in ACPI 2.0 and
> > XP does not like them. Not strictly a blocker as people can
> > avoid using the feature, but nice to have.
> 
> Does XP have a driver for VMGENID?
> 
> If not, then I'd prefer to stick with the qword VGIA.

Not sure but in any case even if it won't be able to use it we also
don't want it to crash.

> > Will either UEFI or seabios allocate
> > memory outside 4G range? If not you do not need a qword.
> 
> Good point (assuming XP has a driver for VMGENID).
> 
> OVMF keeps all such allocations (i.e., for COMMAND_ALLOCATE and the
> upcoming COMMAND_ALLOCATE_RETURN_ADDR) under 4GB, so as far as OVMF is
> concerned, using a dword for the VGIA named object should be fine.
> Accordingly, a 4-byte wide ADD_POINTER command should be used for
> patching VGIA.
> 
> Considering the fw_cfg file that receives the address, and
> COMMAND_ALLOCATE_RETURN_ADDR more generally, I'd still prefer if those
> stayed 8-byte wide, regardless of XP's support for VMGENID.
> 
> 
> Hm... It looks like VMGENID *can* be consumed on Windows XP SP3, as long
> as "Hyper-V integration services" are installed:
> 
> https://msdn.microsoft.com/en-us/library/jj643357(v=vs.85).aspx
> 
>     The virtual machine must be running a guest operating system that
>     has support for the virtual machine generation identifier.
>     Currently, these are the following.
>     The following operating systems have native support for the virtual
>     machine generation identifier.
>       [...]
> 
>     The following operating can be used as the guest operating system
>     if the Hyper-V integration services from Windows 8 or Windows
>     Server 2012 are installed.
> 
>       [...]
>       * Windows XP with Service Pack 3 (SP3)
> 
> Additionally, under
> <https://technet.microsoft.com/en-us/library/dn792027(v=ws.11).aspx>:
> 
>     Supported Windows client guest operating systems
> 
>     Windows XP with       [...] Install the integration  [...]
>     Service Pack 3 (SP3)        services after you set
>                                 up the operating system
>                                 in the virtual machine.
> 
> This seems to be consistent with the VMGENID spec requirement that the
> ADDR method return a package of two 32-bit integers, not a single 64-bit
> one.
> 
> So, I agree with using a dword for VGIA (and a matching 4-byte wide
> ADD_POINTER command).
> 
> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.


What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
COMMAND_ALLOCATE. If we want to allow this stuff in high 64 bit, as you
correctly say we will need a new zone to allocate 64 bit memory.
As for XP support - might it be reasonable to require that
these machines have less than 4G RAM at boot?


> In the future we might introduce more allocation hints (for the "zone"
> field) that would enable the firmware to allocate from the full 64-bit
> address space.

The difficulty with new commands always was compatibility with old
firmware. I guess now that we have writeable fw cfg we will be
able to support negotiation cleanly.

Should we start now?

> NB, I didn't check SeaBIOS (should be easy for Ben); AFAIR it only uses
> 16-bit and 32-bit modes.
> 
> Thanks!
> Laszlo

Right.

> > 
> > 
> > 
> > 
> >>
> >>     (2) Since we're adding a qword (8-byte integer), the hexadecimal
> >>     constant should have 16 nibbles: 0x0000000000000000. (After copying the
> >>     comment from build_append_named_dword(), it should be actualized.)
> >>
> >>     (3) Normally the functions in this file that create AML operators carry
> >>     a note about the ACPI spec version and exact location that defines the
> >>     operator. I see that commit f20354910893 ("acpi: add
> >>     build_append_named_dword, returning an offset in buffer", 2016-03-01)
> >>     missed that too.
> >>
> >>     Unless this tradition has been willfully abandoned, I suggest that you
> >>     add the right reference here, and also (in retrospect) to
> >>     build_append_named_dword().
> >>
> >>
> >>         +int
> >>         +build_append_named_qword(GArray *array, const char *name_format, ...)
> >>         +{
> >>         +    int offset;
> >>         +    va_list ap;
> >>         +
> >>         +    build_append_byte(array, 0x08); /* NameOp */
> >>         +    va_start(ap, name_format);
> >>         +    build_append_namestringv(array, name_format, ap);
> >>         +    va_end(ap);
> >>         +
> >>         +    build_append_byte(array, 0x0E); /* QWordPrefix */
> >>         +
> >>         +    offset = array->len;
> >>         +    build_append_int_noprefix(array, 0x00000000, 8);
> >>
> >>
> >>     (4) I guess the constant should be updated here too, for consistency
> >>     with the comment.
> >>
> >>     The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically,
> >>     because an error there would break the functionality immediately, and
> >>     you'd notice.)
> >>
> >>     Thanks!
> >>     Laszlo
> >>
> >>
> >>         +    assert(array->len == offset + 8);
> >>         +
> >>         +    return offset;
> >>         +}
> >>         +
> >>         static GPtrArray *alloc_list;
> >>
> >>         static Aml *aml_alloc(void)
> >>         diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> >>         index 559326c..dbf63cf 100644
> >>         --- a/include/hw/acpi/aml-build.h
> >>         +++ b/include/hw/acpi/aml-build.h
> >>         @@ -385,6 +385,10 @@ int
> >>         build_append_named_dword(GArray *array, const char *name_format, ...)
> >>         GCC_FMT_ATTR(2, 3);
> >>
> >>         +int
> >>         +build_append_named_qword(GArray *array, const char *name_format, ...)
> >>         +GCC_FMT_ATTR(2, 3);
> >>         +
> >>         void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
> >>                                uint64_t len, int node, MemoryAffinityFlags
> >>         flags);
> >>
> >>
> >> —Ben
> >>
> > 
> > 

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/26/17 16:20, Michael S. Tsirkin wrote:
> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:

>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
> 
> 
> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
> COMMAND_ALLOCATE.

It's a new command being introduced in this series, at my suggestion. It
does the exact same thing as COMMAND_ALLOCATE, except once the
allocation / download is carried out by the firmware, the firmware
writes back the allocation address to the fw_cfg file that is named in
an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
is how QEMU learns where the blob in GPA space was placed by the
firmware.) The format for this address-receiving fw_cfg file is supposed
to be 8-byte, little endian.

My request above is simply that we stick with the 8-byte size for this
fw_cfg file, for receiving a guest allocation address. Regardless of the
fact that currently all such allocation addresses fit in 4 bytes.

> If we want to allow this stuff in high 64 bit, as you
> correctly say we will need a new zone to allocate 64 bit memory.
> As for XP support - might it be reasonable to require that
> these machines have less than 4G RAM at boot?

Perhaps; I'm not sure. At the moment I have zero concrete use cases in
mind. I just want COMMAND_ALLOCATE_RETURN_ADDR to promise the firmware
that the firmware will be able to return 8 bytes / LE as the allocation
address. How this will interact with any new zones and RAM sizes vs.
guest OSes is TBD in the future.

>> In the future we might introduce more allocation hints (for the "zone"
>> field) that would enable the firmware to allocate from the full 64-bit
>> address space.
> 
> The difficulty with new commands always was compatibility with old
> firmware. I guess now that we have writeable fw cfg we will be
> able to support negotiation cleanly.

Specifically for the zone field of COMMAND_ALLOCATE (and identically,
COMMAND_ALLOCATE_RETURN_ADDR), I think we might not need full-blown
negotiation; there aren't that many firmwares to check compatibility
with -- OVMF and SeaBIOS. If old versions of those happen to handle a
new zone value gracefully (such as "not fseg", simply), i.e. they'd
behave the same as now, then we shouldn't need negotiation. Otherwise,
we'll need it (once we have a particular use case).

> Should we start now?

No, I don't think so. I don't have any use case for 64-bit allocation;
what we have now works perfectly. I just wanted to emphasize that
permitting an 8-byte width for the alloc address to be returned is more
"future proof" than a 4-byte size, for COMMAND_ALLOCATE_RETURN_ADDR;
independently of what size we choose right here for VGIA.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
> On 01/26/17 16:20, Michael S. Tsirkin wrote:
> > On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
> 
> >> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
> > 
> > 
> > What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
> > COMMAND_ALLOCATE.
> 
> It's a new command being introduced in this series, at my suggestion. It
> does the exact same thing as COMMAND_ALLOCATE, except once the
> allocation / download is carried out by the firmware, the firmware
> writes back the allocation address to the fw_cfg file that is named in
> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
> is how QEMU learns where the blob in GPA space was placed by the
> firmware.) The format for this address-receiving fw_cfg file is supposed
> to be 8-byte, little endian.

I see. I really think it's better as a separate command though.
E.g. COMMAND_WRITE_PTR?

> My request above is simply that we stick with the 8-byte size for this
> fw_cfg file, for receiving a guest allocation address. Regardless of the
> fact that currently all such allocation addresses fit in 4 bytes.
>
> > If we want to allow this stuff in high 64 bit, as you
> > correctly say we will need a new zone to allocate 64 bit memory.
> > As for XP support - might it be reasonable to require that
> > these machines have less than 4G RAM at boot?
> 
> Perhaps; I'm not sure. At the moment I have zero concrete use cases in
> mind. I just want COMMAND_ALLOCATE_RETURN_ADDR to promise the firmware
> that the firmware will be able to return 8 bytes / LE as the allocation
> address. How this will interact with any new zones and RAM sizes vs.
> guest OSes is TBD in the future.
> 
> >> In the future we might introduce more allocation hints (for the "zone"
> >> field) that would enable the firmware to allocate from the full 64-bit
> >> address space.
> > 
> > The difficulty with new commands always was compatibility with old
> > firmware. I guess now that we have writeable fw cfg we will be
> > able to support negotiation cleanly.
> 
> Specifically for the zone field of COMMAND_ALLOCATE (and identically,
> COMMAND_ALLOCATE_RETURN_ADDR), I think we might not need full-blown
> negotiation; there aren't that many firmwares to check compatibility
> with -- OVMF and SeaBIOS. If old versions of those happen to handle a
> new zone value gracefully (such as "not fseg", simply), i.e. they'd
> behave the same as now, then we shouldn't need negotiation. Otherwise,
> we'll need it (once we have a particular use case).
> 
> > Should we start now?
> 
> No, I don't think so. I don't have any use case for 64-bit allocation;
> what we have now works perfectly. I just wanted to emphasize that
> permitting an 8-byte width for the alloc address to be returned is more
> "future proof" than a 4-byte size, for COMMAND_ALLOCATE_RETURN_ADDR;
> independently of what size we choose right here for VGIA.
> 
> Thanks,
> Laszlo

I agree here.

-- 
MST

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/26/17 19:15, Michael S. Tsirkin wrote:
> On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
>> On 01/26/17 16:20, Michael S. Tsirkin wrote:
>>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
>>
>>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
>>>
>>>
>>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
>>> COMMAND_ALLOCATE.
>>
>> It's a new command being introduced in this series, at my suggestion. It
>> does the exact same thing as COMMAND_ALLOCATE, except once the
>> allocation / download is carried out by the firmware, the firmware
>> writes back the allocation address to the fw_cfg file that is named in
>> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
>> is how QEMU learns where the blob in GPA space was placed by the
>> firmware.) The format for this address-receiving fw_cfg file is supposed
>> to be 8-byte, little endian.
> 
> I see. I really think it's better as a separate command though.
> E.g. COMMAND_WRITE_PTR?

Sure, but please provide specifics, otherwise Ben & myself will have a
hard time divining & implementing your intent :)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote:
> On 01/26/17 19:15, Michael S. Tsirkin wrote:
> > On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
> >> On 01/26/17 16:20, Michael S. Tsirkin wrote:
> >>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
> >>
> >>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
> >>>
> >>>
> >>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
> >>> COMMAND_ALLOCATE.
> >>
> >> It's a new command being introduced in this series, at my suggestion. It
> >> does the exact same thing as COMMAND_ALLOCATE, except once the
> >> allocation / download is carried out by the firmware, the firmware
> >> writes back the allocation address to the fw_cfg file that is named in
> >> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
> >> is how QEMU learns where the blob in GPA space was placed by the
> >> firmware.) The format for this address-receiving fw_cfg file is supposed
> >> to be 8-byte, little endian.
> > 
> > I see. I really think it's better as a separate command though.
> > E.g. COMMAND_WRITE_PTR?
> 
> Sure, but please provide specifics, otherwise Ben & myself will have a
> hard time divining & implementing your intent :)
> 
> Thanks,
> Laszlo

I would say a variant of ADD_POINTER:

        /*
	 * COMMAND_WRITE_POINTER - update a writeable file named
	 * @pointer.dest_file at @pointer.offset, by writing pointer to
	 * the table originating from @src_file. 1,2,4 or 8 byte
	 * unsigned write is used depending on @pointer.size.
         */
        struct {
            char dest_file[BIOS_LINKER_LOADER_FILESZ];
            char src_file[BIOS_LINKER_LOADER_FILESZ];
            uint32_t offset;
            uint8_t size;
        } pointer;

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/26/17 19:59, Michael S. Tsirkin wrote:
> On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote:
>> On 01/26/17 19:15, Michael S. Tsirkin wrote:
>>> On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
>>>> On 01/26/17 16:20, Michael S. Tsirkin wrote:
>>>>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
>>>>
>>>>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
>>>>>
>>>>>
>>>>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
>>>>> COMMAND_ALLOCATE.
>>>>
>>>> It's a new command being introduced in this series, at my suggestion. It
>>>> does the exact same thing as COMMAND_ALLOCATE, except once the
>>>> allocation / download is carried out by the firmware, the firmware
>>>> writes back the allocation address to the fw_cfg file that is named in
>>>> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
>>>> is how QEMU learns where the blob in GPA space was placed by the
>>>> firmware.) The format for this address-receiving fw_cfg file is supposed
>>>> to be 8-byte, little endian.
>>>
>>> I see. I really think it's better as a separate command though.
>>> E.g. COMMAND_WRITE_PTR?
>>
>> Sure, but please provide specifics, otherwise Ben & myself will have a
>> hard time divining & implementing your intent :)
>>
>> Thanks,
>> Laszlo
> 
> I would say a variant of ADD_POINTER:
> 
>         /*
> 	 * COMMAND_WRITE_POINTER - update a writeable file named
> 	 * @pointer.dest_file at @pointer.offset, by writing pointer to
> 	 * the table originating from @src_file. 1,2,4 or 8 byte
> 	 * unsigned write is used depending on @pointer.size.
>          */
>         struct {
>             char dest_file[BIOS_LINKER_LOADER_FILESZ];
>             char src_file[BIOS_LINKER_LOADER_FILESZ];
>             uint32_t offset;
>             uint8_t size;
>         } pointer;

This will require more work in OVMF, but it does seem feasible, and I
agree this command is significantly more flexible than what I proposed.
In particular, it allows us to receive multiple guest-side allocation
addresses into the same writeable fw_cfg file, just at different offsets
of the fw_cfg file.

For the comment block above, I have one suggestion: replace

  pointer to the table originating from @src_file

with

  pointer to the blob originating from @src_file

"table" is quite overloaded in this context. As we normally pack several
ACPI tables in a single fw_cfg blob, I like to be pedantic about "blob"
vs. "table" (restricting the latter to "ACPI table"). E.g., structured
buffer coming from "etc/vmgenid" is not an ACPI table, but "blob"
definitely covers it.

If Ben is okay with it, I think this command is a good fit. I'll make an
effort to write the OVMF patches in time to test them with one of Ben's
next patch set versions (so we can determine if the QEMU stuff works
with both SeaBIOS and OVMF before committing the QEMU patches).

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Kevin O'Connor]

On Thu, Jan 26, 2017 at 08:59:04PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote:
> > On 01/26/17 19:15, Michael S. Tsirkin wrote:
> > > On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
> > >> On 01/26/17 16:20, Michael S. Tsirkin wrote:
> > >>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
> > >>
> > >>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
> > >>>
> > >>>
> > >>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
> > >>> COMMAND_ALLOCATE.
> > >>
> > >> It's a new command being introduced in this series, at my suggestion. It
> > >> does the exact same thing as COMMAND_ALLOCATE, except once the
> > >> allocation / download is carried out by the firmware, the firmware
> > >> writes back the allocation address to the fw_cfg file that is named in
> > >> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
> > >> is how QEMU learns where the blob in GPA space was placed by the
> > >> firmware.) The format for this address-receiving fw_cfg file is supposed
> > >> to be 8-byte, little endian.
> > > 
> > > I see. I really think it's better as a separate command though.
> > > E.g. COMMAND_WRITE_PTR?
> > 
> > Sure, but please provide specifics, otherwise Ben & myself will have a
> > hard time divining & implementing your intent :)
> > 
> > Thanks,
> > Laszlo
> 
> I would say a variant of ADD_POINTER:
> 
>         /*
> 	 * COMMAND_WRITE_POINTER - update a writeable file named
> 	 * @pointer.dest_file at @pointer.offset, by writing pointer to
> 	 * the table originating from @src_file. 1,2,4 or 8 byte
> 	 * unsigned write is used depending on @pointer.size.
>          */
>         struct {
>             char dest_file[BIOS_LINKER_LOADER_FILESZ];
>             char src_file[BIOS_LINKER_LOADER_FILESZ];
>             uint32_t offset;
>             uint8_t size;
>         } pointer;
> 

I'm okay with this approach.

If an offset is going to be added, shouldn't both a source offset and
destination offset be used?

        /*
         * COMMAND_WRITE_POINTER - update a writeable file named
         * @pointer.dest_file at @pointer.dest_offset, by writing pointer
         * plus @pointer.src_offset to the blob originating from
         * @src_file. 1,2,4 or 8 byte unsigned write is used depending
         * on @pointer.size.
         */
        struct {
            char dest_file[BIOS_LINKER_LOADER_FILESZ];
            char src_file[BIOS_LINKER_LOADER_FILESZ];
            uint32_t src_offset, dest_offset;
            uint8_t size;
        } pointer;

I doubt the offsets or size is really all that important though.

-Kevin

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/27/17 15:18, Kevin O'Connor wrote:
> On Thu, Jan 26, 2017 at 08:59:04PM +0200, Michael S. Tsirkin wrote:
>> On Thu, Jan 26, 2017 at 07:25:22PM +0100, Laszlo Ersek wrote:
>>> On 01/26/17 19:15, Michael S. Tsirkin wrote:
>>>> On Thu, Jan 26, 2017 at 06:43:22PM +0100, Laszlo Ersek wrote:
>>>>> On 01/26/17 16:20, Michael S. Tsirkin wrote:
>>>>>> On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
>>>>>
>>>>>>> But, again, I'd like to keep COMMAND_ALLOCATE_RETURN_ADDR 8-byte wide.
>>>>>>
>>>>>>
>>>>>> What is COMMAND_ALLOCATE_RETURN_ADDR? I'm only familiar with
>>>>>> COMMAND_ALLOCATE.
>>>>>
>>>>> It's a new command being introduced in this series, at my suggestion. It
>>>>> does the exact same thing as COMMAND_ALLOCATE, except once the
>>>>> allocation / download is carried out by the firmware, the firmware
>>>>> writes back the allocation address to the fw_cfg file that is named in
>>>>> an additional field of the COMMAND_ALLOCATE_RETURN_ADDR structure. (This
>>>>> is how QEMU learns where the blob in GPA space was placed by the
>>>>> firmware.) The format for this address-receiving fw_cfg file is supposed
>>>>> to be 8-byte, little endian.
>>>>
>>>> I see. I really think it's better as a separate command though.
>>>> E.g. COMMAND_WRITE_PTR?
>>>
>>> Sure, but please provide specifics, otherwise Ben & myself will have a
>>> hard time divining & implementing your intent :)
>>>
>>> Thanks,
>>> Laszlo
>>
>> I would say a variant of ADD_POINTER:
>>
>>         /*
>> 	 * COMMAND_WRITE_POINTER - update a writeable file named
>> 	 * @pointer.dest_file at @pointer.offset, by writing pointer to
>> 	 * the table originating from @src_file. 1,2,4 or 8 byte
>> 	 * unsigned write is used depending on @pointer.size.
>>          */
>>         struct {
>>             char dest_file[BIOS_LINKER_LOADER_FILESZ];
>>             char src_file[BIOS_LINKER_LOADER_FILESZ];
>>             uint32_t offset;
>>             uint8_t size;
>>         } pointer;
>>
> 
> I'm okay with this approach.
> 
> If an offset is going to be added, shouldn't both a source offset and
> destination offset be used?
> 
>         /*
>          * COMMAND_WRITE_POINTER - update a writeable file named
>          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
>          * plus @pointer.src_offset to the blob originating from
>          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
>          * on @pointer.size.
>          */
>         struct {
>             char dest_file[BIOS_LINKER_LOADER_FILESZ];
>             char src_file[BIOS_LINKER_LOADER_FILESZ];
>             uint32_t src_offset, dest_offset;
>             uint8_t size;
>         } pointer;
> 
> I doubt the offsets or size is really all that important though.

The offset into the fw_cfg file that receives the allocation address is
important, that allows the same file to receive several different
addresses (for different downloaded blobs), at different offsets.

OTOH, asking the firmware to add a constant to the address value before
writing it to the fw_cfg file is not necessary, in my opinion. The blob
that the firmware allocated and downloaded originates from QEMU to begin
with, so QEMU knows its internal structure.

Here's a diagram for ADD_POINTER:

blob-to-patch
+---------------+
|               |
| pointer = N   |-----------+
|               |           |       pointed-to-blob
+---------------+           |       +----------------+
                            |       |                |
                            +-----> | offset N: ...  |
                                    |                |
                                    +----------------+

In this case, QEMU pre-populates the pointer field in "blob-to-patch"
with the value N (a small relative offset, from the beginning of
"pointed-to-blob"). The firmware then increases the pointer field in
"blob-to-patch" with the absolute allocation address of
"pointed-to-blob". Thus "pointer" will point to the absolute address of
offset N into "pointed-to-blob".

This is useful primarily when the "pointer" field in "blob-to-patch" is
part of an ACPI table in "blob-to-patch". ACPI tables are consumed by
the guest OS.

The information necessary for the above is:
- name of "blob-to-patch"
- name of "pointed-to-blob"
- relative address of pointer field to patch within "blob-to-patch"
- size of the same

Compare WRITE_POINTER:

fw-cfg-file-to-rewrite
+---------------+
|               |
| pointer = XXX |-----------+
|               |           |       pointed-to-blob
+---------------+           +-----> +----------------+
                                    |                |
                                    | offset N: ...  |
                                    | offset K: ...  |
                                    |                |
                                    +----------------+

In this case, the "pointer field" that the firmware should update lives
inside QEMU only. The only consumer for it is QEMU itself, from which
"pointed-to-blob" originates too. The original value XXX is irrelevant.
The firmware writes the base address of "pointed-to-blob" over XXX, and
then QEMU can add N whenever it wants to access the field at offset N in
"pointed-to-blob". It can add K too, whenever it wants to access another
field in "pointed-to-blob".

The information necessary for this command is:
- name of "fw-cfg-file-to-rewrite"
- name of "pointed-to-blob"
- offset of "pointer field" to overwrite within "fw-cfg-file-to-rewrite"
- size of the same

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Kevin O'Connor]

On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:
> On 01/27/17 15:18, Kevin O'Connor wrote:
> > If an offset is going to be added, shouldn't both a source offset and
> > destination offset be used?
> > 
> >         /*
> >          * COMMAND_WRITE_POINTER - update a writeable file named
> >          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
> >          * plus @pointer.src_offset to the blob originating from
> >          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
> >          * on @pointer.size.
> >          */
> >         struct {
> >             char dest_file[BIOS_LINKER_LOADER_FILESZ];
> >             char src_file[BIOS_LINKER_LOADER_FILESZ];
> >             uint32_t src_offset, dest_offset;
> >             uint8_t size;
> >         } pointer;
> > 
> > I doubt the offsets or size is really all that important though.
> 
> The offset into the fw_cfg file that receives the allocation address is
> important, that allows the same file to receive several different
> addresses (for different downloaded blobs), at different offsets.
> 
> OTOH, asking the firmware to add a constant to the address value before
> writing it to the fw_cfg file is not necessary, in my opinion. The blob
> that the firmware allocated and downloaded originates from QEMU to begin
> with, so QEMU knows its internal structure.

I guess I'm missing why QEMU would want to use the same writable file
for multiple pointers as well as why it would want support for
pointers smaller than 8 bytes in size.  If it's because it may be
easier to support an internal QEMU blob of a particular format, then
adding a src_offset would facilitate that.

However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
that's fine too.  I'm okay with either format.

-Kevin

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/27/17 16:43, Kevin O'Connor wrote:
> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:
>> On 01/27/17 15:18, Kevin O'Connor wrote:
>>> If an offset is going to be added, shouldn't both a source offset and
>>> destination offset be used?
>>>
>>>         /*
>>>          * COMMAND_WRITE_POINTER - update a writeable file named
>>>          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
>>>          * plus @pointer.src_offset to the blob originating from
>>>          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
>>>          * on @pointer.size.
>>>          */
>>>         struct {
>>>             char dest_file[BIOS_LINKER_LOADER_FILESZ];
>>>             char src_file[BIOS_LINKER_LOADER_FILESZ];
>>>             uint32_t src_offset, dest_offset;
>>>             uint8_t size;
>>>         } pointer;
>>>
>>> I doubt the offsets or size is really all that important though.
>>
>> The offset into the fw_cfg file that receives the allocation address is
>> important, that allows the same file to receive several different
>> addresses (for different downloaded blobs), at different offsets.
>>
>> OTOH, asking the firmware to add a constant to the address value before
>> writing it to the fw_cfg file is not necessary, in my opinion. The blob
>> that the firmware allocated and downloaded originates from QEMU to begin
>> with, so QEMU knows its internal structure.
> 
> I guess I'm missing why QEMU would want to use the same writable file
> for multiple pointers

I know no specific reason; I just thought this possible generalization
was one of the advantages in Michael's suggestion.

> as well as why it would want support for
> pointers smaller than 8 bytes in size.

Hm, right, good point.

> If it's because it may be
> easier to support an internal QEMU blob of a particular format, then
> adding a src_offset would facilitate that.
> 
> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
> that's fine too.

That might be the main reason I guess; reading back a bit, Michael wrote
"... a variant of ADD_POINTER".

>  I'm okay with either format.

I'd say let's go ahead with Michael's proposal then. Ben, are you okay
with that?

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Ben Warren]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]


> On Jan 27, 2017, at 8:12 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 01/27/17 16:43, Kevin O'Connor wrote:
>> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:
>>> On 01/27/17 15:18, Kevin O'Connor wrote:
>>>> If an offset is going to be added, shouldn't both a source offset and
>>>> destination offset be used?
>>>> 
>>>>        /*
>>>>         * COMMAND_WRITE_POINTER - update a writeable file named
>>>>         * @pointer.dest_file at @pointer.dest_offset, by writing pointer
>>>>         * plus @pointer.src_offset to the blob originating from
>>>>         * @src_file. 1,2,4 or 8 byte unsigned write is used depending
>>>>         * on @pointer.size.
>>>>         */
>>>>        struct {
>>>>            char dest_file[BIOS_LINKER_LOADER_FILESZ];
>>>>            char src_file[BIOS_LINKER_LOADER_FILESZ];
>>>>            uint32_t src_offset, dest_offset;
>>>>            uint8_t size;
>>>>        } pointer;
>>>> 
>>>> I doubt the offsets or size is really all that important though.
>>> 
>>> The offset into the fw_cfg file that receives the allocation address is
>>> important, that allows the same file to receive several different
>>> addresses (for different downloaded blobs), at different offsets.
>>> 
>>> OTOH, asking the firmware to add a constant to the address value before
>>> writing it to the fw_cfg file is not necessary, in my opinion. The blob
>>> that the firmware allocated and downloaded originates from QEMU to begin
>>> with, so QEMU knows its internal structure.
>> 
>> I guess I'm missing why QEMU would want to use the same writable file
>> for multiple pointers
> 
> I know no specific reason; I just thought this possible generalization
> was one of the advantages in Michael's suggestion.
> 
>> as well as why it would want support for
>> pointers smaller than 8 bytes in size.
> 
> Hm, right, good point.
> 
>> If it's because it may be
>> easier to support an internal QEMU blob of a particular format, then
>> adding a src_offset would facilitate that.
>> 
>> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
>> that's fine too.
> 
> That might be the main reason I guess; reading back a bit, Michael wrote
> "... a variant of ADD_POINTER".
> 
>> I'm okay with either format.
> 
> I'd say let's go ahead with Michael's proposal then. Ben, are you okay
> with that?
> 
Yes, this seems reasonable.  If I’m interpreting this properly, it’s like ADD_POINTER, except that we extend the write path back to QEMU over DMA.  Is that about right?

> Thanks!
> Laszlo
—Ben


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3583 bytes --]

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 01/27/17 19:19, Ben Warren wrote:
> 
>> On Jan 27, 2017, at 8:12 AM, Laszlo Ersek <lersek@redhat.com
>> <mailto:lersek@redhat.com>> wrote:
>>
>> On 01/27/17 16:43, Kevin O'Connor wrote:
>>> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:
>>>> On 01/27/17 15:18, Kevin O'Connor wrote:
>>>>> If an offset is going to be added, shouldn't both a source offset and
>>>>> destination offset be used?
>>>>>
>>>>>        /*
>>>>>         * COMMAND_WRITE_POINTER - update a writeable file named
>>>>>         * @pointer.dest_file at @pointer.dest_offset, by writing
>>>>> pointer
>>>>>         * plus @pointer.src_offset to the blob originating from
>>>>>         * @src_file. 1,2,4 or 8 byte unsigned write is used depending
>>>>>         * on @pointer.size.
>>>>>         */
>>>>>        struct {
>>>>>            char dest_file[BIOS_LINKER_LOADER_FILESZ];
>>>>>            char src_file[BIOS_LINKER_LOADER_FILESZ];
>>>>>            uint32_t src_offset, dest_offset;
>>>>>            uint8_t size;
>>>>>        } pointer;
>>>>>
>>>>> I doubt the offsets or size is really all that important though.
>>>>
>>>> The offset into the fw_cfg file that receives the allocation address is
>>>> important, that allows the same file to receive several different
>>>> addresses (for different downloaded blobs), at different offsets.
>>>>
>>>> OTOH, asking the firmware to add a constant to the address value before
>>>> writing it to the fw_cfg file is not necessary, in my opinion. The blob
>>>> that the firmware allocated and downloaded originates from QEMU to begin
>>>> with, so QEMU knows its internal structure.
>>>
>>> I guess I'm missing why QEMU would want to use the same writable file
>>> for multiple pointers
>>
>> I know no specific reason; I just thought this possible generalization
>> was one of the advantages in Michael's suggestion.
>>
>>> as well as why it would want support for
>>> pointers smaller than 8 bytes in size.
>>
>> Hm, right, good point.
>>
>>> If it's because it may be
>>> easier to support an internal QEMU blob of a particular format, then
>>> adding a src_offset would facilitate that.
>>>
>>> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
>>> that's fine too.
>>
>> That might be the main reason I guess; reading back a bit, Michael wrote
>> "... a variant of ADD_POINTER".
>>
>>> I'm okay with either format.
>>
>> I'd say let's go ahead with Michael's proposal then. Ben, are you okay
>> with that?
>>
> Yes, this seems reasonable.  If I’m interpreting this properly, it’s
> like ADD_POINTER, except that we extend the write path back to QEMU over
> DMA.  Is that about right?

Yes. Basically the command differs from ADD_POINTER in that (a) the
pointer field to patch lives in "fw_cfg space", not in guest RAM, so
rather than updating guest RAM, the firmware has to select a writeable
fw_cfg file, seek ("skip") to the specified offset, and rewrite the
field, (b) the nature of the patching is not "increment", just a plain
"overwrite".

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote:
> On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:
> > On 01/27/17 15:18, Kevin O'Connor wrote:
> > > If an offset is going to be added, shouldn't both a source offset and
> > > destination offset be used?
> > > 
> > >         /*
> > >          * COMMAND_WRITE_POINTER - update a writeable file named
> > >          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
> > >          * plus @pointer.src_offset to the blob originating from
> > >          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
> > >          * on @pointer.size.
> > >          */
> > >         struct {
> > >             char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > >             char src_file[BIOS_LINKER_LOADER_FILESZ];
> > >             uint32_t src_offset, dest_offset;
> > >             uint8_t size;
> > >         } pointer;
> > > 
> > > I doubt the offsets or size is really all that important though.
> > 
> > The offset into the fw_cfg file that receives the allocation address is
> > important, that allows the same file to receive several different
> > addresses (for different downloaded blobs), at different offsets.
> > 
> > OTOH, asking the firmware to add a constant to the address value before
> > writing it to the fw_cfg file is not necessary, in my opinion. The blob
> > that the firmware allocated and downloaded originates from QEMU to begin
> > with, so QEMU knows its internal structure.
> 
> I guess I'm missing why QEMU would want to use the same writable file
> for multiple pointers as well as why it would want support for
> pointers smaller than 8 bytes in size.  If it's because it may be
> easier to support an internal QEMU blob of a particular format, then
> adding a src_offset would facilitate that.
> 
> However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
> that's fine too.  I'm okay with either format.
> 
> -Kevin

Both reasons :) offset is because it's easier for QEMU not to have to add
more files (e.g. it simplifies cross-version migration if we don't).
size is to mimick ADD_POINTER.

-- 
MST

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Igor Mammedov]

On Mon, 30 Jan 2017 22:28:41 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote:
> > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:  
> > > On 01/27/17 15:18, Kevin O'Connor wrote:  
> > > > If an offset is going to be added, shouldn't both a source offset and
> > > > destination offset be used?
> > > > 
> > > >         /*
> > > >          * COMMAND_WRITE_POINTER - update a writeable file named
> > > >          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
> > > >          * plus @pointer.src_offset to the blob originating from
> > > >          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
> > > >          * on @pointer.size.
> > > >          */
> > > >         struct {
> > > >             char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > > >             char src_file[BIOS_LINKER_LOADER_FILESZ];
> > > >             uint32_t src_offset, dest_offset;
> > > >             uint8_t size;
> > > >         } pointer;
> > > > 
> > > > I doubt the offsets or size is really all that important though.  
> > > 
> > > The offset into the fw_cfg file that receives the allocation address is
> > > important, that allows the same file to receive several different
> > > addresses (for different downloaded blobs), at different offsets.
> > > 
> > > OTOH, asking the firmware to add a constant to the address value before
> > > writing it to the fw_cfg file is not necessary, in my opinion. The blob
> > > that the firmware allocated and downloaded originates from QEMU to begin
> > > with, so QEMU knows its internal structure.  
> > 
> > I guess I'm missing why QEMU would want to use the same writable file
> > for multiple pointers as well as why it would want support for
> > pointers smaller than 8 bytes in size.  If it's because it may be
> > easier to support an internal QEMU blob of a particular format, then
> > adding a src_offset would facilitate that.
> > 
> > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
> > that's fine too.  I'm okay with either format.
> > 
> > -Kevin  
> 
> Both reasons :) offset is because it's easier for QEMU not to have to add
> more files (e.g. it simplifies cross-version migration if we don't).
On one hand offset simplifies since one file could be re-used for
several pointers, on the other hand it doesn't make difference wrt
migration since offset becomes ABI and has to be maintained in
cross-version migration scenario (size of file shouldn't be issue
as they are re-sizable now). So we just end-up with offset vs new file
versioning. However considering that number of files is limited,
offset scales up better.

> size is to mimick ADD_POINTER.
> 

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Tue, Jan 31, 2017 at 10:51:02AM +0100, Igor Mammedov wrote:
> On Mon, 30 Jan 2017 22:28:41 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote:
> > > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:  
> > > > On 01/27/17 15:18, Kevin O'Connor wrote:  
> > > > > If an offset is going to be added, shouldn't both a source offset and
> > > > > destination offset be used?
> > > > > 
> > > > >         /*
> > > > >          * COMMAND_WRITE_POINTER - update a writeable file named
> > > > >          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
> > > > >          * plus @pointer.src_offset to the blob originating from
> > > > >          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
> > > > >          * on @pointer.size.
> > > > >          */
> > > > >         struct {
> > > > >             char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > > > >             char src_file[BIOS_LINKER_LOADER_FILESZ];
> > > > >             uint32_t src_offset, dest_offset;
> > > > >             uint8_t size;
> > > > >         } pointer;
> > > > > 
> > > > > I doubt the offsets or size is really all that important though.  
> > > > 
> > > > The offset into the fw_cfg file that receives the allocation address is
> > > > important, that allows the same file to receive several different
> > > > addresses (for different downloaded blobs), at different offsets.
> > > > 
> > > > OTOH, asking the firmware to add a constant to the address value before
> > > > writing it to the fw_cfg file is not necessary, in my opinion. The blob
> > > > that the firmware allocated and downloaded originates from QEMU to begin
> > > > with, so QEMU knows its internal structure.  
> > > 
> > > I guess I'm missing why QEMU would want to use the same writable file
> > > for multiple pointers as well as why it would want support for
> > > pointers smaller than 8 bytes in size.  If it's because it may be
> > > easier to support an internal QEMU blob of a particular format, then
> > > adding a src_offset would facilitate that.
> > > 
> > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
> > > that's fine too.  I'm okay with either format.
> > > 
> > > -Kevin  
> > 
> > Both reasons :) offset is because it's easier for QEMU not to have to add
> > more files (e.g. it simplifies cross-version migration if we don't).
> On one hand offset simplifies since one file could be re-used for
> several pointers, on the other hand it doesn't make difference wrt
> migration since offset becomes ABI and has to be maintained in
> cross-version migration scenario (size of file shouldn't be issue
> as they are re-sizable now). So we just end-up with offset vs new file
> versioning.

Not really - offset is migrated automatically since it's in RAM.
No need to version it.

> However considering that number of files is limited,
> offset scales up better.
> 
> > size is to mimick ADD_POINTER.
> > 

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Igor Mammedov]

On Tue, 31 Jan 2017 23:39:44 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jan 31, 2017 at 10:51:02AM +0100, Igor Mammedov wrote:
> > On Mon, 30 Jan 2017 22:28:41 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote:  
> > > > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:    
> > > > > On 01/27/17 15:18, Kevin O'Connor wrote:    
> > > > > > If an offset is going to be added, shouldn't both a source offset and
> > > > > > destination offset be used?
> > > > > > 
> > > > > >         /*
> > > > > >          * COMMAND_WRITE_POINTER - update a writeable file named
> > > > > >          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
> > > > > >          * plus @pointer.src_offset to the blob originating from
> > > > > >          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
> > > > > >          * on @pointer.size.
> > > > > >          */
> > > > > >         struct {
> > > > > >             char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > > > > >             char src_file[BIOS_LINKER_LOADER_FILESZ];
> > > > > >             uint32_t src_offset, dest_offset;
> > > > > >             uint8_t size;
> > > > > >         } pointer;
> > > > > > 
> > > > > > I doubt the offsets or size is really all that important though.    
> > > > > 
> > > > > The offset into the fw_cfg file that receives the allocation address is
> > > > > important, that allows the same file to receive several different
> > > > > addresses (for different downloaded blobs), at different offsets.
> > > > > 
> > > > > OTOH, asking the firmware to add a constant to the address value before
> > > > > writing it to the fw_cfg file is not necessary, in my opinion. The blob
> > > > > that the firmware allocated and downloaded originates from QEMU to begin
> > > > > with, so QEMU knows its internal structure.    
> > > > 
> > > > I guess I'm missing why QEMU would want to use the same writable file
> > > > for multiple pointers as well as why it would want support for
> > > > pointers smaller than 8 bytes in size.  If it's because it may be
> > > > easier to support an internal QEMU blob of a particular format, then
> > > > adding a src_offset would facilitate that.
> > > > 
> > > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
> > > > that's fine too.  I'm okay with either format.
> > > > 
> > > > -Kevin    
> > > 
> > > Both reasons :) offset is because it's easier for QEMU not to have to add
> > > more files (e.g. it simplifies cross-version migration if we don't).  
> > On one hand offset simplifies since one file could be re-used for
> > several pointers, on the other hand it doesn't make difference wrt
> > migration since offset becomes ABI and has to be maintained in
> > cross-version migration scenario (size of file shouldn't be issue
> > as they are re-sizable now). So we just end-up with offset vs new file
> > versioning.  
> 
> Not really - offset is migrated automatically since it's in RAM.
> No need to version it.
I've meant offset inside of writebale blob dest_offset,
it has to stay the same within a machine type across builds,
so that if migration happens during linking time, the old
already shadowed and migrated liker file would match offsets in
writable fwcfg file on new qemu.
In other words format of writable fw_cfg file becomes ABI.

> 
> > However considering that number of files is limited,
> > offset scales up better.
> >   
> > > size is to mimick ADD_POINTER.
> > >   

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries

[Michael S. Tsirkin]

On Wed, Feb 01, 2017 at 12:46:47PM +0100, Igor Mammedov wrote:
> On Tue, 31 Jan 2017 23:39:44 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jan 31, 2017 at 10:51:02AM +0100, Igor Mammedov wrote:
> > > On Mon, 30 Jan 2017 22:28:41 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Fri, Jan 27, 2017 at 10:43:13AM -0500, Kevin O'Connor wrote:  
> > > > > On Fri, Jan 27, 2017 at 03:46:33PM +0100, Laszlo Ersek wrote:    
> > > > > > On 01/27/17 15:18, Kevin O'Connor wrote:    
> > > > > > > If an offset is going to be added, shouldn't both a source offset and
> > > > > > > destination offset be used?
> > > > > > > 
> > > > > > >         /*
> > > > > > >          * COMMAND_WRITE_POINTER - update a writeable file named
> > > > > > >          * @pointer.dest_file at @pointer.dest_offset, by writing pointer
> > > > > > >          * plus @pointer.src_offset to the blob originating from
> > > > > > >          * @src_file. 1,2,4 or 8 byte unsigned write is used depending
> > > > > > >          * on @pointer.size.
> > > > > > >          */
> > > > > > >         struct {
> > > > > > >             char dest_file[BIOS_LINKER_LOADER_FILESZ];
> > > > > > >             char src_file[BIOS_LINKER_LOADER_FILESZ];
> > > > > > >             uint32_t src_offset, dest_offset;
> > > > > > >             uint8_t size;
> > > > > > >         } pointer;
> > > > > > > 
> > > > > > > I doubt the offsets or size is really all that important though.    
> > > > > > 
> > > > > > The offset into the fw_cfg file that receives the allocation address is
> > > > > > important, that allows the same file to receive several different
> > > > > > addresses (for different downloaded blobs), at different offsets.
> > > > > > 
> > > > > > OTOH, asking the firmware to add a constant to the address value before
> > > > > > writing it to the fw_cfg file is not necessary, in my opinion. The blob
> > > > > > that the firmware allocated and downloaded originates from QEMU to begin
> > > > > > with, so QEMU knows its internal structure.    
> > > > > 
> > > > > I guess I'm missing why QEMU would want to use the same writable file
> > > > > for multiple pointers as well as why it would want support for
> > > > > pointers smaller than 8 bytes in size.  If it's because it may be
> > > > > easier to support an internal QEMU blob of a particular format, then
> > > > > adding a src_offset would facilitate that.
> > > > > 
> > > > > However, if it was done so that WRITE_POINTER mimicks ADD_POINTER then
> > > > > that's fine too.  I'm okay with either format.
> > > > > 
> > > > > -Kevin    
> > > > 
> > > > Both reasons :) offset is because it's easier for QEMU not to have to add
> > > > more files (e.g. it simplifies cross-version migration if we don't).  
> > > On one hand offset simplifies since one file could be re-used for
> > > several pointers, on the other hand it doesn't make difference wrt
> > > migration since offset becomes ABI and has to be maintained in
> > > cross-version migration scenario (size of file shouldn't be issue
> > > as they are re-sizable now). So we just end-up with offset vs new file
> > > versioning.  
> > 
> > Not really - offset is migrated automatically since it's in RAM.
> > No need to version it.
> I've meant offset inside of writebale blob dest_offset,
> it has to stay the same within a machine type across builds,
> so that if migration happens during linking time, the old
> already shadowed and migrated liker file would match offsets in
> writable fwcfg file on new qemu.
> In other words format of writable fw_cfg file becomes ABI.

We can always add new offsets without versioning though.
So yes an ABI but easier to extend.

> > 
> > > However considering that number of files is limited,
> > > offset scales up better.
> > >   
> > > > size is to mimick ADD_POINTER.
> > > >   

[Qemu-devel] [PATCH v4 2/9] linker-loader: Add new 'allocate and return address' cmd

[ben]

From: Ben Warren <ben@skyportsystems.com>

This adds a new linker-loader command to instruct the guest to allocate
memory for a fw_cfg file and write the address back into another
writeable fw_cfg file.  Knowing this address, QEMU can then write into
guest memory at runtime.

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 hw/acpi/bios-linker-loader.c         | 71 ++++++++++++++++++++++++++++++++++--
 include/hw/acpi/bios-linker-loader.h |  7 ++++
 2 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
index d963ebe..1d991ba 100644
--- a/hw/acpi/bios-linker-loader.c
+++ b/hw/acpi/bios-linker-loader.c
@@ -78,6 +78,22 @@ struct BiosLinkerLoaderEntry {
             uint32_t length;
         } cksum;
 
+        /*
+         * COMMAND_ALLOCATE_RETURN_ADDR - allocate a table from @alloc_ret_file
+         * subject to @alloc_ret_align alignment (must be power of 2)
+         * and @alloc_ret_zone (can be HIGH or FSEG) requirements.
+         * Additionally, return the address of the allocation in
+         * @addr_file.
+         *
+         * This may be used instead of COMMAND_ALLOCATE
+         */
+        struct {
+            char alloc_ret_file[BIOS_LINKER_LOADER_FILESZ];
+            uint32_t alloc_ret_align;
+            uint8_t alloc_ret_zone;
+            char alloc_ret_addr_file[BIOS_LINKER_LOADER_FILESZ];
+        };
+
         /* padding */
         char pad[124];
     };
@@ -85,9 +101,10 @@ struct BiosLinkerLoaderEntry {
 typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
 
 enum {
-    BIOS_LINKER_LOADER_COMMAND_ALLOCATE     = 0x1,
-    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER  = 0x2,
-    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
+    BIOS_LINKER_LOADER_COMMAND_ALLOCATE          = 0x1,
+    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER       = 0x2,
+    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
+    BIOS_LINKER_LOADER_COMMAND_ALLOCATE_RET_ADDR = 0x4,
 };
 
 enum {
@@ -278,3 +295,51 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
 
     g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
 }
+
+/*
+ * bios_linker_loader_alloc_ret_addr: ask guest to load file into guest memory
+ * and patch the address in another file
+ *
+ * @linker: linker object instance
+ * @data_file: name of the file blob to be loaded
+ * @file_blob: pointer to blob corresponding to @file_name
+ * @alloc_align: required minimal alignment in bytes. Must be a power of 2.
+ * @alloc_fseg: request allocation in FSEG zone (useful for the RSDP ACPI table)
+ * @addr_file: destination file that will contain the address.
+ *             This must already exist
+ *
+ * Note: this command must precede any other linker command that uses
+ * the data file.
+ */
+void bios_linker_loader_alloc_ret_addr(BIOSLinker *linker,
+                              const char *data_file,
+                              GArray *file_blob,
+                              uint32_t alloc_align,
+                              bool alloc_fseg,
+                              const char *addr_file)
+{
+    BiosLinkerLoaderEntry entry;
+    BiosLinkerFileEntry d_file = { g_strdup(data_file), file_blob};
+
+    /* Address file is expected to already be loaded */
+    const BiosLinkerFileEntry *a_file =
+        bios_linker_find_file(linker, addr_file);
+
+    assert(!(alloc_align & (alloc_align - 1)));
+    assert(a_file);
+
+    g_array_append_val(linker->file_list, d_file);
+
+    memset(&entry, 0, sizeof entry);
+    strncpy(entry.alloc_ret_file, data_file,
+            sizeof entry.alloc_ret_file - 1);
+    strncpy(entry.alloc_ret_addr_file, addr_file,
+            sizeof entry.alloc_ret_addr_file - 1);
+    entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE_RET_ADDR);
+    entry.alloc.align = cpu_to_le32(alloc_align);
+    entry.alloc.zone = alloc_fseg ? BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
+                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH;
+
+    /* Alloc entries must come first, so prepend them */
+    g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
+}
diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
index fa1e5d1..69953e6 100644
--- a/include/hw/acpi/bios-linker-loader.h
+++ b/include/hw/acpi/bios-linker-loader.h
@@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
                                     const char *src_file,
                                     uint32_t src_offset);
 
+void bios_linker_loader_alloc_ret_addr(BIOSLinker *linker,
+                                       const char *data_file,
+                                       GArray *file_blob,
+                                       uint32_t alloc_align,
+                                       bool alloc_fseg,
+                                       const char *addr_file);
+
 void bios_linker_loader_cleanup(BIOSLinker *linker);
 #endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 2/9] linker-loader: Add new 'allocate and return address' cmd

[Laszlo Ersek]

On 01/25/17 02:43, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> This adds a new linker-loader command to instruct the guest to allocate
> memory for a fw_cfg file and write the address back into another
> writeable fw_cfg file.  Knowing this address, QEMU can then write into
> guest memory at runtime.
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  hw/acpi/bios-linker-loader.c         | 71 ++++++++++++++++++++++++++++++++++--
>  include/hw/acpi/bios-linker-loader.h |  7 ++++
>  2 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index d963ebe..1d991ba 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -78,6 +78,22 @@ struct BiosLinkerLoaderEntry {
>              uint32_t length;
>          } cksum;
>  
> +        /*
> +         * COMMAND_ALLOCATE_RETURN_ADDR - allocate a table from @alloc_ret_file
> +         * subject to @alloc_ret_align alignment (must be power of 2)
> +         * and @alloc_ret_zone (can be HIGH or FSEG) requirements.
> +         * Additionally, return the address of the allocation in
> +         * @addr_file.
> +         *
> +         * This may be used instead of COMMAND_ALLOCATE
> +         */
> +        struct {
> +            char alloc_ret_file[BIOS_LINKER_LOADER_FILESZ];
> +            uint32_t alloc_ret_align;
> +            uint8_t alloc_ret_zone;
> +            char alloc_ret_addr_file[BIOS_LINKER_LOADER_FILESZ];
> +        };
> +
>          /* padding */
>          char pad[124];
>      };

(1) I remember that, when we discussed this command first, I provided
you with an explicit list of fields, for the new command structure.
Nonetheless, I suggest rephrasing both the comment block and the
structure definition in terms of the existent COMMAND_ALLOCATE:

- please give an actual struct tag to the structure that describes
COMMAND_ALLOCATE,
- reuse that type, as the first field, of the new structure,
- only add the new, last "addr_file" field explicitly.
  (This name is also simpler than "alloc_ret_addr_file".)

(2) Furthermore, the new union member lacks a name. It should be called
"alloc_ret_addr".

(3) The documentation can say something like,

  COMMAND_ALLOCATE_RETURN_ADDR - perform the COMMAND_ALLOCATE request
  described in @alloc_ret_addr.alloc, then write the resultant 8-byte
  allocation address, in little-endian encoding, to the fw_cfg file
  denoted by @alloc_ret_addr.addr_file.

> @@ -85,9 +101,10 @@ struct BiosLinkerLoaderEntry {
>  typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
>  
>  enum {
> -    BIOS_LINKER_LOADER_COMMAND_ALLOCATE     = 0x1,
> -    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER  = 0x2,
> -    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> +    BIOS_LINKER_LOADER_COMMAND_ALLOCATE          = 0x1,
> +    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER       = 0x2,
> +    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM      = 0x3,
> +    BIOS_LINKER_LOADER_COMMAND_ALLOCATE_RET_ADDR = 0x4,
>  };
>  
>  enum {
> @@ -278,3 +295,51 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>  
>      g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
>  }
> +
> +/*
> + * bios_linker_loader_alloc_ret_addr: ask guest to load file into guest memory
> + * and patch the address in another file

(4) I suggest: "and return the allocation address in another file".

> + *
> + * @linker: linker object instance
> + * @data_file: name of the file blob to be loaded
> + * @file_blob: pointer to blob corresponding to @file_name

(5) You renamed @file_name to @data_file, but then the reference on the
next line wasn't updated. I suggest the following names:

@data_file_name
@data_file_blob

and consistent references.

> + * @alloc_align: required minimal alignment in bytes. Must be a power of 2.
> + * @alloc_fseg: request allocation in FSEG zone (useful for the RSDP ACPI table)
> + * @addr_file: destination file that will contain the address.
> + *             This must already exist

(6) For consistency, I suggest @addr_file_name.

> + *
> + * Note: this command must precede any other linker command that uses
> + * the data file.
> + */
> +void bios_linker_loader_alloc_ret_addr(BIOSLinker *linker,
> +                              const char *data_file,
> +                              GArray *file_blob,
> +                              uint32_t alloc_align,
> +                              bool alloc_fseg,
> +                              const char *addr_file)

(7) White space should be updated to line up the parameters with the
opening paren.

> +{
> +    BiosLinkerLoaderEntry entry;
> +    BiosLinkerFileEntry d_file = { g_strdup(data_file), file_blob};
> +
> +    /* Address file is expected to already be loaded */
> +    const BiosLinkerFileEntry *a_file =
> +        bios_linker_find_file(linker, addr_file);

(8) That's incorrect. The fw_cfg file that receives the allocation
address is to be written-to by the firmware; it doesn't need to be
downloaded into any firmware-allocated array. Therefore we shouldn't try
to enforce that @addr_file_name has been appended to "linker->file_list".

The "a_file" variable can be dropped IMO.

> +
> +    assert(!(alloc_align & (alloc_align - 1)));
> +    assert(a_file);
> +
> +    g_array_append_val(linker->file_list, d_file);

(9) I think the assertion is worth preserving (from
bios_linker_loader_alloc()) that the data file name doesn't exist yet.

> +
> +    memset(&entry, 0, sizeof entry);
> +    strncpy(entry.alloc_ret_file, data_file,
> +            sizeof entry.alloc_ret_file - 1);
> +    strncpy(entry.alloc_ret_addr_file, addr_file,
> +            sizeof entry.alloc_ret_addr_file - 1);
> +    entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE_RET_ADDR);
> +    entry.alloc.align = cpu_to_le32(alloc_align);
> +    entry.alloc.zone = alloc_fseg ? BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
> +                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH;
> +
> +    /* Alloc entries must come first, so prepend them */
> +    g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
> +}

(10) The logic is messy here. The code mixes direct access to fields of
the new, unnamed, union member structure -- see point (2) near the top
--, with access to fields of the "entry.alloc" union member that
corresponds to COMMAND_ALLOCATE.

Instead, please give an explicit name to the new union member (see (2)
again), i.e., "alloc_ret_addr", and then refer to the following fields:

- entry.alloc_ret_addr.alloc.file
- entry.alloc_ret_addr.alloc.align
- entry.alloc_ret_addr.alloc.zone
- entry.alloc_ret_addr.addr_file

You can of course use pointers to the sub-structures, for saving source
code text.

Thanks!
Laszlo

> diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
> index fa1e5d1..69953e6 100644
> --- a/include/hw/acpi/bios-linker-loader.h
> +++ b/include/hw/acpi/bios-linker-loader.h
> @@ -26,5 +26,12 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>                                      const char *src_file,
>                                      uint32_t src_offset);
>  
> +void bios_linker_loader_alloc_ret_addr(BIOSLinker *linker,
> +                                       const char *data_file,
> +                                       GArray *file_blob,
> +                                       uint32_t alloc_align,
> +                                       bool alloc_fseg,
> +                                       const char *addr_file);
> +
>  void bios_linker_loader_cleanup(BIOSLinker *linker);
>  #endif
> 

Re: [Qemu-devel] [PATCH v4 2/9] linker-loader: Add new 'allocate and return address' cmd

[Laszlo Ersek]

On 01/25/17 02:43, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> This adds a new linker-loader command to instruct the guest to allocate
> memory for a fw_cfg file and write the address back into another
> writeable fw_cfg file.  Knowing this address, QEMU can then write into
> guest memory at runtime.
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  hw/acpi/bios-linker-loader.c         | 71 ++++++++++++++++++++++++++++++++++--
>  include/hw/acpi/bios-linker-loader.h |  7 ++++
>  2 files changed, 75 insertions(+), 3 deletions(-)

The OVMF side for this is now tracked by:

https://bugzilla.tianocore.org/show_bug.cgi?id=359

Thanks
Laszlo

[Qemu-devel] [PATCH v4 3/9] docs: VM Generation ID device description

[ben]

From: Ben Warren <ben@skyportsystems.com>

This patch is based off an earlier version by Gal Hammer (ghammer@redhat.com)

Signed-off-by: Gal Hammer <ghammer@redhat.com>
Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 docs/specs/vmgenid.txt | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 docs/specs/vmgenid.txt

diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
new file mode 100644
index 0000000..d36ed5b
--- /dev/null
+++ b/docs/specs/vmgenid.txt
@@ -0,0 +1,40 @@
+VIRTUAL MACHINE GENERATION ID
+=============================
+
+Copyright (C) 2016 Red Hat, Inc.
+Copyright (C) 2017 Skyport Systems, Inc.
+
+This work is licensed under the terms of the GNU GPL, version 2 or later.
+See the COPYING file in the top-level directory.
+
+===
+
+The VM generation ID (vmgenid) device is an emulated device which
+exposes a 128-bit, cryptographically random, integer value identifier.
+This allows management applications (e.g. libvirt) to notify the guest
+operating system when the virtual machine is executed with a different
+configuration (e.g. snapshot execution or creation from a template).
+
+This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709
+
+---
+
+The vmgenid device is a sysbus device with the ACPI ID "QEMUGVID".
+
+The device has one properties, which can be set using the command line
+argument or the QMP interface:
+ guid - sets the value of the GUID.  A special value "auto" instructs
+        QEMU to generate a new random GUID.
+For example:
+QEMU  -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+
+Or to change guid in runtime use:
+ set-vm-generation-id guid=124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87
+ set-vm-generation-id guid=auto
+
+According to the specification, any change to the GUID executes an
+ACPI notification. The vmgenid device triggers the \_GPE._E05 handler
+which executes the ACPI Notify operation.
+
+Although not specified in Microsoft's document, it is assumed that the
+device is expected to use the little-endian system.
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 3/9] docs: VM Generation ID device description

[Laszlo Ersek]

On 01/25/17 02:43, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> This patch is based off an earlier version by Gal Hammer (ghammer@redhat.com)

(1) This commit msg line is too long; please wrap it at 74 chars.

> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  docs/specs/vmgenid.txt | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 docs/specs/vmgenid.txt
> 
> diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
> new file mode 100644
> index 0000000..d36ed5b
> --- /dev/null
> +++ b/docs/specs/vmgenid.txt
> @@ -0,0 +1,40 @@
> +VIRTUAL MACHINE GENERATION ID
> +=============================
> +
> +Copyright (C) 2016 Red Hat, Inc.
> +Copyright (C) 2017 Skyport Systems, Inc.
> +
> +This work is licensed under the terms of the GNU GPL, version 2 or later.
> +See the COPYING file in the top-level directory.
> +
> +===
> +
> +The VM generation ID (vmgenid) device is an emulated device which
> +exposes a 128-bit, cryptographically random, integer value identifier.
> +This allows management applications (e.g. libvirt) to notify the guest
> +operating system when the virtual machine is executed with a different
> +configuration (e.g. snapshot execution or creation from a template).
> +
> +This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709
> +
> +---
> +
> +The vmgenid device is a sysbus device with the ACPI ID "QEMUGVID".

(2) The ID has a typo, it should be QEMUVGID (see the next patch).

> +
> +The device has one properties,

(3) "property", singular

> which can be set using the command line
> +argument or the QMP interface:
> + guid - sets the value of the GUID.  A special value "auto" instructs
> +        QEMU to generate a new random GUID.
> +For example:
> +QEMU  -device vmgenid,guid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> +
> +Or to change guid in runtime use:
> + set-vm-generation-id guid=124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87
> + set-vm-generation-id guid=auto
> +
> +According to the specification, any change to the GUID executes an
> +ACPI notification. The vmgenid device triggers the \_GPE._E05 handler
> +which executes the ACPI Notify operation.
> +
> +Although not specified in Microsoft's document, it is assumed that the
> +device is expected to use the little-endian system.
> 

The above seems okay to me, but it's too terse. Please add an ASCII
diagram (or document in plain text) that describes what object in what
ACPI table points where, what fw_cfg files are used, and so on.
Practically, the entire idea behind patch #4.

I'll try to write more about this in response to patch #4. The goal is
to help QEMU developers understand patch #4 better.

... I have something like this in mind:

https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg03409.html

(4) I think you could simply steal the "Requirements" section (just
mention in the commit message that that part was originally written up
by me). I think this should be helpful, because we can more easily
validate the implementation against such a bullet list.

(5) For the rest of the document, I suggest a quick skim. Quite a good
chunk will not apply to your implementation (justifiedly!), but it will
explain to you how OVMF behaves in this regard. Plus, it should provide
an example or two for ASCII diagrams.

(If you are interested why that patch series was abandoned ultimately:
it's because Microsoft's ACPI interpreter doesn't support the
DataTableRegion operator, as we found out.
<https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg03406.html>.)

(6) With regard to OVMF, I'd like to emphasize the "OVMF SDT Header
probe suppressor". (SDT stands for ACPI System Description Table.)

In your implementation, it should be a 40-byte zero-padding at the front
of the fw_cfg blob with the GUID. It is necessary due to how OVMF
processes the ADD_POINTER commands (explained in the linked document).

The gist of it is that OVMF will look for an ACPI table header wherever
an ADD_POINTER command points, therefore 36 bytes -- see the size of
ACPI_TABLE_HEADER_DEF -- should be padded out first, to suppress that
heuristics for the vmgenid GUID. Then 4 more bytes are needed to round
up the start address of the GUID to a multiple of 8.

(7) Another comment related to the fw_cfg file that contains the GUID:
If you check requirement R1e, it follows that the fw_cfg file hosting
the GUID should be placed at a 4KB boundary, and cover a full page.

Therefore the fw_cfg file structure should be like:

- 36 bytes zero padding for suppressing OVMF's SDT Header probe
- 4 bytes zero padding for getting to an 8-byte alignment
- 8 bytes vmgenid GUID
- 4048 bytes zero padding, to ensure that nothing else is allocated on
the same page, either by the firmware or the OS.

The above layout has a consequence for both the ACPI payload you
generate, and for how QEMU acts upon the address written by the
firmware. Namely, the allocation address returned by the firmware will
point to the beginning of the buffer, but both the ADDR method and QEMU
will have to reference the GUID field at offset 40 decimal.

For this, when QEMU writes the new GUID, it will have to add 40 to the
address returned by the firmware.

Plus, the ADDR AML method will have to add 40 to the VGIA field manually.

Illustration (feel free to include it in the document):

+----------------------------------+
| SSDT with OEM Table ID = VMGENID |
+----------------------------------+
| ...                              |       TOP OF PAGE
| VGIA qword object ---------------|-----> +---------------------------+
| ...                              |       | fw-allocated array for    |
| _STA method referring to VGIA    |       | "etc/vmgenid"             |
| ...                              |       +---------------------------+
| ADDR method referring to VGIA    |       |  0: OVMF SDT Header probe |
| ...                              |       |     suppressor            |
+----------------------------------+       | 36: padding for 8-byte    |
                                           |     alignment             |
                                           | 40: GUID                  |
                                           | 48: padding to page size  |
                                           +---------------------------+
                                           END OF PAGE

When you patch the VGIA object with an ADD_POINTER command, it must
point to the OVMF SDT Header probe suppressor, at offset 0. It is fine
if the _STA method compares VGIA against plain zero, to see in OSPM if
VGIA was indeed patched by the firmware.

However, the ADDR method must add 40 (offset of GUID) to VGIA.
Similarly, QEMU must add 40 to the value received in "etc/vmgenid_addr".

(8) I also suggest including a decompiled dump of the new ACPI payload.
It *really* helps understanding patch #4, whereas currently I
practically have to reverse-engineer patch #4.

Again, I'll try to associate all of these notes with actual code in
patch #4. Still, the documentation should explain them separately.

Thanks
Laszlo

[Qemu-devel] [PATCH v4 4/9] ACPI: Add Virtual Machine Generation ID support

[ben]

From: Ben Warren <ben@skyportsystems.com>

This implements the VM Generation ID feature by passing a 128-bit
GUID to the guest via a fw_cfg blob.
Any time the GUID changes, and ACPI notify event is sent to the guest

The user interface is a simple device with one parameter:
 - guid (string, must be "auto" or in UUID format
   xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 default-configs/i386-softmmu.mak     |   1 +
 default-configs/x86_64-softmmu.mak   |   1 +
 hw/acpi/Makefile.objs                |   1 +
 hw/acpi/vmgenid.c                    | 244 +++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c                 |   9 ++
 include/hw/acpi/acpi_dev_interface.h |   1 +
 include/hw/acpi/vmgenid.h            |  32 +++++
 7 files changed, 289 insertions(+)
 create mode 100644 hw/acpi/vmgenid.c
 create mode 100644 include/hw/acpi/vmgenid.h

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 0b51360..b2bccf6 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -56,3 +56,4 @@ CONFIG_IOH3420=y
 CONFIG_I82801B11=y
 CONFIG_SMBIOS=y
 CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
+CONFIG_ACPI_VMGENID=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 7f89503..c6bd310 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -56,3 +56,4 @@ CONFIG_IOH3420=y
 CONFIG_I82801B11=y
 CONFIG_SMBIOS=y
 CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
+CONFIG_ACPI_VMGENID=y
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 6acf798..11c35bc 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
 common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
 common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
 common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
+common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
 common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
 
 common-obj-y += acpi_interface.o
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
new file mode 100644
index 0000000..63cb039
--- /dev/null
+++ b/hw/acpi/vmgenid.c
@@ -0,0 +1,244 @@
+/*
+ *  Virtual Machine Generation ID Device
+ *
+ *  Copyright (C) 2017 Skyport Systems.
+ *
+ *  Author: Ben Warren <ben@skyportsystems.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qmp-commands.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/vmgenid.h"
+#include "hw/nvram/fw_cfg.h"
+#include "sysemu/sysemu.h"
+
+Object *find_vmgenid_dev(Error **errp)
+{
+    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
+    if (!obj) {
+        error_setg(errp, VMGENID_DEVICE " is not found");
+    }
+    return obj;
+}
+
+void vmgenid_build_acpi(GArray *table_offsets, GArray *table_data,
+         BIOSLinker *linker)
+{
+    Object *obj;
+    VmGenIdState *s;
+    GArray *guid, *vgia;
+    Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
+    uint32_t vgia_offset;
+
+    obj = find_vmgenid_dev(NULL);
+    if (!obj) {
+        return;
+    }
+    s = VMGENID(obj);
+
+    acpi_add_table(table_offsets, table_data);
+
+    guid = g_array_new(false, true, sizeof(s->guid.data));
+    g_array_append_val(guid, s->guid.data);
+    vgia = g_array_new(false, true, sizeof(uint64_t));
+    g_array_append_val(vgia, s->vgia);
+
+    /* Put this in a separate SSDT table */
+    ssdt = init_aml_allocator();
+
+    /* Reserve space for header */
+    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
+
+    /* Storage for the GUID address */
+    vgia_offset = table_data->len +
+        build_append_named_qword(ssdt->buf, "VGIA");
+    scope = aml_scope("\\_SB");
+    dev = aml_device("VGEN");
+    aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID")));
+    aml_append(dev, aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
+    aml_append(dev, aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
+
+    /* Simple status method to check that address is linked and non-zero */
+    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
+    addr = aml_local(0);
+    aml_append(method, aml_store(aml_int(0xf), addr));
+    if_ctx = aml_if(aml_equal(aml_name("VGIA"), aml_int(0)));
+    aml_append(if_ctx, aml_store(aml_int(0), addr));
+    aml_append(method, if_ctx);
+    aml_append(method, aml_return(addr));
+    aml_append(dev, method);
+
+    /* the ADDR method returns two 32-bit words representing the lower and
+     * upper halves * of the physical address of the fw_cfg blob
+     * (holding the GUID) */
+    method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
+
+    addr = aml_local(0);
+    aml_append(method, aml_store(aml_package(2), addr));
+
+    aml_append(method, aml_store(aml_and(aml_name("VGIA"),
+        aml_int(0xffffffff), NULL), aml_index(addr, aml_int(0))));
+    aml_append(method, aml_store(aml_shiftright(aml_name("VGIA"),
+        aml_int(32), NULL), aml_index(addr, aml_int(1))));
+    aml_append(method, aml_return(addr));
+
+    aml_append(dev, method);
+    aml_append(scope, dev);
+    aml_append(ssdt, scope);
+
+    /* attach an ACPI notify */
+    method = aml_method("\\_GPE._E05", 0, AML_NOTSERIALIZED);
+    aml_append(method, aml_notify(aml_name("\\_SB.VGEN"), aml_int(0x80)));
+    aml_append(ssdt, method);
+
+    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
+
+    /* Allocate guest memory for the Address fw_cfg blob */
+    bios_linker_loader_alloc(linker, VMGENID_ADDR_FW_CFG_FILE, vgia, 0,
+                             false /* high memory */);
+    /* Allocate guest memory for the GUID fw_cfg blob and return address */
+    bios_linker_loader_alloc_ret_addr(linker, VMGENID_GUID_FW_CFG_FILE, guid,
+                                      0, false, VMGENID_ADDR_FW_CFG_FILE);
+    /* Patch address of GUID fw_cfg blob into the AML */
+    bios_linker_loader_add_pointer(linker,
+        ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint64_t),
+        VMGENID_GUID_FW_CFG_FILE, 0);
+
+    build_header(linker, table_data,
+        (void *)(table_data->data + table_data->len - ssdt->buf->len),
+        "SSDT", ssdt->buf->len, 1, NULL, "VMGENID");
+    free_aml_allocator();
+}
+
+void vmgenid_add_fw_cfg(FWCfgState *s)
+{
+    Object *obj = find_vmgenid_dev(NULL);
+    if (!obj) {
+        return;
+    }
+    VmGenIdState *vms = VMGENID(obj);
+    /* Create a read-only fw_cfg file for GUID */
+    fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, vms->guid.data,
+        sizeof(vms->guid.data));
+    /* Create a read-write fw_cfg file for Address */
+    fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL,
+                             &vms->vgia, sizeof(uint64_t), false);
+}
+
+static void vmgenid_update_guest(VmGenIdState *s)
+{
+    Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
+    if (obj) {
+
+        /* Write the GUID to guest memory */
+        if (s->vgia) {
+            cpu_physical_memory_write(s->vgia, s->guid.data,
+                                      sizeof(s->guid.data));
+        }
+        /* Send _GPE.E05 event */
+        acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS);
+    }
+}
+
+static void vmgenid_set_guid(Object *obj, const char *value, Error **errp)
+{
+    VmGenIdState *s = VMGENID(obj);
+
+    if (!strncmp(value, "auto", 4)) {
+        qemu_uuid_generate(&s->guid);
+    } else if (qemu_uuid_parse(value, &s->guid) < 0) {
+        error_setg(errp, "'%s." VMGENID_GUID
+                   "': Failed to parse GUID string: %s",
+                   object_get_typename(OBJECT(s)),
+                   value);
+        return;
+    }
+    /* Send the ACPI notify */
+    vmgenid_update_guest(s);
+}
+
+/* After restoring an image, we need to update the guest memory and notify
+ * it of a potential change to VM Generation ID */
+static int vmgenid_post_load(void *opaque, int version_id)
+{
+    VmGenIdState *s = opaque;
+    vmgenid_update_guest(s);
+    return 0;
+}
+
+/* Store the VM Generation ID guest address as part of state info
+ * This is necessary because BIOS will not run when a VM is restored
+ * and so will not tell QEMU the guest address */
+static const VMStateDescription vmstate_vmgenid = {
+    .name = "vmgenid",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = vmgenid_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(vgia, VmGenIdState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void vmgenid_initfn(Object *obj)
+{
+    VmGenIdState *s = VMGENID(obj);
+
+    object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL);
+    s->vgia = 0;
+}
+
+static void vmgenid_device_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_vmgenid;
+}
+
+static const TypeInfo vmgenid_device_info = {
+    .name          = VMGENID_DEVICE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(VmGenIdState),
+    .instance_init = vmgenid_initfn,
+    .class_init    = vmgenid_device_class_init,
+};
+
+static void vmgenid_register_types(void)
+{
+    type_register_static(&vmgenid_device_info);
+}
+
+type_init(vmgenid_register_types)
+
+GuidInfo *qmp_query_vm_generation_id(Error **errp)
+{
+    GuidInfo *info;
+    VmGenIdState *vdev;
+    Object *obj = find_vmgenid_dev(errp);
+
+    if (!obj) {
+        return NULL;
+    }
+    vdev = VMGENID(obj);
+    info = g_malloc0(sizeof(*info));
+    info->guid = qemu_uuid_unparse_strdup(&vdev->guid);
+    return info;
+}
+
+void qmp_set_vm_generation_id(const char *guid, Error **errp)
+{
+    Object *obj = find_vmgenid_dev(errp);
+
+    if (!obj) {
+        return;
+    }
+
+    object_property_set_str(obj, guid, VMGENID_GUID, errp);
+    return;
+}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1c928ab..78b4da5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -42,6 +42,7 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
+#include "hw/acpi/vmgenid.h"
 #include "sysemu/tpm_backend.h"
 #include "hw/timer/mc146818rtc_regs.h"
 #include "sysemu/numa.h"
@@ -2653,6 +2654,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, pcms);
 
+    if (has_vmgenid()) {
+        vmgenid_build_acpi(table_offsets, tables_blob, tables->linker);
+    }
+
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
         build_hpet(tables_blob, tables->linker);
@@ -2859,6 +2864,10 @@ void acpi_setup(void)
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
 
+    if (has_vmgenid()) {
+        vmgenid_add_fw_cfg(pcms->fw_cfg);
+    }
+
     if (!pcmc->rsdp_in_ram) {
         /*
          * Keep for compatibility with old machine types.
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 71d3c48..3c2e4e9 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -11,6 +11,7 @@ typedef enum {
     ACPI_CPU_HOTPLUG_STATUS = 4,
     ACPI_MEMORY_HOTPLUG_STATUS = 8,
     ACPI_NVDIMM_HOTPLUG_STATUS = 16,
+    ACPI_VMGENID_CHANGE_STATUS = 32,
 } AcpiEventStatusBits;
 
 #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
new file mode 100644
index 0000000..e4ac6f8
--- /dev/null
+++ b/include/hw/acpi/vmgenid.h
@@ -0,0 +1,32 @@
+#ifndef ACPI_VMGENID_H
+#define ACPI_VMGENID_H
+
+#include "hw/acpi/bios-linker-loader.h"
+#include "hw/sysbus.h"
+#include "qemu/uuid.h"
+
+#define VMGENID_DEVICE           "vmgenid"
+#define VMGENID_GUID             "guid"
+#define VMGENID_GUID_FW_CFG_FILE      "etc/vmgenid"
+#define VMGENID_ADDR_FW_CFG_FILE      "etc/vmgenid_addr"
+
+Object *find_vmgenid_dev(Error **errp);
+void vmgenid_add_fw_cfg(FWCfgState *s);
+void vmgenid_build_acpi(GArray *table_offsets, GArray *table_data,
+                       BIOSLinker *linker);
+
+#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
+
+typedef struct VmGenIdState {
+    SysBusDevice parent_obj;
+    QemuUUID guid;
+    uint64_t vgia;
+
+} VmGenIdState;
+
+static inline bool has_vmgenid(void)
+{
+    return object_resolve_path_type("", VMGENID_DEVICE, NULL) != NULL;
+}
+
+#endif
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 4/9] ACPI: Add Virtual Machine Generation ID support

[Laszlo Ersek]

On 01/25/17 02:43, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> This implements the VM Generation ID feature by passing a 128-bit
> GUID to the guest via a fw_cfg blob.
> Any time the GUID changes, and ACPI notify event is sent to the guest

(1) typo: "and" -> "an"

> 
> The user interface is a simple device with one parameter:
>  - guid (string, must be "auto" or in UUID format
>    xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  default-configs/i386-softmmu.mak     |   1 +
>  default-configs/x86_64-softmmu.mak   |   1 +
>  hw/acpi/Makefile.objs                |   1 +
>  hw/acpi/vmgenid.c                    | 244 +++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c                 |   9 ++
>  include/hw/acpi/acpi_dev_interface.h |   1 +
>  include/hw/acpi/vmgenid.h            |  32 +++++
>  7 files changed, 289 insertions(+)
>  create mode 100644 hw/acpi/vmgenid.c
>  create mode 100644 include/hw/acpi/vmgenid.h
> 
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 0b51360..b2bccf6 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -56,3 +56,4 @@ CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
>  CONFIG_SMBIOS=y
>  CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> +CONFIG_ACPI_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 7f89503..c6bd310 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -56,3 +56,4 @@ CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
>  CONFIG_SMBIOS=y
>  CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> +CONFIG_ACPI_VMGENID=y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 6acf798..11c35bc 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
>  common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>  common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>  common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
> +common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>  common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>  
>  common-obj-y += acpi_interface.o
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> new file mode 100644
> index 0000000..63cb039
> --- /dev/null
> +++ b/hw/acpi/vmgenid.c
> @@ -0,0 +1,244 @@
> +/*
> + *  Virtual Machine Generation ID Device
> + *
> + *  Copyright (C) 2017 Skyport Systems.
> + *
> + *  Author: Ben Warren <ben@skyportsystems.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qmp-commands.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/vmgenid.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "sysemu/sysemu.h"
> +
> +Object *find_vmgenid_dev(Error **errp)
> +{
> +    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> +    if (!obj) {
> +        error_setg(errp, VMGENID_DEVICE " is not found");
> +    }

(2) For general cleanliness, we should use "%s" in the format string,
and pass VMGENID_DEVICE as an argument.

> +    return obj;
> +}
> +
> +void vmgenid_build_acpi(GArray *table_offsets, GArray *table_data,
> +         BIOSLinker *linker)

(3) I think the "table_offsets" parameter should be dropped.

In this function, that param is used only for calling acpi_add_table().
Looking at other similar call sites in acpi_build(), the pattern is always:

  if (check_feature_or_device()) {
    acpi_add_table(table_offsets, table_data);
    build_stuff_for_feature_or_device();
  }

That is, the parameter should be dropped, and the acpi_add_table() call
should be moved out to acpi_build().

> +{
> +    Object *obj;
> +    VmGenIdState *s;
> +    GArray *guid, *vgia;
> +    Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
> +    uint32_t vgia_offset;
> +
> +    obj = find_vmgenid_dev(NULL);
> +    if (!obj) {
> +        return;
> +    }
> +    s = VMGENID(obj);

(4) The call to this function, from acpi_build(), is already protected
by has_vmgenid(). (And, see my suggestion near the end of this email
about rebasing has_vmgenid() to find_vmgenid_dev().) Therefore we should
simply assert that find_vmgenid_dev() succeeds.

> +
> +    acpi_add_table(table_offsets, table_data);

(So this should be moved to acpi_build(), see (3) above.)

> +
> +    guid = g_array_new(false, true, sizeof(s->guid.data));
> +    g_array_append_val(guid, s->guid.data);

(5) We're not considering host vs. guest endianness.
Let me quote the VmGenIdState structure:

typedef struct VmGenIdState {
    SysBusDevice parent_obj;
    QemuUUID guid;
    uint64_t vgia;
} VmGenIdState;

According to the documentation of QemuUUID, and the qemu_uuid_parse()
and qemu_uuid_unparse() functions -- used later in this patch --, the
representation of the UUID fields is big-endian in the QemuUUID structure.

However, the Windows guest will expect the UUID fields in little-endian
representation.

(This is mentioned in the previous docs patch, and also this is general
practice for Microsoft; see for example the SMBIOS 3.0 specification,
section "7.2.1 System — UUID":

    Although RFC4122 recommends network byte order for all fields, the
    PC industry (including the ACPI, UEFI, and Microsoft
    specifications) has consistently used little-endian byte encoding
    for the first three fields: time_low, time_mid,
    time_hi_and_version. [...]
)

Therefore I think we should byte-swap the fields with qemu_uuid_bswap()
first.

(6) As discussed under the previous patch, the blob constructed for the
"etc/vmgenid" file should consist of 40 bytes of zero padding, then the
(little-endian) GUID, then more padding, up to the page size (4KB).

(The byte swap for the GUID should occur only in the blob, not in the
VmGenIdState.guid field.)

(7) The blob constructed in this function, as a GArray, should be the
exact same object that is later linked into fw_cfg, via acpi_setup() -->
vmgenid_add_fw_cfg().

Currently, the blob is allocated here under the variable "guid", and
passed to bios_linker_loader_alloc_ret_addr(). That results in the
creation of a new BiosLinkerFileEntry object, with the "blob" field
being set to "guid".

However, in vmgenid_add_fw_cfg(), the VmGenIdState.guid.data field is
linked into fw_cfg. This is incorrect, those objects are independent,
but they should be the same.

Here's how to implement it:

* Add the field

    GArray *vmgenid

  to the "AcpiBuildTables" structure in "include/hw/acpi/aml-build.h",
  under the "tcpalog" field.

* Extend the acpi_build_tables_init() and acpi_build_tables_cleanup()
  functions in "hw/acpi/aml-build.c", so that the new field is
  initialized and released.

* In the acpi_build() function, pass "tables->vmgenid" to
  vmgenid_build_acpi(). This will require the a new parameter for the
  latter function.

* In vmgenid_build_acpi(), construct the blob as described under (5)
  and (6).

* In the acpi_setup() function, pass "tables.vmgenid" to
  vmgenid_add_fw_cfg(). (Again, new function parameter is necessary.)

* In vmgenid_add_fw_cfg(), link "tables.vmgenid->data" into fw_cfg, not
  VmGenIdState.guid.data.

> +    vgia = g_array_new(false, true, sizeof(uint64_t));
> +    g_array_append_val(vgia, s->vgia);

(8) This is unnecessary.

As I mentioned earlier, under patch #2, we need not and should not
instruct the firmware to allocate room for, and download, the fw_cfg
file that is going to receive the allocation address. So this should be
simply dropped (including the "vgia" local variable).

I shall comment more on the "VmGenIdState.vgia" field, just a bit lower
down.

> +
> +    /* Put this in a separate SSDT table */
> +    ssdt = init_aml_allocator();
> +
> +    /* Reserve space for header */
> +    acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> +
> +    /* Storage for the GUID address */
> +    vgia_offset = table_data->len +
> +        build_append_named_qword(ssdt->buf, "VGIA");
> +    scope = aml_scope("\\_SB");
> +    dev = aml_device("VGEN");
> +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID")));
> +    aml_append(dev, aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
> +    aml_append(dev, aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
> +
> +    /* Simple status method to check that address is linked and non-zero */
> +    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> +    addr = aml_local(0);
> +    aml_append(method, aml_store(aml_int(0xf), addr));
> +    if_ctx = aml_if(aml_equal(aml_name("VGIA"), aml_int(0)));
> +    aml_append(if_ctx, aml_store(aml_int(0), addr));
> +    aml_append(method, if_ctx);
> +    aml_append(method, aml_return(addr));
> +    aml_append(dev, method);
> +
> +    /* the ADDR method returns two 32-bit words representing the lower and
> +     * upper halves * of the physical address of the fw_cfg blob
> +     * (holding the GUID) */
> +    method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
> +
> +    addr = aml_local(0);
> +    aml_append(method, aml_store(aml_package(2), addr));
> +
> +    aml_append(method, aml_store(aml_and(aml_name("VGIA"),
> +        aml_int(0xffffffff), NULL), aml_index(addr, aml_int(0))));
> +    aml_append(method, aml_store(aml_shiftright(aml_name("VGIA"),
> +        aml_int(32), NULL), aml_index(addr, aml_int(1))));
> +    aml_append(method, aml_return(addr));

(9) This is the part (the ADDR method) where you should please add 40
decimal to the contents of the VGIA field, and return that value.

> +
> +    aml_append(dev, method);
> +    aml_append(scope, dev);
> +    aml_append(ssdt, scope);
> +
> +    /* attach an ACPI notify */
> +    method = aml_method("\\_GPE._E05", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_notify(aml_name("\\_SB.VGEN"), aml_int(0x80)));
> +    aml_append(ssdt, method);
> +
> +    g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> +
> +    /* Allocate guest memory for the Address fw_cfg blob */
> +    bios_linker_loader_alloc(linker, VMGENID_ADDR_FW_CFG_FILE, vgia, 0,
> +                             false /* high memory */);

(10) this should be dropped, in accordance with (8).

> +    /* Allocate guest memory for the GUID fw_cfg blob and return address */
> +    bios_linker_loader_alloc_ret_addr(linker, VMGENID_GUID_FW_CFG_FILE, guid,
> +                                      0, false, VMGENID_ADDR_FW_CFG_FILE);

(11) The arguments are not entirely correct; the ones to update are:

- "guid" should be replaced by the new parameter of vmgenid_build_acpi()
that stands for "AcpiBuildTables.vmgenid" and whose contents you
constuct here. See (7).

- 0 (for "alloc_align") should be replaced with 4096, because we want
the blob to be page aligned in the guest.

> +    /* Patch address of GUID fw_cfg blob into the AML */
> +    bios_linker_loader_add_pointer(linker,
> +        ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint64_t),
> +        VMGENID_GUID_FW_CFG_FILE, 0);

Yes, this is correct!

> +
> +    build_header(linker, table_data,
> +        (void *)(table_data->data + table_data->len - ssdt->buf->len),
> +        "SSDT", ssdt->buf->len, 1, NULL, "VMGENID");
> +    free_aml_allocator();
> +}

Looks fine to me.

> +
> +void vmgenid_add_fw_cfg(FWCfgState *s)
> +{
> +    Object *obj = find_vmgenid_dev(NULL);
> +    if (!obj) {
> +        return;
> +    }

(12) The call to this function is protected by has_vmgenid(), so we
should just assert success here.

> +    VmGenIdState *vms = VMGENID(obj);
> +    /* Create a read-only fw_cfg file for GUID */
> +    fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, vms->guid.data,
> +        sizeof(vms->guid.data));

(13) According to (7) and (11), the data linked into fw_cfg should be
"tables.vmgenid->data". (From a new parameter of this function.)

> +    /* Create a read-write fw_cfg file for Address */
> +    fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL,
> +                             &vms->vgia, sizeof(uint64_t), false);
> +}

(14) Okay, so this is where I comment on the "VmGenIdState.vgia".

The address written by the guest is not approprite to represent as a
uint64_t field in "VmGenIdState". (The same applies to "vmstate_vmgenid"
/ VMSTATE_UINT64() lower.)

The reason is that the guest will always write the address in
little-endian order, but the host can be little endian and big endian
alike. Given that you need to work with this address value as a numeric
quantity in QEMU, you should explicitly convert from guest endian(LE) to
host endian (whatever that is).

IOW,

- the "VmGenIdState.vgia" field should be defined as:

    uint8_t vgia_le[8];

- linked as such into fw_cfg,

- migrated as such (because migration between little-endian and
  big-endian hosts keeps the *host value*, doing any necessary byte
  swaps automatically, and that, with the current representation, would
  appear to the guest, if it read back the contents of the address
  fw_cfg file),

- and converted explicitly to host endian when dereferenced.

I'll point out the last two items more precisely below.

> +
> +static void vmgenid_update_guest(VmGenIdState *s)
> +{
> +    Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
> +    if (obj) {
> +
> +        /* Write the GUID to guest memory */
> +        if (s->vgia) {
> +            cpu_physical_memory_write(s->vgia, s->guid.data,
> +                                      sizeof(s->guid.data));
> +        }

(15) So here you need to copy the "s->vgia_le" array into a local
uint64_t variable, and convert it from LE to host endian with the
le64_to_cpus() function.

If the result compares unequal to zero, then you should add 40 decimal
-- see the blob's structure before --, and write the current GUID to
*that* guest physical address.

NB, the data to write (i.e., s->guid) should be treated with
qemu_uuid_bswap() for the guest first, because on the host side, it is
guaranteed BE, but the guest needs LE. Again, see (5) above.


> +        /* Send _GPE.E05 event */
> +        acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS);

(16) I think this should also depend on vgia_le being nonzero!

> +    }
> +}
> +
> +static void vmgenid_set_guid(Object *obj, const char *value, Error **errp)
> +{
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    if (!strncmp(value, "auto", 4)) {
> +        qemu_uuid_generate(&s->guid);
> +    } else if (qemu_uuid_parse(value, &s->guid) < 0) {
> +        error_setg(errp, "'%s." VMGENID_GUID
> +                   "': Failed to parse GUID string: %s",
> +                   object_get_typename(OBJECT(s)),
> +                   value);

(17) Please don't embed VMGENID_GUID in the format string, use %s and an
argument instead.

> +        return;
> +    }
> +    /* Send the ACPI notify */
> +    vmgenid_update_guest(s);
> +}
> +
> +/* After restoring an image, we need to update the guest memory and notify
> + * it of a potential change to VM Generation ID */
> +static int vmgenid_post_load(void *opaque, int version_id)
> +{
> +    VmGenIdState *s = opaque;
> +    vmgenid_update_guest(s);
> +    return 0;
> +}

Hm.... Okay, I think this should be fine; on the target host (or when
the vmstate is reloaded from a file / snapshot), it is the management
layer's responsibility to set a new GUID on the command line.

So here we don't have to generate a new GUID. I believe the following
happens:

- Libvirt starts QEMU on the target host, with a new GUID (auto, or
specific). vmgenid_set_guid() gets called due to the property being set,
which either generates a random GUID, or parses it from the command line.

- vmgenid_set_guid() calls vmgenid_update_guest(). Given that at this
point the vgia_le field is zero, nothing is written to guest memory.
And, according to my remark (16), the _GPE.E05 event is also not raised
(which would otherwise consist of setting bit5 in the STS register, and
injecting an SCI).

- the incoming migration stream is processed. The "vgia_le" field is
loaded from the source host, but the "guid" field preserves the value
parsed from the command line on the target host (or the value generated
on the target host)

- vmgenid_post_load() is called. Given that "vgia_le" is no longer zero,
the GUID parsed (or generated) on the host side is written into guest
memory.

Seems sane to me.

> +
> +/* Store the VM Generation ID guest address as part of state info
> + * This is necessary because BIOS will not run when a VM is restored
> + * and so will not tell QEMU the guest address */

(18) I propose to drop this comment.

The more general explanation (which covers all vmstate descriptions) is
that all state that is under the guest's influence must be migrated. In
our case, the address is under guest influence.

> +static const VMStateDescription vmstate_vmgenid = {
> +    .name = "vmgenid",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = vmgenid_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(vgia, VmGenIdState),

(19) Referring back to (14), this should be

  VMSTATE_UINT8_ARRAY(vgia_le, VmGenIdState, sizeof(uint64_t))

> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static void vmgenid_initfn(Object *obj)
> +{
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    object_property_add_str(obj, VMGENID_GUID, NULL, vmgenid_set_guid, NULL);
> +    s->vgia = 0;

(20) The zero assignment should not be necessary, the full object is
zeroed when allocated.

> +}
> +
> +static void vmgenid_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_vmgenid;
> +}
> +
> +static const TypeInfo vmgenid_device_info = {
> +    .name          = VMGENID_DEVICE,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(VmGenIdState),
> +    .instance_init = vmgenid_initfn,
> +    .class_init    = vmgenid_device_class_init,
> +};
> +
> +static void vmgenid_register_types(void)
> +{
> +    type_register_static(&vmgenid_device_info);
> +}
> +
> +type_init(vmgenid_register_types)
> +
> +GuidInfo *qmp_query_vm_generation_id(Error **errp)
> +{
> +    GuidInfo *info;
> +    VmGenIdState *vdev;
> +    Object *obj = find_vmgenid_dev(errp);
> +
> +    if (!obj) {
> +        return NULL;
> +    }
> +    vdev = VMGENID(obj);
> +    info = g_malloc0(sizeof(*info));
> +    info->guid = qemu_uuid_unparse_strdup(&vdev->guid);
> +    return info;
> +}
> +
> +void qmp_set_vm_generation_id(const char *guid, Error **errp)
> +{
> +    Object *obj = find_vmgenid_dev(errp);
> +
> +    if (!obj) {
> +        return;
> +    }
> +
> +    object_property_set_str(obj, guid, VMGENID_GUID, errp);
> +    return;
> +}

(21) These two QMP functions are probably fine, but IMO they don't
belong to this patch. They should go into patch #5 ("qmp/hmp: add
query-vm-generation-id and 'info vm-generation-id' commands").

What's more, given that GuidInfo is generated from "qapi-schema.json",
and that file is extended only in the next patch, I believe if you check
out the tree at this stage exactly, it won't build.

I think you may have squashed these functions into the wrong patch
during a git-rebase.

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1c928ab..78b4da5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -42,6 +42,7 @@
>  #include "hw/acpi/memory_hotplug.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
> +#include "hw/acpi/vmgenid.h"
>  #include "sysemu/tpm_backend.h"
>  #include "hw/timer/mc146818rtc_regs.h"
>  #include "sysemu/numa.h"
> @@ -2653,6 +2654,10 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>      acpi_add_table(table_offsets, tables_blob);
>      build_madt(tables_blob, tables->linker, pcms);
>  
> +    if (has_vmgenid()) {
> +        vmgenid_build_acpi(table_offsets, tables_blob, tables->linker);
> +    }
> +
>      if (misc.has_hpet) {
>          acpi_add_table(table_offsets, tables_blob);
>          build_hpet(tables_blob, tables->linker);
> @@ -2859,6 +2864,10 @@ void acpi_setup(void)
>      fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
>                      tables.tcpalog->data, acpi_data_len(tables.tcpalog));
>  
> +    if (has_vmgenid()) {
> +        vmgenid_add_fw_cfg(pcms->fw_cfg);
> +    }
> +
>      if (!pcmc->rsdp_in_ram) {
>          /*
>           * Keep for compatibility with old machine types.

The necessary changes for these hunks follow from the above points.

> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index 71d3c48..3c2e4e9 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -11,6 +11,7 @@ typedef enum {
>      ACPI_CPU_HOTPLUG_STATUS = 4,
>      ACPI_MEMORY_HOTPLUG_STATUS = 8,
>      ACPI_NVDIMM_HOTPLUG_STATUS = 16,
> +    ACPI_VMGENID_CHANGE_STATUS = 32,
>  } AcpiEventStatusBits;
>  
>  #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
> new file mode 100644
> index 0000000..e4ac6f8
> --- /dev/null
> +++ b/include/hw/acpi/vmgenid.h
> @@ -0,0 +1,32 @@
> +#ifndef ACPI_VMGENID_H
> +#define ACPI_VMGENID_H
> +
> +#include "hw/acpi/bios-linker-loader.h"
> +#include "hw/sysbus.h"
> +#include "qemu/uuid.h"
> +
> +#define VMGENID_DEVICE           "vmgenid"
> +#define VMGENID_GUID             "guid"
> +#define VMGENID_GUID_FW_CFG_FILE      "etc/vmgenid"
> +#define VMGENID_ADDR_FW_CFG_FILE      "etc/vmgenid_addr"
> +
> +Object *find_vmgenid_dev(Error **errp);
> +void vmgenid_add_fw_cfg(FWCfgState *s);
> +void vmgenid_build_acpi(GArray *table_offsets, GArray *table_data,
> +                       BIOSLinker *linker);
> +
> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
> +
> +typedef struct VmGenIdState {
> +    SysBusDevice parent_obj;
> +    QemuUUID guid;
> +    uint64_t vgia;
> +

(22) Unnecessary empty line.

> +} VmGenIdState;
> +
> +static inline bool has_vmgenid(void)
> +{
> +    return object_resolve_path_type("", VMGENID_DEVICE, NULL) != NULL;
> +}
> +
> +#endif
> 

(23) We have two very similar functions here, find_vmgenid_dev() and
has_vmgenid(). I think has_vmgenid() should be rebased to
find_vmgenid_dev(). See also (4) above.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v4 4/9] ACPI: Add Virtual Machine Generation ID support

[Laszlo Ersek]

On 01/25/17 11:04, Laszlo Ersek wrote:

> (7) The blob constructed in this function, as a GArray, should be the
> exact same object that is later linked into fw_cfg, via acpi_setup() -->
> vmgenid_add_fw_cfg().
> 
> Currently, the blob is allocated here under the variable "guid", and
> passed to bios_linker_loader_alloc_ret_addr(). That results in the
> creation of a new BiosLinkerFileEntry object, with the "blob" field
> being set to "guid".
> 
> However, in vmgenid_add_fw_cfg(), the VmGenIdState.guid.data field is
> linked into fw_cfg. This is incorrect, those objects are independent,
> but they should be the same.
> 
> Here's how to implement it:
> 
> * Add the field
> 
>     GArray *vmgenid
> 
>   to the "AcpiBuildTables" structure in "include/hw/acpi/aml-build.h",
>   under the "tcpalog" field.
> 
> * Extend the acpi_build_tables_init() and acpi_build_tables_cleanup()
>   functions in "hw/acpi/aml-build.c", so that the new field is
>   initialized and released.

In acpi_build_tables_cleanup(), the line you need is

    g_array_free(tables->vmgenid, mfre);

similarly to "tcpalog".

> 
> * In the acpi_build() function, pass "tables->vmgenid" to
>   vmgenid_build_acpi(). This will require the a new parameter for the
>   latter function.
> 
> * In vmgenid_build_acpi(), construct the blob as described under (5)
>   and (6).
> 
> * In the acpi_setup() function, pass "tables.vmgenid" to
>   vmgenid_add_fw_cfg(). (Again, new function parameter is necessary.)
> 
> * In vmgenid_add_fw_cfg(), link "tables.vmgenid->data" into fw_cfg, not
>   VmGenIdState.guid.data.

Thanks
Laszlo

[Qemu-devel] [PATCH v4 5/9] qmp/hmp: add query-vm-generation-id and 'info vm-generation-id' commands

[ben]

From: Igor Mammedov <imammedo@redhat.com>

Add commands to query Virtual Machine Generation ID counter.

QMP command example:
    { "execute": "query-vm-generation-id" }

HMP command example:
    info vm-generation-id

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 hmp-commands-info.hx | 13 +++++++++++++
 hmp.c                |  9 +++++++++
 hmp.h                |  1 +
 qapi-schema.json     | 20 ++++++++++++++++++++
 stubs/Makefile.objs  |  1 +
 stubs/vmgenid.c      |  8 ++++++++
 6 files changed, 52 insertions(+)
 create mode 100644 stubs/vmgenid.c

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 55d50c4..b0bb052 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -802,6 +802,19 @@ Show information about hotpluggable CPUs
 ETEXI
 
 STEXI
+@item info vm-generation-id
+Show Virtual Machine Generation ID
+ETEXI
+
+    {
+        .name       = "vm-generation-id",
+        .args_type  = "",
+        .params     = "",
+        .help       = "Show Virtual Machine Generation ID",
+        .cmd = hmp_info_vm_generation_id,
+    },
+
+STEXI
 @end table
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 8522efe..c2280e0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2564,3 +2564,12 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
 
     qapi_free_HotpluggableCPUList(saved);
 }
+
+void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
+{
+    GuidInfo *info = qmp_query_vm_generation_id(NULL);
+    if (info) {
+        monitor_printf(mon, "%s\n", info->guid);
+    }
+    qapi_free_GuidInfo(info);
+}
diff --git a/hmp.h b/hmp.h
index 05daf7c..799fd37 100644
--- a/hmp.h
+++ b/hmp.h
@@ -137,5 +137,6 @@ void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
 void hmp_info_dump(Monitor *mon, const QDict *qdict);
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
+void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index ac55f4a..413ac52 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -6026,3 +6026,23 @@
 #
 ##
 { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
+
+##
+# @GuidInfo:
+#
+# GUID information.
+#
+# @guid: the globally unique identifier
+#
+# Since: 2.9
+##
+{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
+
+##
+# @query-vm-generation-id:
+#
+# Show Virtual Machine Generation ID
+#
+# Since 2.9
+##
+{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index a187295..0bffca6 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -35,3 +35,4 @@ stub-obj-y += qmp_pc_dimm_device_list.o
 stub-obj-y += target-monitor-defs.o
 stub-obj-y += target-get-monitor-def.o
 stub-obj-y += pc_madt_cpu_entry.o
+stub-obj-y += vmgenid.o
diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c
new file mode 100644
index 0000000..8c448ac
--- /dev/null
+++ b/stubs/vmgenid.c
@@ -0,0 +1,8 @@
+#include "qemu/osdep.h"
+#include "qmp-commands.h"
+
+GuidInfo *qmp_query_vm_generation_id(Error **errp)
+{
+    error_setg(errp, "this command is not currently supported");
+    return NULL;
+}
-- 
2.7.4

[Qemu-devel] [PATCH v4 6/9] qmp/hmp: add set-vm-generation-id commands

[ben]

From: Igor Mammedov <imammedo@redhat.com>

Add set-vm-generation-id command to set Virtual Machine
Generation ID counter.

QMP command example:
    { "execute": "set-vm-generation-id",
          "arguments": {
              "guid": "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
          }
    }

HMP command example:
    set-vm-generation-id guid=324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 hmp-commands.hx  | 13 +++++++++++++
 hmp.c            | 12 ++++++++++++
 hmp.h            |  1 +
 qapi-schema.json | 11 +++++++++++
 stubs/vmgenid.c  |  6 ++++++
 5 files changed, 43 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8819281..56744aa 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1775,5 +1775,18 @@ ETEXI
     },
 
 STEXI
+@item set-vm-generation-id @var{uuid}
+Set Virtual Machine Generation ID counter to @var{guid}
+ETEXI
+
+    {
+        .name       = "set-vm-generation-id",
+        .args_type  = "guid:s",
+        .params     = "guid",
+        .help       = "Set Virtual Machine Generation ID counter",
+        .cmd = hmp_set_vm_generation_id,
+    },
+
+STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index c2280e0..3a4db8b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2573,3 +2573,15 @@ void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
     }
     qapi_free_GuidInfo(info);
 }
+
+void hmp_set_vm_generation_id(Monitor *mon, const QDict *qdict)
+{
+    Error *errp = NULL;
+    const char *guid = qdict_get_str(qdict, "guid");
+
+    qmp_set_vm_generation_id(guid, &errp);
+    if (errp) {
+        hmp_handle_error(mon, &errp);
+        return;
+    }
+}
diff --git a/hmp.h b/hmp.h
index 799fd37..e0ac1e8 100644
--- a/hmp.h
+++ b/hmp.h
@@ -138,5 +138,6 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
 void hmp_info_dump(Monitor *mon, const QDict *qdict);
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
 void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
+void hmp_set_vm_generation_id(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 413ac52..e2ed75c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -6046,3 +6046,14 @@
 # Since 2.9
 ##
 { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
+
+##
+# @set-vm-generation-id:
+#
+# Set Virtual Machine Generation ID
+#
+# @guid: new GUID to set as Virtual Machine Generation ID
+#
+# Since 2.9
+##
+{ 'command': 'set-vm-generation-id', 'data': {'guid': 'str'} }
diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c
index 8c448ac..d25d41b 100644
--- a/stubs/vmgenid.c
+++ b/stubs/vmgenid.c
@@ -6,3 +6,9 @@ GuidInfo *qmp_query_vm_generation_id(Error **errp)
     error_setg(errp, "this command is not currently supported");
     return NULL;
 }
+
+void qmp_set_vm_generation_id(const char *guid, Error **errp)
+{
+    error_setg(errp, "this command is not currently supported");
+    return;
+}
-- 
2.7.4

[Qemu-devel] [PATCH v4 7/9] PC: Support dynamic sysbus on pc_i440fx

[ben]

From: Ben Warren <ben@skyportsystems.com>

This allows pc_i440fx-based machines to add new devices such as VM Generation ID
directly to the sysbus.

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 hw/i386/pc_piix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 9f102aa..c8ad99c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -435,6 +435,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->hot_add_cpu = pc_hot_add_cpu;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
+    m->has_dynamic_sysbus = true;
 }
 
 static void pc_i440fx_2_9_machine_options(MachineClass *m)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 7/9] PC: Support dynamic sysbus on pc_i440fx

[Laszlo Ersek]

On 01/25/17 02:43, ben@skyportsystems.com wrote:
> From: Ben Warren <ben@skyportsystems.com>
> 
> This allows pc_i440fx-based machines to add new devices such as VM Generation ID
> directly to the sysbus.
> 
> Signed-off-by: Ben Warren <ben@skyportsystems.com>
> ---
>  hw/i386/pc_piix.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 9f102aa..c8ad99c 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -435,6 +435,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->hot_add_cpu = pc_hot_add_cpu;
>      m->default_machine_opts = "firmware=bios-256k.bin";
>      m->default_display = "std";
> +    m->has_dynamic_sysbus = true;
>  }
>  
>  static void pc_i440fx_2_9_machine_options(MachineClass *m)
> 

Looks reasonable to me, especially that Q35 already sets it.

One nit: please wrap the commit message at 74 chars.

I'll let others review the rest of the patches. (My interest lies in the
ACPI & fw_cfg bits.)

Thanks!
Laszlo

[Qemu-devel] [PATCH v4 8/9] tests: Move reusable ACPI macros into a new header file

[ben]

From: Ben Warren <ben@skyportsystems.com>

Also usable by upcoming VM Generation ID tests

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 tests/acpi-utils.h       | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/bios-tables-test.c | 72 +---------------------------------------------
 2 files changed, 76 insertions(+), 71 deletions(-)
 create mode 100644 tests/acpi-utils.h

diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
new file mode 100644
index 0000000..d5e5eff
--- /dev/null
+++ b/tests/acpi-utils.h
@@ -0,0 +1,75 @@
+#ifndef TEST_ACPI_UTILS_H
+#define TEST_ACPI_UTILS_H
+
+/* DSDT and SSDTs format */
+typedef struct {
+    AcpiTableHeader header;
+    gchar *aml;            /* aml bytecode from guest */
+    gsize aml_len;
+    gchar *aml_file;
+    gchar *asl;            /* asl code generated from aml */
+    gsize asl_len;
+    gchar *asl_file;
+    bool tmp_files_retain;   /* do not delete the temp asl/aml */
+} QEMU_PACKED AcpiSdtTable;
+
+#define ACPI_READ_FIELD(field, addr)           \
+    do {                                       \
+        switch (sizeof(field)) {               \
+        case 1:                                \
+            field = readb(addr);               \
+            break;                             \
+        case 2:                                \
+            field = readw(addr);               \
+            break;                             \
+        case 4:                                \
+            field = readl(addr);               \
+            break;                             \
+        case 8:                                \
+            field = readq(addr);               \
+            break;                             \
+        default:                               \
+            g_assert(false);                   \
+        }                                      \
+        addr += sizeof(field);                  \
+    } while (0);
+
+#define ACPI_READ_ARRAY_PTR(arr, length, addr)  \
+    do {                                        \
+        int idx;                                \
+        for (idx = 0; idx < length; ++idx) {    \
+            ACPI_READ_FIELD(arr[idx], addr);    \
+        }                                       \
+    } while (0);
+
+#define ACPI_READ_ARRAY(arr, addr)                               \
+    ACPI_READ_ARRAY_PTR(arr, sizeof(arr) / sizeof(arr[0]), addr)
+
+#define ACPI_READ_TABLE_HEADER(table, addr)                      \
+    do {                                                         \
+        ACPI_READ_FIELD((table)->signature, addr);               \
+        ACPI_READ_FIELD((table)->length, addr);                  \
+        ACPI_READ_FIELD((table)->revision, addr);                \
+        ACPI_READ_FIELD((table)->checksum, addr);                \
+        ACPI_READ_ARRAY((table)->oem_id, addr);                  \
+        ACPI_READ_ARRAY((table)->oem_table_id, addr);            \
+        ACPI_READ_FIELD((table)->oem_revision, addr);            \
+        ACPI_READ_ARRAY((table)->asl_compiler_id, addr);         \
+        ACPI_READ_FIELD((table)->asl_compiler_revision, addr);   \
+    } while (0);
+
+#define ACPI_ASSERT_CMP(actual, expected) do { \
+    uint32_t ACPI_ASSERT_CMP_le = cpu_to_le32(actual); \
+    char ACPI_ASSERT_CMP_str[5] = {}; \
+    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 4); \
+    g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
+} while (0)
+
+#define ACPI_ASSERT_CMP64(actual, expected) do { \
+    uint64_t ACPI_ASSERT_CMP_le = cpu_to_le64(actual); \
+    char ACPI_ASSERT_CMP_str[9] = {}; \
+    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 8); \
+    g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
+} while (0)
+
+#endif  /* TEST_ACPI_UTILS_H */
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 5404805..c642f7f 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -17,6 +17,7 @@
 #include "hw/acpi/acpi-defs.h"
 #include "hw/smbios/smbios.h"
 #include "qemu/bitmap.h"
+#include "acpi-utils.h"
 #include "boot-sector.h"
 
 #define MACHINE_PC "pc"
@@ -24,18 +25,6 @@
 
 #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
 
-/* DSDT and SSDTs format */
-typedef struct {
-    AcpiTableHeader header;
-    gchar *aml;            /* aml bytecode from guest */
-    gsize aml_len;
-    gchar *aml_file;
-    gchar *asl;            /* asl code generated from aml */
-    gsize asl_len;
-    gchar *asl_file;
-    bool tmp_files_retain;   /* do not delete the temp asl/aml */
-} QEMU_PACKED AcpiSdtTable;
-
 typedef struct {
     const char *machine;
     const char *variant;
@@ -53,65 +42,6 @@ typedef struct {
     int required_struct_types_len;
 } test_data;
 
-#define ACPI_READ_FIELD(field, addr)           \
-    do {                                       \
-        switch (sizeof(field)) {               \
-        case 1:                                \
-            field = readb(addr);               \
-            break;                             \
-        case 2:                                \
-            field = readw(addr);               \
-            break;                             \
-        case 4:                                \
-            field = readl(addr);               \
-            break;                             \
-        case 8:                                \
-            field = readq(addr);               \
-            break;                             \
-        default:                               \
-            g_assert(false);                   \
-        }                                      \
-        addr += sizeof(field);                  \
-    } while (0);
-
-#define ACPI_READ_ARRAY_PTR(arr, length, addr)  \
-    do {                                        \
-        int idx;                                \
-        for (idx = 0; idx < length; ++idx) {    \
-            ACPI_READ_FIELD(arr[idx], addr);    \
-        }                                       \
-    } while (0);
-
-#define ACPI_READ_ARRAY(arr, addr)                               \
-    ACPI_READ_ARRAY_PTR(arr, sizeof(arr)/sizeof(arr[0]), addr)
-
-#define ACPI_READ_TABLE_HEADER(table, addr)                      \
-    do {                                                         \
-        ACPI_READ_FIELD((table)->signature, addr);               \
-        ACPI_READ_FIELD((table)->length, addr);                  \
-        ACPI_READ_FIELD((table)->revision, addr);                \
-        ACPI_READ_FIELD((table)->checksum, addr);                \
-        ACPI_READ_ARRAY((table)->oem_id, addr);                  \
-        ACPI_READ_ARRAY((table)->oem_table_id, addr);            \
-        ACPI_READ_FIELD((table)->oem_revision, addr);            \
-        ACPI_READ_ARRAY((table)->asl_compiler_id, addr);         \
-        ACPI_READ_FIELD((table)->asl_compiler_revision, addr);   \
-    } while (0);
-
-#define ACPI_ASSERT_CMP(actual, expected) do { \
-    uint32_t ACPI_ASSERT_CMP_le = cpu_to_le32(actual); \
-    char ACPI_ASSERT_CMP_str[5] = {}; \
-    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 4); \
-    g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
-} while (0)
-
-#define ACPI_ASSERT_CMP64(actual, expected) do { \
-    uint64_t ACPI_ASSERT_CMP_le = cpu_to_le64(actual); \
-    char ACPI_ASSERT_CMP_str[9] = {}; \
-    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 8); \
-    g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
-} while (0)
-
 static char disk[] = "tests/acpi-test-disk-XXXXXX";
 static const char *data_dir = "tests/acpi-test-data";
 #ifdef CONFIG_IASL
-- 
2.7.4

[Qemu-devel] [PATCH v4 9/9] tests: Add unit tests for the VM Generation ID feature

[ben]

From: Ben Warren <ben@skyportsystems.com>

The following tests are implemented:
* test that a GUID passed in by command line is propagated to the guest.
* test that changing the GUID at runtime via the monitor is reflected in
  the guest.
* test that the "auto" argument to the GUID generates a different, and
  correct GUID as seen by the guest.

  This patch is loosely based on a previous patch from:
  Gal Hammer <ghammer@redhat.com>  and Igor Mammedov <imammedo@redhat.com>

Signed-off-by: Ben Warren <ben@skyportsystems.com>
---
 tests/Makefile.include |   2 +
 tests/vmgenid-test.c   | 184 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 186 insertions(+)
 create mode 100644 tests/vmgenid-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 22ea256..e2dcf15 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -239,6 +239,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
+check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
 check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
 ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
@@ -715,6 +716,7 @@ tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem
 tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o contrib/libvhost-user/libvhost-user.o $(test-util-obj-y)
 tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
 tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
+tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
new file mode 100644
index 0000000..0f5ba07
--- /dev/null
+++ b/tests/vmgenid-test.c
@@ -0,0 +1,184 @@
+/*
+ * QTest testcase for VM Generation ID
+ *
+ * Copyright (c) 2016 Red Hat, Inc.
+ * Copyright (c) 2017 Skyport Systems
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <glib.h>
+#include <string.h>
+#include <unistd.h>
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/uuid.h"
+#include "hw/acpi/acpi-defs.h"
+#include "acpi-utils.h"
+#include "libqtest.h"
+
+#define VGID_GUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+
+static uint64_t vgia;
+
+typedef struct {
+    AcpiTableHeader header;
+    gchar name_op;
+    gchar vgia[4];
+    gchar val_op;
+    uint64_t vgia_val;
+} QEMU_PACKED VgidTable;
+
+static uint64_t find_vgia(void)
+{
+    uint32_t off;
+    AcpiRsdpDescriptor rsdp_table;
+    uint32_t rsdt;
+    AcpiRsdtDescriptorRev1 rsdt_table;
+    int tables_nr;
+    uint32_t *tables;
+    AcpiTableHeader ssdt_table;
+    VgidTable vgid_table;
+    int i;
+
+    /* First, find the RSDP */
+    for (off = 0xf0000; off < 0x100000; off += 0x10) {
+        uint8_t sig[] = "RSD PTR ";
+
+        for (i = 0; i < sizeof sig - 1; ++i) {
+            sig[i] = readb(off + i);
+        }
+
+        if (!memcmp(sig, "RSD PTR ", sizeof sig)) {
+            break;
+        }
+    }
+    g_assert_cmphex(off, <, 0x100000);
+
+    /* Parse the RSDP header so we can find the RSDT */
+    ACPI_READ_FIELD(rsdp_table.signature, off);
+    ACPI_ASSERT_CMP64(rsdp_table.signature, "RSD PTR ");
+
+    ACPI_READ_FIELD(rsdp_table.checksum, off);
+    ACPI_READ_ARRAY(rsdp_table.oem_id, off);
+    ACPI_READ_FIELD(rsdp_table.revision, off);
+    ACPI_READ_FIELD(rsdp_table.rsdt_physical_address, off);
+
+    rsdt = rsdp_table.rsdt_physical_address;
+    /* read the header */
+    ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
+    ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
+
+    /* compute the table entries in rsdt */
+    tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
+                sizeof(uint32_t);
+    g_assert_cmpint(tables_nr, >, 0);
+
+    /* get the addresses of the tables pointed by rsdt */
+    tables = g_new0(uint32_t, tables_nr);
+    ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
+
+    for (i = 0; i < tables_nr; i++) {
+        ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
+        if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
+            /* the first entry in the table should be VGIA
+             * That's all we need */
+            ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
+            g_assert(vgid_table.name_op == 0x08);  /* name */
+            ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
+            g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
+            ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
+            g_assert(vgid_table.val_op == 0x0E);  /* qword */
+            ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
+            return vgid_table.vgia_val;
+        }
+    }
+    return 0;
+}
+
+static void vmgenid_read_guid(uint8_t *guid)
+{
+    int i;
+
+    if (vgia == 0) {
+        vgia = find_vgia();
+    }
+    g_assert(vgia);
+
+    /* Read the GUID directly from guest memory */
+    for (i = 0; i < 16; i++) {
+        guid[i] = readb(vgia + i);
+    }
+}
+
+static void vmgenid_test(void)
+{
+    uint8_t measured[16];
+    QemuUUID expected;
+    g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
+    vmgenid_read_guid(measured);
+    g_assert(memcmp(measured, expected.data, sizeof(measured)) == 0);
+}
+
+static void vmgenid_set_guid_test(void)
+{
+    QDict *response;
+    gchar *cmd;
+    uint8_t measured[16];
+    QemuUUID expected;
+    g_assert(qemu_uuid_parse(VGID_GUID, &expected) == 0);
+    /* Change the GUID slightly */
+    expected.data[0] += 1;
+
+    cmd = g_strdup_printf("{ 'execute': 'qom-set', 'arguments': { "
+                   "'path': '/machine/peripheral/testvgid', "
+                   "'property': 'guid', 'value': '%s' } }",
+                   qemu_uuid_unparse_strdup(&expected));
+    response = qmp(cmd);
+    g_assert(qdict_haskey(response, "return"));
+    QDECREF(response);
+
+    vmgenid_read_guid(measured);
+    g_assert(memcmp(measured, expected.data, sizeof(measured)) == 0);
+}
+
+static void vmgenid_set_guid_auto_test(void)
+{
+    QDict *response;
+    QemuUUID measured;
+    QemuUUID expected;
+
+    /* Read the initial value */
+    vmgenid_read_guid(expected.data);
+
+    /* Setting to 'auto' generates a random GUID */
+    response = qmp("{ 'execute': 'qom-set', 'arguments': { "
+                   "'path': '/machine/peripheral/testvgid', "
+                   "'property': 'guid', 'value': 'auto' } }");
+
+    g_assert(qdict_haskey(response, "return"));
+    QDECREF(response);
+
+    vmgenid_read_guid(measured.data);
+    g_assert(memcmp(measured.data, expected.data, sizeof(measured.data)) != 0);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_start("-machine accel=tcg -device vmgenid,id=testvgid,"
+                "guid=" VGID_GUID);
+    qtest_add_func("/vmgenid/vmgenid", vmgenid_test);
+    qtest_add_func("/vmgenid/vmgenid/set-guid", vmgenid_set_guid_test);
+    qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
+                   vmgenid_set_guid_auto_test);
+    ret = g_test_run();
+
+    qtest_end();
+
+    return ret;
+}
-- 
2.7.4