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 <ler...@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, b...@skyportsystems.com wrote: >> >> From: Ben Warren <b...@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 <b...@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 >> > >
