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

[Igor Mammedov]

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. 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.


> 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);
> >>    
> > 
> >   
>