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

[Laszlo Ersek]

On 01/25/17 19:35, Michael S. Tsirkin wrote:
> On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
>> Hi Laszlo,
>>
>>
>>     On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <address@hidden> 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, address@hidden wrote:
>>
>>         From: Ben Warren <address@hidden>
>>
>>         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 <address@hidden>
>>         ---
>>         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
>>
> 
>