Re: [Qemu-devel] [RFC] copy OEM ACPI parameters from SLIC table to RSDT

[Michael S. Tsirkin]

On Sun, Apr 06, 2014 at 03:08:46PM +0400, Michael Tokarev wrote:
> 06.04.2014 14:53, Michael S. Tsirkin wrote:
> > On Sun, Apr 06, 2014 at 01:49:11PM +0400, Michael Tokarev wrote:
> >> When building RSDT table, pick OEM ID fields from uer-supplied SLIC
> >> table instead of using hard-coded QEMU defaults.  This way, say,
> >> OEM version of Windows7 can be run inside qemu using the same OEM
> >> activation as on bare metal, by pointing at system firmware:
> >>
> >>   -acpitable file=/sys/firmware/acpi/tables/SLIC
> > 
> > Right, so this doesn't work in 1.7 either, right?
> > It's not a regression?
> 
> It doesn't work in 1.7 too, but it is not a regression.
> Before 1.7 I used a very similar approach patching seabios,
> for several years.  Here's one of the first versions of this
> approach -- http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg03080.html
> 
> I used this approach since that time locally.
> For 1.7 I used seabios without acpi table loading interface, so
> old approach worked fine for me.  Here's the current seabios patch:
> 
> --- a/src/fw/acpi.c     2013-11-24 00:03:36.000000000 +0400
> +++ b/src/fw/acpi.c     2013-11-24 00:03:48.053886298 +0400
> @@ -20,6 +20,8 @@
> 
>  u32 acpi_pm1a_cnt VARFSEG;
> 
> +static const struct acpi_table_header *slic;
> +
>  static void
>  build_header(struct acpi_table_header *h, u32 sig, int len, u8 rev)
>  {
> @@ -32,6 +34,9 @@ build_header(struct acpi_table_header *h
>      h->oem_revision = cpu_to_le32(1);
>      memcpy(h->asl_compiler_id, BUILD_APPNAME4, 4);
>      h->asl_compiler_revision = cpu_to_le32(1);
> +    if (slic && sig == RSDT_SIGNATURE)
> +        /* for win7/vista, RSDT oem should match SLIC, min 14 bytes */
> +        memcpy(h->oem_id, slic->oem_id, 6 + 4 + 4);
>      h->checksum -= checksum(h, len);
>  }
> 
> @@ -642,6 +647,8 @@ acpi_setup(void)
>              }
>          } else {
>              ACPI_INIT_TABLE(table);
> +            if (table->signature == 0x43494c53/*SLIC*/)
> +                slic = table;
>          }
>          if (tbl_idx == MAX_ACPI_TABLES) {
>              warn_noalloc();
> 
> >> Windows7 requires that OEM ID in RSDT matches those in SLIC to
> >> consider SLIC to be valid.
> > 
> > Which fields need to match which, exactly?
> 
> As can be seen in the patch itself, that's 2 fields - oem_id [4]

Surely oem_id[6]?

> and oem_table_id[8], in SLIC and RSDT.  This is enough for win7
> at least.
> 
> >> This is somewhat hackish approach, but it works fairy well in
> >> practice.
> >>
> >> I'm not asking to apply this directly, but instead am trying to
> >> show what's needed to get win to work.  Maybe a new command-line
> >> option for that will do, maybe something else.
> > 
> > I think it's kind of reasonable - seems better than
> > adding more flags - but I'd like to avoid
> > poking at acpi_tables array in acpi-build.c
> > 
> > How about an API to set/query OEM ID?
> 
> Such an api might be useful, and it actually _is_ useful to keep
> compatibility with older guests, which is a slightly different
> but very important topic (for example, my win8 guest which were
> activated in qemu-1.1 does not work anymore in qemu-1.2, exactly
> because of changed OEM ID, -- it is the commit which updated
> qemu version number which breaks win8 activation here).

I'm guessing 1.1 compatibility isn't perfect, but
this shouldn't happen with recent versions like 1.6 and 2.0 at least:
2.0 should not need re-activation if you run
with -M option compatible with 1.6.

> But this api wont help when we already have whole SLIC table as
> in the example above.

I think I wasn't clear. I merely meant an internal API,
have acpi_table_install call it internally instead of
exposing the offset field.


> 
> Overall, the more I think about it, the less I consider it useful.
> Because it looks like windows vista and 7 era is finished (yes, the
> same thing applies to vista as well),

windows  7 is still in mainstream support.  Extended support is until
2020.  Seems worth supporting.

> now it is win8+ which does
> not use this OEM activation mechanism anymore.  Instead, it uses
> hw-based activation, for which we need to keep OEM ID in ACPI too.

Yes, if this patch is not in 2.0, we need to be careful and
only change ID with new -M flag.

> Maybe back in 2010 when I faced this problem for the first time it
> was a good idea to have slic-based adoption for rsdt in qemu, now
> it isn't that important anymore.
> 
> Thanks,
> 
> /mjt
> 
> >>  hw/acpi/core.c       |    5 +++++
> >>  hw/i386/acpi-build.c |    7 +++++++
> >>  2 files changed, 12 insertions(+)
> >>
> >> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> >> index 79414b4..a8a3f26 100644
> >> --- a/hw/acpi/core.c
> >> +++ b/hw/acpi/core.c
> >> @@ -53,6 +53,7 @@ static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - 
> >> ACPI_TABLE_PFX_SIZE] =
> >>  
> >>  char unsigned *acpi_tables;
> >>  size_t acpi_tables_len;
> >> +size_t slic_table_offset;
> >>  
> >>  static QemuOptsList qemu_acpi_opts = {
> >>      .name = "acpi",
> >> @@ -226,6 +227,10 @@ static void acpi_table_install(const char unsigned 
> >> *blob, size_t bloblen,
> >>      /* recalculate checksum */
> >>      ext_hdr->checksum = acpi_checksum((const char unsigned *)ext_hdr +
> >>                                        ACPI_TABLE_PFX_SIZE, 
> >> acpi_payload_size);
> >> +
> >> +    if (memcmp(ext_hdr->sig, "SLIC", 4) == 0) {
> >> +       slic_table_offset = acpi_tables_len - acpi_payload_size;
> >> +    }
> >>  }
> >>  
> >>  void acpi_table_add(const QemuOpts *opts, Error **errp)
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index a5d3fbf..9e0e16a 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -224,6 +224,8 @@ static void acpi_get_pci_info(PcPciInfo *info)
> >>  #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
> >>  #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
> >>  
> >> +extern size_t slic_table_offset;
> >> +
> >>  static void
> >>  build_header(GArray *linker, GArray *table_data,
> >>               AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
> >> @@ -237,6 +239,11 @@ build_header(GArray *linker, GArray *table_data,
> >>      h->oem_revision = cpu_to_le32(1);
> >>      memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
> >>      h->asl_compiler_revision = cpu_to_le32(1);
> >> +    if (memcmp(sig, "RSDT", 4) == 0 && slic_table_offset) {
> >> +      /* for win7: OEM info in RSDT and SLIC should be the same */
> >> +      AcpiTableHeader *s = (AcpiTableHeader *)(acpi_tables + 
> >> slic_table_offset);
> >> +      memcpy(h->oem_id, s->oem_id, 6 + 4 + 4);
> > 
> > 
> > what does 6 + 4 +4 mean?
> > I see:
> >     uint8_t  oem_id [6];             /* OEM identification */ \
> >     uint8_t  oem_table_id [8];       /* OEM table identification */ \
> >     uint32_t oem_revision;           /* OEM revision number */ \
> > 
> > Do table id have to match? It seems a bit wrong to have two tables
> > with the same id.
> > 
> >> +    }
> >>      h->checksum = 0;
> >>      /* Checksum to be filled in by Guest linker */
> >>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
> >> -- 
> >> 1.7.10.4
> >>