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

[Michael S. Tsirkin]

On Thu, Jan 26, 2017 at 01:48:37AM +0100, Laszlo Ersek wrote:
> On 01/25/17 19:35, Michael S. Tsirkin wrote:
> > On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
> >> Hi Laszlo,
> >>
> >>
> >>     On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <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.

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