> On Jan 25, 2017, at 4:48 PM, 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 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 >>> >>> 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 >>> --- >>> 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 > >: > > 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