Re: [Qemu-devel] [PATCH] acpi: switch to a dummy SSDT

[Igor Mammedov]

On Wed, 10 Jan 2018 13:55:37 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, Jan 10, 2018 at 12:41:52PM +0100, Igor Mammedov wrote:
> > On Wed, 10 Jan 2018 13:29:09 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >   
> > > On Wed, Jan 10, 2018 at 12:25:34PM +0100, Igor Mammedov wrote:  
> > > > On Tue, 9 Jan 2018 15:58:11 +0200
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > >   
> > > > > We prefer not changing table sizes depending on parameters,
> > > > > that's why we create a dummy table rather than just drop
> > > > > the MCFG table.
> > > > > 
> > > > > However, a table named "QEMU" could be put to a better
> > > > > use than just a stub, e.g. we could use it to pass
> > > > > some QEMU specific info to guests.
> > > > > 
> > > > > Replace with an SSDT and pad with the Nop opcode
> > > > > to preserve the length.  
> > > > Not sure it's useful, do you have patches on top of that, which will 
> > > > use this?  
> > > 
> > > Yes but not me - Stefan (Cc'd) wants to add a custom table.  
> > Re-purposing dummy table that might become not dummy doesn't look like good 
> > approach.
> > What if later the guest enables MCFG and reboots, DUMMY SSDT would be 
> > replaced by MCFG?   
> 
> So what's the issue?  Why is this worse than a dummy QEMU table?
> There's actually no re-purposing here at all. Either we create
> an MCFG or an SSDT. We never re-purpose.
I think, I've misread commit message, so it's table name 'QEMU'
that's going to be reused.

Having several tables with name QEMU shouldn't be an issue,
adding oem/oem_id should be sufficient to distinguish them
for parsing side (if there is any).

As for change from ignored table to always parsed SSDT table,
having ignored one seems better.
So I'd drop SSDT+memset and keep oem[id] parts in this patch.
 
> > It'd be cleaner if Stefan would just use his own dedicated SSDT.  
> 
> SSDT doesn't always work. A data table is needed so e.g. seabios
> can process it easily.
> 
> > > >   
> > > > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > > > ---
> > > > >  hw/i386/acpi-build.c | 17 ++++++++++++-----
> > > > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index 73519ab..d0afb31e 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -2419,6 +2419,8 @@ build_mcfg_q35(GArray *table_data, BIOSLinker 
> > > > > *linker, AcpiMcfgInfo *info)
> > > > >  {
> > > > >      AcpiTableMcfg *mcfg;
> > > > >      const char *sig;
> > > > > +    const char *oem;
> > > > > +    const char *oem_id;
> > > > >      int len = sizeof(*mcfg) + 1 * sizeof(mcfg->allocation[0]);
> > > > >  
> > > > >      mcfg = acpi_data_push(table_data, len);
> > > > > @@ -2431,16 +2433,21 @@ build_mcfg_q35(GArray *table_data, BIOSLinker 
> > > > > *linker, AcpiMcfgInfo *info)
> > > > >      /* MCFG is used for ECAM which can be enabled or disabled by 
> > > > > guest.
> > > > >       * To avoid table size changes (which create migration issues),
> > > > >       * always create the table even if there are no allocations,
> > > > > -     * but set the signature to a reserved value in this case.
> > > > > -     * ACPI spec requires OSPMs to ignore such tables.
> > > > > +     * but fill it with Noop values.
> > > > > +     * OSPMs ignore such tables.
> > > > >       */
> > > > >      if (info->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) {
> > > > > -        /* Reserved signature: ignored by OSPM */
> > > > > -        sig = "QEMU";
> > > > > +        sig = "SSDT";
> > > > > +        oem = "QEMU ";
> > > > > +        oem_id = "NOOP";
> > > > > +        /* 0xa3 - NoopOp */
> > > > > +        memset(&mcfg->reserved, 0xa3, len - offsetof(AcpiTableMcfg, 
> > > > > reserved));
> > > > >      } else {
> > > > >          sig = "MCFG";
> > > > > +        oem = NULL;
> > > > > +        oem_id = NULL;
> > > > >      }
> > > > > -    build_header(linker, table_data, (void *)mcfg, sig, len, 1, 
> > > > > NULL, NULL);
> > > > > +    build_header(linker, table_data, (void *)mcfg, sig, len, 1, oem, 
> > > > > oem_id);
> > > > >  }
> > > > >  
> > > > >  /*