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

[Michael S. Tsirkin]

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