[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries |
Date: |
Thu, 26 Jan 2017 01:48:37 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 |
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
>>
>
>
- [Qemu-devel] [PATCH v4 0/9] Add support for VM Generation ID, ben, 2017/01/24
- [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, ben, 2017/01/24
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Laszlo Ersek, 2017/01/24
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Michael S. Tsirkin, 2017/01/26
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Laszlo Ersek, 2017/01/26
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Michael S. Tsirkin, 2017/01/26
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Laszlo Ersek, 2017/01/26
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Michael S. Tsirkin, 2017/01/26
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Laszlo Ersek, 2017/01/26
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Kevin O'Connor, 2017/01/27
- Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries, Laszlo Ersek, 2017/01/27