Re: [Qemu-devel] [PATCH v5 01/10] ACPI: Add a function for building named qword entries

[Laszlo Ersek]

On 02/08/17 11:43, Igor Mammedov wrote:
> On Tue, 7 Feb 2017 21:09:27 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 02/07/17 14:51, Igor Mammedov wrote:
>>> On Sun,  5 Feb 2017 01:11:56 -0800
>>> address@hidden wrote:
>>>   
>>>> From: Ben Warren <address@hidden>
>>>>
>>>> This is initially used to patch a 64-bit address into
>>>> the VM Generation ID SSDT
>>>>
>>>> Signed-off-by: Ben Warren <address@hidden>
>>>> ---  
>>> ...  
>>>> +int
>>>> +build_append_named_qword(GArray *array, const char *name_format, ...)  
>>> it ain't used anywhere, I'd just drop this patch.  
>>
>> Ben and I discussed this under
>> - msgid <address@hidden>
>>   https://www.mail-archive.com/address@hidden/msg425496.html
>> - msgid <address@hidden>
>>   https://www.mail-archive.com/address@hidden/msg425519.html
>>
>> On 01/26/17 06:35, Ben Warren wrote:
>>> 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 generally agree that dead code is undesirable, but this function has
>> surfaced several times until now, and we get to review it every single
>> time. Ben tested it, I support its inclusion.
>>
>> OTOH I also pointed it out to Ben
>>
>> https://www.mail-archive.com/address@hidden/msg425218.html
>>
>> that he should expect disagreement between his reviewers :) Given that
>> I'm observing this series more from the sidelines and you maintain /
>> support ACPI gen in QEMU, I certainly defer to you on this.
> It's not only dead, having patchable QWORD ready for use in QEMU
> would tempt someone to use it and that would lead to XP BSOD
> if it slips screening at review time.

Good point.

> I'd delay this patch until
> we announce that ACPI 1.0 (XP based) guest no more supported by
> new QEMU.
> 
> Anyway I won't object to merging this if you insist and give it your RB.

No, you are entirely right. As long as we consider Windows XP guests
first class citizens (i.e., we even reject the idea of additional
command line options), then keeping QWORD "out of sight" is safer.

Follow up question: when are we going to drop Windows XP? Do we have a
plan? :) It's quite a struggle to support both ends of the spectrum with
the same machine types and options (decades-old DOS and Windows XP vs.
supercomputer-class NUMA setups). I think this limits development.

(Just curious; I'm not suggesting to drop Windows XP "soon".)

Thanks!
Laszlo


>>
>>
>>>> +{
>>>> +    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, 0x0000000000000000, 8);
>>>> +    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);
>>>>    
>>>
>>>   
>>
>