On Wed, Jan 10, 2018 at 01:31:09PM +0100, Igor Mammedov wrote:
> On Wed, 10 Jan 2018 13:55:37 +0200
> "Michael S. Tsirkin" <m...@redhat.com> 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" <m...@redhat.com> 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" <m...@redhat.com> 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).

It seems more work for bios to scan all such
tables and not just the first one. I might be wrong here.

> 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.

I'll wait until Stefan posts his patches using this, then we
can see what does look better.


> > > 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 <m...@redhat.com>
> > > > > > ---
> > > > > >  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);
> > > > > >  }
> > > > > >  
> > > > > >  /*  

Reply via email to