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

[Ben Warren]

> On Feb 8, 2017, at 2:53 AM, Laszlo Ersek <address@hidden> wrote:
> 
> 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
> 
OK, I’ll drop this patch from the set.  I guess it will always be around if we 
want to resurrect.
> 
>>> 
>>> 
>>>>> +{
>>>>> +    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);

smime.p7s
Description: S/MIME cryptographic signature