Re: [Qemu-devel] [PATCH 15/19] pc: acpi: bump DSDT revision compliance to v2

[Michael S. Tsirkin]

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" <address@hidden> 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 <address@hidden>
> > 
> > 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