| Message ID | 1445612242-79172-16-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 23, 2015 at 04:57:18PM +0200, Igor Mammedov wrote: > it turns on 64-bit integer handling in OSPM, which we could use > for writing simpler/smaller AML code. > Tested with Windows XP and Windows Server 2008, Linux: > * XP doesn't care about revision and continues to use 32 integers > and boots just fine with this change. > * WS 2008 and Linux - support rev2 and use 64-bit integers > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> This is still planned, right? IIUC you didn't post any code that needs the 64 bit math. > --- > hw/i386/acpi-build.c | 2 +- > hw/i386/acpi-dsdt.dsl | 2 +- > hw/i386/q35-acpi-dsdt.dsl | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 8add4d9..c929540 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -1484,7 +1484,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc) > > memset(dsdt, 0, sizeof *dsdt); > build_header(linker, table_data, dsdt, "DSDT", > - misc->dsdt_size, 1); > + misc->dsdt_size, 2); > } > > static GArray * > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > index 8dba096..6d46b36 100644 > --- a/hw/i386/acpi-dsdt.dsl > +++ b/hw/i386/acpi-dsdt.dsl > @@ -22,7 +22,7 @@ ACPI_EXTRACT_ALL_CODE AcpiDsdtAmlCode > DefinitionBlock ( > "acpi-dsdt.aml", // Output Filename > "DSDT", // Signature > - 0x01, // DSDT Compliance Revision > + 0x02, // DSDT Compliance Revision > "BXPC", // OEMID > "BXDSDT", // TABLE ID > 0x1 // OEM Revision > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > index 7be7b37..ecefdec 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -28,7 +28,7 @@ ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode > DefinitionBlock ( > "q35-acpi-dsdt.aml",// Output Filename > "DSDT", // Signature > - 0x01, // DSDT Compliance Revision > + 0x02, // DSDT Compliance Revision > "BXPC", // OEMID > "BXDSDT", // TABLE ID > 0x2 // OEM Revision > -- > 1.8.3.1
On Sat, 24 Oct 2015 22:40:59 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Oct 23, 2015 at 04:57:18PM +0200, Igor Mammedov wrote: > > it turns on 64-bit integer handling in OSPM, which we could use > > for writing simpler/smaller AML code. > > Tested with Windows XP and Windows Server 2008, Linux: > > * XP doesn't care about revision and continues to use 32 integers > > and boots just fine with this change. > > * WS 2008 and Linux - support rev2 and use 64-bit integers > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > This is still planned, right? IIUC you didn't post any code > that needs the 64 bit math. nope, the next patch 16/19 uses 64-bit math, it greatly simplifies _CRS as we don't have to do 64-bit math manually using 32-bit integers. And even if we put new MHPD.MCRS() that uses 64-bit math in SSDT, it won't crash XP unless someone would try to do memory hotplug and even it could be 'fixed' if we check _REV on every hotplug event, it's a bit ugly but should work. > > > --- > > hw/i386/acpi-build.c | 2 +- > > hw/i386/acpi-dsdt.dsl | 2 +- > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 8add4d9..c929540 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -1484,7 +1484,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc) > > > > memset(dsdt, 0, sizeof *dsdt); > > build_header(linker, table_data, dsdt, "DSDT", > > - misc->dsdt_size, 1); > > + misc->dsdt_size, 2); > > } > > > > static GArray * > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > index 8dba096..6d46b36 100644 > > --- a/hw/i386/acpi-dsdt.dsl > > +++ b/hw/i386/acpi-dsdt.dsl > > @@ -22,7 +22,7 @@ ACPI_EXTRACT_ALL_CODE AcpiDsdtAmlCode > > DefinitionBlock ( > > "acpi-dsdt.aml", // Output Filename > > "DSDT", // Signature > > - 0x01, // DSDT Compliance Revision > > + 0x02, // DSDT Compliance Revision > > "BXPC", // OEMID > > "BXDSDT", // TABLE ID > > 0x1 // OEM Revision > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > index 7be7b37..ecefdec 100644 > > --- a/hw/i386/q35-acpi-dsdt.dsl > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > @@ -28,7 +28,7 @@ ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode > > DefinitionBlock ( > > "q35-acpi-dsdt.aml",// Output Filename > > "DSDT", // Signature > > - 0x01, // DSDT Compliance Revision > > + 0x02, // DSDT Compliance Revision > > "BXPC", // OEMID > > "BXDSDT", // TABLE ID > > 0x2 // OEM Revision > > -- > > 1.8.3.1
On Mon, Oct 26, 2015 at 11:03:01AM +0100, Igor Mammedov wrote: > On Sat, 24 Oct 2015 22:40:59 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Fri, Oct 23, 2015 at 04:57:18PM +0200, Igor Mammedov wrote: > > > it turns on 64-bit integer handling in OSPM, which we could use > > > for writing simpler/smaller AML code. > > > Tested with Windows XP and Windows Server 2008, Linux: > > > * XP doesn't care about revision and continues to use 32 integers > > > and boots just fine with this change. > > > * WS 2008 and Linux - support rev2 and use 64-bit integers > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > This is still planned, right? IIUC you didn't post any code > > that needs the 64 bit math. > nope, the next patch 16/19 uses 64-bit math, > > it greatly simplifies _CRS as we don't have to do > 64-bit math manually using 32-bit integers. > > And even if we put new MHPD.MCRS() that uses 64-bit math in SSDT, > it won't crash XP unless someone would try to do memory hotplug > > and even it could be 'fixed' if we check _REV on every > hotplug event, it's a bit ugly but should work. Aha. That's exactly what I said. All that patch commit log says is pc: acpi: memhp: move MHPD.MCRS method into MHPT table when in fact you also rework it all to use 64 bit math. So pls don't do this. Pls start by rewriting ASL in C while keeping AML code identical. Cleanups - next. > > > > > --- > > > hw/i386/acpi-build.c | 2 +- > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index 8add4d9..c929540 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -1484,7 +1484,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc) > > > > > > memset(dsdt, 0, sizeof *dsdt); > > > build_header(linker, table_data, dsdt, "DSDT", > > > - misc->dsdt_size, 1); > > > + misc->dsdt_size, 2); > > > } > > > > > > static GArray * > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > index 8dba096..6d46b36 100644 > > > --- a/hw/i386/acpi-dsdt.dsl > > > +++ b/hw/i386/acpi-dsdt.dsl > > > @@ -22,7 +22,7 @@ ACPI_EXTRACT_ALL_CODE AcpiDsdtAmlCode > > > DefinitionBlock ( > > > "acpi-dsdt.aml", // Output Filename > > > "DSDT", // Signature > > > - 0x01, // DSDT Compliance Revision > > > + 0x02, // DSDT Compliance Revision > > > "BXPC", // OEMID > > > "BXDSDT", // TABLE ID > > > 0x1 // OEM Revision > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > index 7be7b37..ecefdec 100644 > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > @@ -28,7 +28,7 @@ ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode > > > DefinitionBlock ( > > > "q35-acpi-dsdt.aml",// Output Filename > > > "DSDT", // Signature > > > - 0x01, // DSDT Compliance Revision > > > + 0x02, // DSDT Compliance Revision > > > "BXPC", // OEMID > > > "BXDSDT", // TABLE ID > > > 0x2 // OEM Revision > > > -- > > > 1.8.3.1
On Mon, 26 Oct 2015 12:07:50 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Oct 26, 2015 at 11:03:01AM +0100, Igor Mammedov wrote: > > On Sat, 24 Oct 2015 22:40:59 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Fri, Oct 23, 2015 at 04:57:18PM +0200, Igor Mammedov wrote: > > > > it turns on 64-bit integer handling in OSPM, which we could use > > > > for writing simpler/smaller AML code. > > > > Tested with Windows XP and Windows Server 2008, Linux: > > > > * XP doesn't care about revision and continues to use 32 integers > > > > and boots just fine with this change. > > > > * WS 2008 and Linux - support rev2 and use 64-bit integers > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > This is still planned, right? IIUC you didn't post any code > > > that needs the 64 bit math. > > nope, the next patch 16/19 uses 64-bit math, > > > > it greatly simplifies _CRS as we don't have to do > > 64-bit math manually using 32-bit integers. > > > > And even if we put new MHPD.MCRS() that uses 64-bit math in SSDT, > > it won't crash XP unless someone would try to do memory hotplug > > > > and even it could be 'fixed' if we check _REV on every > > hotplug event, it's a bit ugly but should work. > > Aha. That's exactly what I said. All that patch commit log > says is > pc: acpi: memhp: move MHPD.MCRS method into MHPT table > when in fact you also rework it all to use 64 bit math. yep, it's my fault. I'll fix it. > > So pls don't do this. Pls start by rewriting ASL in C > while keeping AML code identical. Cleanups - next. That's what I'm trying to avoid in this case as reworked code is much simpler than the original ASL. Rewriting original complex MHPD.MCRS() ASL into C and immediately replacing it with simplified version is rather counterproductive especially taking in account that 'make check' tests will fail anyway since code is moved between tables and C-produced AML doesn't exactly match blobs produced by a particular IASL version anyway. But I can certainly do/split it other way around, simplify ASL first and then rewrite result in C. That should make reviewing easier even if tests are broken. > > > > > > > > --- > > > > hw/i386/acpi-build.c | 2 +- > > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > index 8add4d9..c929540 100644 > > > > --- a/hw/i386/acpi-build.c > > > > +++ b/hw/i386/acpi-build.c > > > > @@ -1484,7 +1484,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc) > > > > > > > > memset(dsdt, 0, sizeof *dsdt); > > > > build_header(linker, table_data, dsdt, "DSDT", > > > > - misc->dsdt_size, 1); > > > > + misc->dsdt_size, 2); > > > > } > > > > > > > > static GArray * > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > > index 8dba096..6d46b36 100644 > > > > --- a/hw/i386/acpi-dsdt.dsl > > > > +++ b/hw/i386/acpi-dsdt.dsl > > > > @@ -22,7 +22,7 @@ ACPI_EXTRACT_ALL_CODE AcpiDsdtAmlCode > > > > DefinitionBlock ( > > > > "acpi-dsdt.aml", // Output Filename > > > > "DSDT", // Signature > > > > - 0x01, // DSDT Compliance Revision > > > > + 0x02, // DSDT Compliance Revision > > > > "BXPC", // OEMID > > > > "BXDSDT", // TABLE ID > > > > 0x1 // OEM Revision > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > > index 7be7b37..ecefdec 100644 > > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > > @@ -28,7 +28,7 @@ ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode > > > > DefinitionBlock ( > > > > "q35-acpi-dsdt.aml",// Output Filename > > > > "DSDT", // Signature > > > > - 0x01, // DSDT Compliance Revision > > > > + 0x02, // DSDT Compliance Revision > > > > "BXPC", // OEMID > > > > "BXDSDT", // TABLE ID > > > > 0x2 // OEM Revision > > > > -- > > > > 1.8.3.1
On Mon, Oct 26, 2015 at 12:06:51PM +0100, Igor Mammedov wrote: > On Mon, 26 Oct 2015 12:07:50 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Oct 26, 2015 at 11:03:01AM +0100, Igor Mammedov wrote: > > > On Sat, 24 Oct 2015 22:40:59 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Fri, Oct 23, 2015 at 04:57:18PM +0200, Igor Mammedov wrote: > > > > > it turns on 64-bit integer handling in OSPM, which we could use > > > > > for writing simpler/smaller AML code. > > > > > Tested with Windows XP and Windows Server 2008, Linux: > > > > > * XP doesn't care about revision and continues to use 32 integers > > > > > and boots just fine with this change. > > > > > * WS 2008 and Linux - support rev2 and use 64-bit integers > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > This is still planned, right? IIUC you didn't post any code > > > > that needs the 64 bit math. > > > nope, the next patch 16/19 uses 64-bit math, > > > > > > it greatly simplifies _CRS as we don't have to do > > > 64-bit math manually using 32-bit integers. > > > > > > And even if we put new MHPD.MCRS() that uses 64-bit math in SSDT, > > > it won't crash XP unless someone would try to do memory hotplug > > > > > > and even it could be 'fixed' if we check _REV on every > > > hotplug event, it's a bit ugly but should work. > > > > Aha. That's exactly what I said. All that patch commit log > > says is > > pc: acpi: memhp: move MHPD.MCRS method into MHPT table > > when in fact you also rework it all to use 64 bit math. > yep, it's my fault. I'll fix it. Don't fix the comment. Just do it the right way. > > > > So pls don't do this. Pls start by rewriting ASL in C > > while keeping AML code identical. Cleanups - next. > That's what I'm trying to avoid in this case as > reworked code is much simpler than the original ASL. > Rewriting original complex MHPD.MCRS() ASL into C and > immediately replacing it with simplified version is > rather counterproductive especially taking in account that > 'make check' tests will fail anyway since code is moved > between tables and C-produced AML doesn't exactly match > blobs produced by a particular IASL version anyway. So make it match. It seems pretty robust ATM, since we disassemble and only compare the dis-assembled output. Let's keep that invariant. > But I can certainly do/split it other way around, > simplify ASL first and then rewrite result in C. > That should make reviewing easier even if tests > are broken. So your untested (in the field, I trust you to test your code) C matches untested ASL. This doesn't help too much. Sorry, IMO that's the wrong way to prioritize things. I value ease of testing and reviewing patches much higher than ease of writing them, and stability higher than any individual feature. The original migration into QEMU was completely seemless because of full bit for bit compatibility with seabios. I bent the rules when the stuff was first rewritten in C and this introduced some regressions, but there was just too much demand for the stuff, people had real trouble hacking in 3 languages (python/C/ASL), and that was holding up multiple features so some instability was worth it. Nothing like this here, this is just general cleanup. Let's do it the slow, safe way please. > > > > > > > > > > > --- > > > > > hw/i386/acpi-build.c | 2 +- > > > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > > index 8add4d9..c929540 100644 > > > > > --- a/hw/i386/acpi-build.c > > > > > +++ b/hw/i386/acpi-build.c > > > > > @@ -1484,7 +1484,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc) > > > > > > > > > > memset(dsdt, 0, sizeof *dsdt); > > > > > build_header(linker, table_data, dsdt, "DSDT", > > > > > - misc->dsdt_size, 1); > > > > > + misc->dsdt_size, 2); > > > > > } > > > > > > > > > > static GArray * > > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > > > index 8dba096..6d46b36 100644 > > > > > --- a/hw/i386/acpi-dsdt.dsl > > > > > +++ b/hw/i386/acpi-dsdt.dsl > > > > > @@ -22,7 +22,7 @@ ACPI_EXTRACT_ALL_CODE AcpiDsdtAmlCode > > > > > DefinitionBlock ( > > > > > "acpi-dsdt.aml", // Output Filename > > > > > "DSDT", // Signature > > > > > - 0x01, // DSDT Compliance Revision > > > > > + 0x02, // DSDT Compliance Revision > > > > > "BXPC", // OEMID > > > > > "BXDSDT", // TABLE ID > > > > > 0x1 // OEM Revision > > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > > > index 7be7b37..ecefdec 100644 > > > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > > > @@ -28,7 +28,7 @@ ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode > > > > > DefinitionBlock ( > > > > > "q35-acpi-dsdt.aml",// Output Filename > > > > > "DSDT", // Signature > > > > > - 0x01, // DSDT Compliance Revision > > > > > + 0x02, // DSDT Compliance Revision > > > > > "BXPC", // OEMID > > > > > "BXDSDT", // TABLE ID > > > > > 0x2 // OEM Revision > > > > > -- > > > > > 1.8.3.1
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 8add4d9..c929540 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -1484,7 +1484,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc) memset(dsdt, 0, sizeof *dsdt); build_header(linker, table_data, dsdt, "DSDT", - misc->dsdt_size, 1); + misc->dsdt_size, 2); } static GArray * diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl index 8dba096..6d46b36 100644 --- a/hw/i386/acpi-dsdt.dsl +++ b/hw/i386/acpi-dsdt.dsl @@ -22,7 +22,7 @@ ACPI_EXTRACT_ALL_CODE AcpiDsdtAmlCode DefinitionBlock ( "acpi-dsdt.aml", // Output Filename "DSDT", // Signature - 0x01, // DSDT Compliance Revision + 0x02, // DSDT Compliance Revision "BXPC", // OEMID "BXDSDT", // TABLE ID 0x1 // OEM Revision diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl index 7be7b37..ecefdec 100644 --- a/hw/i386/q35-acpi-dsdt.dsl +++ b/hw/i386/q35-acpi-dsdt.dsl @@ -28,7 +28,7 @@ ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode DefinitionBlock ( "q35-acpi-dsdt.aml",// Output Filename "DSDT", // Signature - 0x01, // DSDT Compliance Revision + 0x02, // DSDT Compliance Revision "BXPC", // OEMID "BXDSDT", // TABLE ID 0x2 // OEM Revision
it turns on 64-bit integer handling in OSPM, which we could use for writing simpler/smaller AML code. Tested with Windows XP and Windows Server 2008, Linux: * XP doesn't care about revision and continues to use 32 integers and boots just fine with this change. * WS 2008 and Linux - support rev2 and use 64-bit integers Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/acpi-build.c | 2 +- hw/i386/acpi-dsdt.dsl | 2 +- hw/i386/q35-acpi-dsdt.dsl | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)