Re: [Qemu-devel] [PATCH RFC 00/13] qemu: generate acpi tables for the guest

[Anthony Liguori]

"Michael S. Tsirkin" <address@hidden> writes:

> This patchset moves all generation of ACPI tables
> from guest BIOS to the hypervisor.
>
> Although ACPI tables come from a system BIOS on real hw,
> it makes sense that the ACPI tables are coupled with the
> virtual machine, since they have to abstract the x86 machine to
> the OS's.
>
> Several future developments that this will enable:
> - make it easier to use alternative firmware:
>   any firmware can just load the ACPI tables from QEMU.
>   case in point OVMF.

UEFI obviously can create ACPI tables already so I don't think this is a
valid advantage.

You could use this argument to say that QEMU should implement int13 or
int10 too...

> - make it easier to add more chipsets without bumping
>   into fw_cfg boundaries.

But then you're just introducing a different boundary (the ACPI tables).

fw_cfg is a cross-platform interface so it makes a lot more sense to add
more features to it to better enumerate the underlying platform.

> - describe a complex configuration
>   e.g. a bridged PCI topology to enable
>   acpi hotplug of devices behind a PCI bridge,
>   or for multi-root topology.

This has strong analogies to generating device trees and is also a good
reason why exposing this information via a common interface (fw_cfg)
would be a good idea.

>
> If you look at the actual code:
>  i386: imports ACPI table generation code from seabios
> you will see that it's more complex than it
> needs to be, with lots of low level casts
> and similar tricks.
>
> There's also a bit of duplication where we
> already declare similar acpi structures in qemu.
>
> This is the result of code being a direct port from seabios.
> Laszlo's patch (build ACPI MADT (APIC) for fw_cfg)
> shows how this will be cleaned up by follow-up work.
> I think it's best to do it in this order: port
> code directly, and apply cleanups and reduce duplication
> that results, on top.
> This way it's much easier to see that we don't introduce
> regressions.
>
> In particular, for a simple VM with piix,
> I booted a guest on qemu with and without the
> change, and verified that ACPI tables are
> unchanged except for trivial pointer address changes.
>
> Such binary compatibility makes it easier to be
> confident that this change won't break things.
>
>
> So the idea is Laszlo's patchset (patches 2-7) will be rebased on top of
> this patchset, this way we'll get small cleanup changes, where each step
> is easy to verify as not changing anything from Guest POV.
>
> One issue this patchset creates is cross-version migration.
> Specifically migration to qemu 1.5 will not work
> if it happens while bios is accessing fw_cfg when we
> migrate.
> This is a general problem of the fw_cfg implementation,
> I plan to extend fw_cfg implementation to address this.
>
> I plan to test q35 and numa setups before I submit
> this for inclusion, in a similar way.
>
> git trees, for those interested in testing this:
>
> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git acpi
>
> git://git.kernel.org/pub/scm/virt/kvm/mst/seabios.git acpi
>
> bios patchset will be posted tomorrow (need to clean up
> the commit log a bit).

I don't think it's a good idea to move BIOS functionality in QEMU.

We don't frequently add firmware or chipsets so it seems like we're
optimizing for an uncommon scenario here.

Regards,

Anthony Liguori

> Laszlo Ersek (1):
>   refer to FWCfgState explicitly
>
> Michael S. Tsirkin (12):
>   apic: rename apic specific bitopts
>   hw/i386/pc.c: move IO_APIC_DEFAULT_ADDRESS to include/hw/i386/apic.h
>   fw_cfg: move typedef to qemu/typedefs.h
>   i386: add ACPI table files from seabios
>   acpi: add rules to compile ASL source
>   acpi: pre-compiled ASL files
>   range: add Range structure
>   i386: add bios linker/loader
>   i386: generate pc guest info
>   pc: pass PCI hole ranges to Guests
>   i386: ACPI table generation code from seabios
>   pc: reuse guest info for legacy fw cfg
>
>  configure                            |    9 +-
>  hw/acpi/ich9.c                       |    7 +-
>  hw/acpi/piix4.c                      |   44 +-
>  hw/core/loader.c                     |    2 +-
>  hw/i386/Makefile.objs                |   27 +
>  hw/i386/acpi-build.c                 |  685 ++++
>  hw/i386/acpi-dsdt-cpu-hotplug.dsl    |   93 +
>  hw/i386/acpi-dsdt-dbug.dsl           |   41 +
>  hw/i386/acpi-dsdt-hpet.dsl           |   51 +
>  hw/i386/acpi-dsdt-isa.dsl            |  117 +
>  hw/i386/acpi-dsdt-pci-crs.dsl        |  105 +
>  hw/i386/acpi-dsdt.dsl                |  343 ++
>  hw/i386/acpi-dsdt.hex.generated      | 4409 ++++++++++++++++++++
>  hw/i386/bios-linker-loader.c         |  155 +
>  hw/i386/multiboot.c                  |    2 +-
>  hw/i386/multiboot.h                  |    4 +-
>  hw/i386/pc.c                         |  159 +-
>  hw/i386/pc_piix.c                    |   28 +-
>  hw/i386/pc_q35.c                     |   14 +-
>  hw/i386/q35-acpi-dsdt.dsl            |  452 +++
>  hw/i386/q35-acpi-dsdt.hex.generated  | 7346 
> ++++++++++++++++++++++++++++++++++
>  hw/i386/ssdt-misc.dsl                |   73 +
>  hw/i386/ssdt-misc.hex.generated      |  190 +
>  hw/i386/ssdt-pcihp.dsl               |   51 +
>  hw/i386/ssdt-pcihp.hex.generated     |  108 +
>  hw/i386/ssdt-proc.dsl                |   63 +
>  hw/i386/ssdt-proc.hex.generated      |  134 +
>  hw/intc/apic.c                       |   42 +-
>  hw/isa/lpc_ich9.c                    |   11 +-
>  hw/mips/mips_malta.c                 |    2 +-
>  hw/pci-host/q35.c                    |    5 +
>  hw/sparc/sun4m.c                     |    6 +-
>  hw/sparc64/sun4u.c                   |    2 +-
>  include/hw/acpi/ich9.h               |    2 +-
>  include/hw/i386/acpi-build.h         |    9 +
>  include/hw/i386/apic.h               |    2 +
>  include/hw/i386/bios-linker-loader.h |   26 +
>  include/hw/i386/ich9.h               |    3 +-
>  include/hw/i386/pc.h                 |   67 +-
>  include/hw/loader.h                  |    3 +-
>  include/hw/nvram/fw_cfg.h            |    2 +-
>  include/hw/pci-host/q35.h            |    2 +
>  include/qemu/range.h                 |   22 +
>  include/qemu/typedefs.h              |    2 +
>  scripts/acpi_extract.py              |  362 ++
>  scripts/acpi_extract_preprocess.py   |   52 +
>  scripts/update-acpi.sh               |    4 +
>  47 files changed, 15248 insertions(+), 90 deletions(-)
>  create mode 100644 hw/i386/acpi-build.c
>  create mode 100644 hw/i386/acpi-dsdt-cpu-hotplug.dsl
>  create mode 100644 hw/i386/acpi-dsdt-dbug.dsl
>  create mode 100644 hw/i386/acpi-dsdt-hpet.dsl
>  create mode 100644 hw/i386/acpi-dsdt-isa.dsl
>  create mode 100644 hw/i386/acpi-dsdt-pci-crs.dsl
>  create mode 100644 hw/i386/acpi-dsdt.dsl
>  create mode 100644 hw/i386/acpi-dsdt.hex.generated
>  create mode 100644 hw/i386/bios-linker-loader.c
>  create mode 100644 hw/i386/q35-acpi-dsdt.dsl
>  create mode 100644 hw/i386/q35-acpi-dsdt.hex.generated
>  create mode 100644 hw/i386/ssdt-misc.dsl
>  create mode 100644 hw/i386/ssdt-misc.hex.generated
>  create mode 100644 hw/i386/ssdt-pcihp.dsl
>  create mode 100644 hw/i386/ssdt-pcihp.hex.generated
>  create mode 100644 hw/i386/ssdt-proc.dsl
>  create mode 100644 hw/i386/ssdt-proc.hex.generated
>  create mode 100644 include/hw/i386/acpi-build.h
>  create mode 100644 include/hw/i386/bios-linker-loader.h
>  create mode 100755 scripts/acpi_extract.py
>  create mode 100755 scripts/acpi_extract_preprocess.py
>  create mode 100644 scripts/update-acpi.sh
>
> -- 
> MST