| Message ID | e531606c10f3c40dbc10d0fa4a4a5d806d7946d9.1486285434.git.ben@skyportsystems.com |
|---|---|
| State | New |
| Headers | show |
On Sun, 5 Feb 2017 01:11:56 -0800 ben@skyportsystems.com wrote: > From: Ben Warren <ben@skyportsystems.com> > > This is initially used to patch a 64-bit address into > the VM Generation ID SSDT > > Signed-off-by: Ben Warren <ben@skyportsystems.com> > --- ... > +int > +build_append_named_qword(GArray *array, const char *name_format, ...) it ain't used anywhere, I'd just drop this patch. > +{ > + 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); >
On 02/07/17 14:51, Igor Mammedov wrote: > On Sun, 5 Feb 2017 01:11:56 -0800 > ben@skyportsystems.com wrote: > >> From: Ben Warren <ben@skyportsystems.com> >> >> This is initially used to patch a 64-bit address into >> the VM Generation ID SSDT >> >> Signed-off-by: Ben Warren <ben@skyportsystems.com> >> --- > ... >> +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 <6E25852D-224A-4BDC-AA83-8DC87DB4D0F0@skyportsystems.com> https://www.mail-archive.com/qemu-devel@nongnu.org/msg425496.html - msgid <a93a6be3-cbed-5afe-7c72-96410fbfce41@redhat.com> https://www.mail-archive.com/qemu-devel@nongnu.org/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/qemu-devel@nongnu.org/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. 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); >> > >
On Tue, 7 Feb 2017 21:09:27 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 02/07/17 14:51, Igor Mammedov wrote: > > On Sun, 5 Feb 2017 01:11:56 -0800 > > ben@skyportsystems.com wrote: > > > >> From: Ben Warren <ben@skyportsystems.com> > >> > >> This is initially used to patch a 64-bit address into > >> the VM Generation ID SSDT > >> > >> Signed-off-by: Ben Warren <ben@skyportsystems.com> > >> --- > > ... > >> +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 <6E25852D-224A-4BDC-AA83-8DC87DB4D0F0@skyportsystems.com> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg425496.html > - msgid <a93a6be3-cbed-5afe-7c72-96410fbfce41@redhat.com> > https://www.mail-archive.com/qemu-devel@nongnu.org/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/qemu-devel@nongnu.org/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); > >> > > > > >
On 02/08/17 11:43, Igor Mammedov wrote: > On Tue, 7 Feb 2017 21:09:27 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 02/07/17 14:51, Igor Mammedov wrote: >>> On Sun, 5 Feb 2017 01:11:56 -0800 >>> ben@skyportsystems.com wrote: >>> >>>> From: Ben Warren <ben@skyportsystems.com> >>>> >>>> This is initially used to patch a 64-bit address into >>>> the VM Generation ID SSDT >>>> >>>> Signed-off-by: Ben Warren <ben@skyportsystems.com> >>>> --- >>> ... >>>> +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 <6E25852D-224A-4BDC-AA83-8DC87DB4D0F0@skyportsystems.com> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg425496.html >> - msgid <a93a6be3-cbed-5afe-7c72-96410fbfce41@redhat.com> >> https://www.mail-archive.com/qemu-devel@nongnu.org/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/qemu-devel@nongnu.org/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); >>>> >>> >>> >> >
> On Feb 8, 2017, at 2:53 AM, Laszlo Ersek <lersek@redhat.com> wrote: > > On 02/08/17 11:43, Igor Mammedov wrote: >> On Tue, 7 Feb 2017 21:09:27 +0100 >> Laszlo Ersek <lersek@redhat.com> wrote: >> >>> On 02/07/17 14:51, Igor Mammedov wrote: >>>> On Sun, 5 Feb 2017 01:11:56 -0800 >>>> ben@skyportsystems.com wrote: >>>> >>>>> From: Ben Warren <ben@skyportsystems.com> >>>>> >>>>> This is initially used to patch a 64-bit address into >>>>> the VM Generation ID SSDT >>>>> >>>>> Signed-off-by: Ben Warren <ben@skyportsystems.com> >>>>> --- >>>> ... >>>>> +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 <6E25852D-224A-4BDC-AA83-8DC87DB4D0F0@skyportsystems.com> >>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg425496.html >>> - msgid <a93a6be3-cbed-5afe-7c72-96410fbfce41@redhat.com> >>> https://www.mail-archive.com/qemu-devel@nongnu.org/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/qemu-devel@nongnu.org/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);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index b2a1e40..9fc54c9 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -264,6 +264,9 @@ static void build_append_int(GArray *table, uint64_t value) * Warning: runtime patching is best avoided. Only use this as * a replacement for DataTableRegion (for guests that don't * support it). + * + * ACPI 5.0: 19.2.5 + * 32-bit integers are supported beginning with ACPI 1.0 */ int build_append_named_dword(GArray *array, const char *name_format, ...) @@ -285,6 +288,38 @@ build_append_named_dword(GArray *array, const char *name_format, ...) return offset; } +/* + * Build NAME(XXXX, 0x0000000000000000) where 0x0000000000000000 + * is encoded as a qword, and return the offset to 0x0000000000000000 + * for runtime patching. + * + * Warning: runtime patching is best avoided. Only use this as + * a replacement for DataTableRegion (for guests that don't + * support it). + * + * ACPI 5.0: 19.2.5 + * 64-bit integers are supported beginning with ACPI 2.0 + */ +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, 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);