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

[Michael S. Tsirkin]

On Wed, Oct 02, 2013 at 03:52:17PM +0200, Igor Mammedov wrote:
> On Wed, 2 Oct 2013 16:30:36 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Wed, Oct 02, 2013 at 03:05:52PM +0200, Igor Mammedov wrote:
> > > On Wed, 2 Oct 2013 00:26:11 +0300
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > 
> > > > This code can also be found here:
> > > > git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git acpi
> > > > 
> > > > While this patch still uses info not available in QOM, I think it's 
> > > > reasonable
> > > > to merge it and then refactor as QOM properties cover more ground.
> > > > 
> > > > In particular, merging this patchset blocks other projects so
> > > > I think its preferable to merge now and not wait
> > > > for all required QOM properties to materialize.
> > > > 
> > > > I added QOM properties in ich/piix where I knew how to
> > > > do this.
> > > > 
> > > > If you already reviewed v4 or later then not much changed.
> > > > 
> > > > Patches 1-2 are trivial style changes.
> > > > Igor posted a drop-in replacement for them: see
> > > >     [PATCH] cleanup object.h: include error.h directly
> > > > that can be used instead if it's deemed preferable,
> > > > patchset was tested with that too and works the same.
> > > > 
> > > > Patches 3-4 are QOM patches really. They are
> > > > same as the patchset I posted previously,
> > > > included here for completeness.
> > > > 
> > > > Igor suggested dropping patches 1-2 and including error.h directly.
> > > > OTOH Paolo already acked 1-4.
> > > > I think I agree with Paolo so I kept 1-2 around for now.
> > > > I hope that's ok.
> > > > They are trivial one-liners so will be easy to revert later
> > > > if we want to.
> > > > 
> > > > If everything's in order, I intend to merge this through my tree.
> > > 
> > > It breaks hotpluged CPUs ACPI tables.
> > > 
> > > steps to reproduce:
> > >  1. qemu-system-x86_64 -enable-kvm -m 1024 -monitor stdio -smp 
> > > 1,maxcpus=4 -qmp unix:/tmp/qmp-sock,server,nowait test.qcow2
> > >  2. hotadd CPU to guest: http://wiki.qemu.org/Features/CPUHotplug
> > >  3. reboot guest
> > 
> > Does it work if you *don't* reboot?
> it does, until reboot.

I see, we need to patch this info on FW CFG read too
(not on reset since bios might have onlined some CPUs).


> > 
> > >  4. check that ALL CPUs are running/visible to guest
> > > 
> > > Actual result: only initial CPUs are visible to guest
> > > (qemu) info cpus
> > > * CPU #0: pc=0x00000000000f891a thread_id=22059
> > >   CPU #1: pc=0x00000000000f6a37 (halted) thread_id=22060
> > > 
> > > should be something like:
> > > (qemu) info cpus
> > > * CPU #0: pc=0xffffffff81042086 (halted) thread_id=22059
> > >   CPU #1: pc=0xffffffff81042086 (halted) thread_id=22060
> > > 
> > > > 
> > > > Please review, and comment.
> > > > 
> > > > Changes from v6:
> > > > - fix 64 bit window bug reported by Igor
> > > > - tweak comments in error.h
> > > > 
> > > > Changes from v5:
> > > > - update generated files to fix build on systems without iasl
> > > > - fix mcfg failure reported by Gerd
> > > > Changes from v4:
> > > > - address comments by Paolo:
> > > >     rename loader interface
> > > >     reuse macro for hpet name
> > > >     better struct names
> > > >     move internal headers to hw/i386/
> > > > - fix typos resulting in bugs reported by Gerd
> > > > 
> > > > Changes from v3:
> > > > - reworked code to use QOM properties
> > > >   some info isn't yet available in QOM,
> > > >   use old-style APIs and lookups by type
> > > > - address comments by Gerd: tables are now updated
> > > >   on guest access after pci configuration
> > > > 
> > > > Changes from v2 repost:
> > > > - address comment by Anthony - convert to use APIs implemented
> > > >   using QOM
> > > > - address comment by Anthony - avoid tricky pointer path,
> > > >   use GArray from glib instead
> > > > - Address lots of comments by Hu Tao and Laszlo Ersek
> > > > 
> > > > Changes from v2:
> > > > - added missing patches to make it actually build
> > > > Changes from v1 RFC:
> > > > - added code to address cross version compatibility
> > > > - rebased to latest bits
> > > > - updated seabios code to latest bits (added pvpanic device)
> > > > 
> > > > 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.
> > > > 
> > > > This is widely desired as a way to avoid the churn
> > > > and proliferation of QEMU-specific interfaces
> > > > associated with ACPI tables in bios code.
> > > > 
> > > > In particular, this patchset (together with coreboot
> > > > support written by Gerd) allows coreboot to work correctly
> > > > with all the ACPI features that plain seabios has.
> > > > 
> > > > There's a bit of code duplication where we
> > > > already declare similar acpi structures in qemu.
> > > > 
> > > > 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, I booted a guest on qemu with and without the
> > > > change, and verified that ACPI tables are
> > > > unchanged except for trivial pointer address changes,
> > > > and the SSDT P_BLK change in the last patch.
> > > > 
> > > > Such binary compatibility makes it easier to be
> > > > confident that this change won't break things.
> > > > 
> > > > 
> > > > Michael S. Tsirkin (27):
> > > >   qemu: add Error to typedefs
> > > >   qom: pull in qemu/typedefs
> > > >   qom: cleanup struct Error references
> > > >   qom: add pointer to int property helpers
> > > >   pci: fix up w64 size calculation helper
> > > >   fw_cfg: interface to trigger callback on read
> > > >   loader: support for unmapped ROM blobs
> > > >   pcie_host: expose UNMAPPED macro
> > > >   pcie_host: expose address format
> > > >   q35: use macro for MCFG property name
> > > >   q35: expose mmcfg size as a property
> > > >   i386: add ACPI table files from seabios
> > > >   acpi: add rules to compile ASL source
> > > >   acpi: pre-compiled ASL files
> > > >   acpi: ssdt pcihp: updat generated file
> > > >   loader: use file path size from fw_cfg.h
> > > >   i386: add bios linker/loader
> > > >   loader: allow adding ROMs in done callbacks
> > > >   i386: define pc guest info
> > > >   acpi/piix: add macros for acpi property names
> > > >   piix: APIs for pc guest info
> > > >   ich9: APIs for pc guest info
> > > >   pvpanic: add API to access io port
> > > >   hpet: add API to find it
> > > >   i386: ACPI table generation code from seabios
> > > >   ssdt: fix PBLK length
> > > >   ssdt-proc: update generated file
> > > > 
> > > >  configure                           |    9 +-
> > > >  hw/i386/acpi-build.h                |    9 +
> > > >  hw/i386/acpi-defs.h                 |  331 ++
> > > >  hw/i386/bios-linker-loader.h        |   27 +
> > > >  hw/lm32/lm32_hwsetup.h              |    2 +-
> > > >  include/hw/acpi/ich9.h              |    2 +
> > > >  include/hw/acpi/piix4.h             |    8 +
> > > >  include/hw/i386/ich9.h              |    2 +
> > > >  include/hw/i386/pc.h                |   23 +
> > > >  include/hw/loader.h                 |    8 +-
> > > >  include/hw/nvram/fw_cfg.h           |    8 +-
> > > >  include/hw/pci-host/q35.h           |    2 +
> > > >  include/hw/pci/pcie_host.h          |   27 +
> > > >  include/hw/timer/hpet.h             |    2 +
> > > >  include/qapi/error.h                |    6 +-
> > > >  include/qemu/typedefs.h             |    1 +
> > > >  include/qom/object.h                |   73 +-
> > > >  hw/acpi/ich9.c                      |   24 +
> > > >  hw/acpi/piix4.c                     |   50 +-
> > > >  hw/core/loader.c                    |   31 +-
> > > >  hw/i386/acpi-build.c                | 1196 ++++++
> > > >  hw/i386/bios-linker-loader.c        |  158 +
> > > >  hw/i386/pc.c                        |   37 +
> > > >  hw/i386/pc_piix.c                   |    5 +
> > > >  hw/i386/pc_q35.c                    |    3 +
> > > >  hw/isa/lpc_ich9.c                   |   40 +
> > > >  hw/misc/pvpanic.c                   |   13 +-
> > > >  hw/nvram/fw_cfg.c                   |   33 +-
> > > >  hw/pci-host/piix.c                  |    8 +
> > > >  hw/pci-host/q35.c                   |   26 +-
> > > >  hw/pci/pci.c                        |   20 +-
> > > >  hw/pci/pcie_host.c                  |   24 -
> > > >  hw/timer/hpet.c                     |    5 +
> > > >  qom/object.c                        |   60 +
> > > >  vl.c                                |    3 +
> > > >  hw/i386/Makefile.objs               |   27 +
> > > >  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/q35-acpi-dsdt.dsl           |  452 +++
> > > >  hw/i386/q35-acpi-dsdt.hex.generated | 7346 
> > > > +++++++++++++++++++++++++++++++++++
> > > >  hw/i386/ssdt-misc.dsl               |  119 +
> > > >  hw/i386/ssdt-misc.hex.generated     |  386 ++
> > > >  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 +
> > > >  scripts/acpi_extract.py             |  362 ++
> > > >  scripts/acpi_extract_preprocess.py  |   51 +
> > > >  scripts/update-acpi.sh              |    4 +
> > > >  54 files changed, 16453 insertions(+), 85 deletions(-)
> > > >  create mode 100644 hw/i386/acpi-build.h
> > > >  create mode 100644 hw/i386/acpi-defs.h
> > > >  create mode 100644 hw/i386/bios-linker-loader.h
> > > >  create mode 100644 include/hw/acpi/piix4.h
> > > >  create mode 100644 hw/i386/acpi-build.c
> > > >  create mode 100644 hw/i386/bios-linker-loader.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/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 100755 scripts/acpi_extract.py
> > > >  create mode 100755 scripts/acpi_extract_preprocess.py
> > > >  create mode 100644 scripts/update-acpi.sh
> > > >