Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 02:08:03AM +0200, Laszlo Ersek wrote:
> On 06/08/15 20:14, Michael S. Tsirkin wrote:
> > XSDT support allows using ACPI 2 features while
> > avoiding breaking legacy windows XP guests:
> > ACPI 2 tables are linked from XSDT only,
> > ACPI 1 tables from both RSDT and XSDT, this way
> > XP does not see ACPI 2 tables.
> > 
> > As a first step, this patchset generates v2 RSDP
> > and fills in XSDT matching RSDT exactly.
> > 
> > ARM can switch to XSDT as well, I'm not bothering
> > until there's an easy way to test that.
> > 
> > Note: unit test files need to be updated with this,
> > I'm not bothering with posting them.
> > 
> > Changes from v1:
> >     xsdt address is 64 bit
> >     arm patch is now tested
> > 
> > Michael S. Tsirkin (4):
> >   acpi: add API for 64 bit offsets
> >   i386/acpi: collect 64 bit offsets for xsdt
> >   i386/acpi: add XSDT
> >   acpi: unify rsdp generation
> > 
> >  include/hw/acpi/acpi-defs.h | 15 +++++--
> >  include/hw/acpi/aml-build.h |  7 +++-
> >  hw/acpi/aml-build.c         | 99 
> > +++++++++++++++++++++++++++++++++++++--------
> >  hw/arm/virt-acpi-build.c    | 39 +++---------------
> >  hw/i386/acpi-build.c        | 64 +++++++++++------------------
> >  5 files changed, 129 insertions(+), 95 deletions(-)
> > 
> 
> I tested this series with ArmVirtQemu (aka AAVMF) and OVMF too. (They
> use common code in edk2.)
> 
> The ARM build works indeed, but that's only because in patch #4 we have
> 
>   build_rsdp(tables->rsdp, tables->linker, rsdt, 0);
> 
> ie. there's only an RSDT.
> 
> Due to patch #3 however, the OVMF client breaks:
> 
>   build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);
> 
> The problem is that the "directed acyclic graph" of ADD_POINTER edges is
> no longer a tree. At least some tables can be reached on multiple paths.
> (Eg. the FADT can be reached via RSDP->RSDT-> FADT, and also
> RSDP->XSDT->FADT.)
> 
> This is a problem because EFI_ACPI_TABLE_PROTOCOL has "builtin
> knowledge" about what tables may not be installed only once vs. several
> times. Unfortunately, in this case both decisions have bad consequences:
> 
> - When FADT is passed to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for
> the second time, the function returns EFI_ACCESS_DENIED, and the
> linker/loader client bails out.
> 
> - When (eg.) the same SSDT is passed to
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for the second time, the
> function will happily install (a copy of) it again, and then we'll end
> up with two copies of the same SSDT.
> 
> EFI_ACPI_TABLE_PROTOCOL manages both the RSDT and the XSDT internally.
> As far as I can see, it puts each table in both. The RSDT and the XSDT
> are not distinguished even on the UEFI spec level; it lumps them
> together under "RSDT/XSDT" every time.
> 
> This is probably very frustrating; sorry about that.
> 
> Laszlo

Thanks for the info!  This is worth fixing.  Can you fix this without
protocol changes, or should we change the protocol to pass a hint that a
pointer is to another instance of a previously used table?

-- 
MST