diff mbox

[v4,1/9] ACPI: Add a function for building named qword entries

Message ID dabc5ffbb5b9d29e88087dcbe03b1ba0232c342c.1485308342.git.ben@skyportsystems.com
State New
Headers show

Commit Message

ben@skyportsystems.com Jan. 25, 2017, 1:43 a.m. UTC 
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(+)

Comments

Laszlo Ersek Jan. 25, 2017, 3:55 a.m. UTC | #1 
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);
>  
>
ben@skyportsystems.com Jan. 25, 2017, 5:36 p.m. UTC | #2 
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
Michael S. Tsirkin Jan. 25, 2017, 6:35 p.m. UTC | #3 
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
>
Laszlo Ersek Jan. 26, 2017, 12:48 a.m. UTC | #4 
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
>>
> 
>
ben@skyportsystems.com Jan. 26, 2017, 5:35 a.m. UTC | #5 
> 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
Laszlo Ersek Jan. 26, 2017, 8:21 a.m. UTC | #6 
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
>
Michael S. Tsirkin Jan. 26, 2017, 3:20 p.m. UTC | #7 
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
> >>
> > 
> >
Laszlo Ersek Jan. 26, 2017, 5:43 p.m. UTC | #8 
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
Michael S. Tsirkin Jan. 26, 2017, 6:15 p.m. UTC | #9 
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.
Laszlo Ersek Jan. 26, 2017, 6:25 p.m. UTC | #10 
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
Michael S. Tsirkin Jan. 26, 2017, 6:59 p.m. UTC | #11 
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;
Laszlo Ersek Jan. 27, 2017, 3:20 a.m. UTC | #12 
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
Kevin O'Connor Jan. 27, 2017, 2:18 p.m. UTC | #13 
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
Laszlo Ersek Jan. 27, 2017, 2:46 p.m. UTC | #14 
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
Kevin O'Connor Jan. 27, 2017, 3:43 p.m. UTC | #15 
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
Laszlo Ersek Jan. 27, 2017, 4:12 p.m. UTC | #16 
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
ben@skyportsystems.com Jan. 27, 2017, 6:19 p.m. UTC | #17 
> 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
Laszlo Ersek Jan. 30, 2017, 12:07 p.m. UTC | #18 
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
Michael S. Tsirkin Jan. 30, 2017, 8:28 p.m. UTC | #19 
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.
Igor Mammedov Jan. 31, 2017, 9:51 a.m. UTC | #20 
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.
>
Michael S. Tsirkin Jan. 31, 2017, 9:39 p.m. UTC | #21 
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.
> >
Igor Mammedov Feb. 1, 2017, 11:46 a.m. UTC | #22 
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.
> > >
Michael S. Tsirkin Feb. 1, 2017, 5:55 p.m. UTC | #23 
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.
> > > >
diff mbox

Patch

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