Re: [Qemu-devel] [PULL 00/43] pci, pc, acpi fixes, enhancements

[Anthony Liguori]

On Mon, Oct 14, 2013 at 10:28 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Mon, Oct 14, 2013 at 03:42:37PM -0700, Anthony Liguori wrote:
>> "Michael S. Tsirkin" <address@hidden> writes:
>>
>> > Anthony, I know you wanted to review some of the patches,
>> > since you didn't respond either all's well or you
>> > could not find the time.
>> > I think we are better off merging them for 1.7 and then - worst case,
>> > if major issues surface - disabling the functionality at the last minute
>> > than delaying the merge even more.
>>
>> There is no way I'll pull this for 1.7.  Changes like this aren't going
>> to get merged at the last minute.
>
> Last minute?  This has been on list for months.

It doesn't matter how long the patches have been on the list.  We have
a very short testing cycle for releases.

This pull request lacks any automated testing.  Something like this
really should come with at least some qtest validation that we are
still generating the right ACPI tables but certainly could have
simpler unit tests too.

There is no statement about what manual testing you actually did.
Have you run kvm autotest?  Have you tested a variety of Windows
guests?

The pull request has a patch with a binary diff and a comment of:

"update generated file, not sure what changed"

And that didn't concern you prior to sending the pull request?

This series is not ready to merge.

>>  A good chunk of the series lacks
>> any Reviewed-bys including the actual hotplug behind a pci bridge bits
>> which is the whole point of the series.
>
> It isn't. The point is getting ACPI out of seabios.
> OK what if I drop the bridge hotplug part?

How does that address the above?

>> This is a huge series and I still am not convinced this is the right
>> path forward.  The alternative to this series is a small set of changes
>> to SeaBIOS to support PCI bridge hotplug, no?
>
> No, we also get alternative firmwares working correctly with QEMU.
>
>> Or 10k SLOC of code into QEMU that includes breaking migration
>> compatibility.
>
> AFAIK it doesn't break migration compatibility.

>From 41/43:

"The interface is actually backwards-compatible with
 existing PIIX4 ACPI (though not migration compatible)."

And does "AFAIK" translate to, "I have tested migration from new and
old and old and new with this series"?  I suspect the answer is no.

Regards,

Anthony Liguori

>> Regards,
>>
>> Anthony Liguori
>>
>> > The following changes since commit 
>> > e26d3e734650640fabd7d95ace4f3a6f88725e0b:
>> >
>> >   smbios: Factor out smbios_maybe_add_str() (2013-09-28 23:49:39 +0300)
>> >
>> > are available in the git repository at:
>> >
>> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony
>> >
>> > for you to fetch changes up to 6cab1e7000021fa6a487f67e1dba986f68fee30d:
>> >
>> >   acpi-build: enable hotplug for PCI bridges (2013-10-14 17:48:58 +0300)
>> >
>> > ----------------------------------------------------------------
>> > pci, pc, acpi fixes, enhancements
>> >
>> > This includes some pretty big changes:
>> > - pci master abort support by Marcel
>> > - pci IRQ API rework by Marcel
>> > - acpi generation and pci bridge hotplug support by myself
>> >
>> > Everything has gone through several revisions, latest versions have been on
>> > list for a while without any more comments, tested by several
>> > people.
>> >
>> > Please pull for 1.7.
>> >
>> > Signed-off-by: Michael S. Tsirkin <address@hidden>
>> >
>> > ----------------------------------------------------------------
>> > Igor Mammedov (1):
>> >       cleanup object.h: include error.h directly
>> >
>> > Marcel Apfelbaum (11):
>> >       memory: Change MemoryRegion priorities from unsigned to signed
>> >       docs/memory: Explictly state that MemoryRegion priority is signed
>> >       hw/pci: partially handle pci master abort
>> >       hw/core: Add interface to allocate and free a single IRQ
>> >       hw/pci: add pci wrappers for allocating and asserting irqs
>> >       hw/pci-bridge: set PCI_INTERRUPT_PIN register before shpc init
>> >       hw/vmxnet3: set interrupts using pci irq wrappers
>> >       hw/vfio: set interrupts using pci irq wrappers
>> >       hw: set interrupts using pci irq wrappers
>> >       hw/pcie: AER and hot-plug events must use device's interrupt
>> >       hw/pci: removed irq field from PCIDevice
>> >
>> > Michael S. Tsirkin (31):
>> >       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
>> >       acpi: add interface to access user-installed tables
>> >       pc: use new api to add builtin tables
>> >       i386: ACPI table generation code from seabios
>> >       ssdt: fix PBLK length
>> >       ssdt-proc: update generated file
>> >       pci: add pci_for_each_bus_depth_first
>> >       pcihp: generalization of piix4 acpi
>> >       piix4: add acpi pci hotplug support
>> >       acpi-build: enable hotplug for PCI bridges
>> >
>> >  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/exec/memory.h               |    4 +-
>> >  include/hw/acpi/acpi.h              |    4 +
>> >  include/hw/acpi/ich9.h              |    2 +
>> >  include/hw/acpi/pcihp.h             |   72 +
>> >  include/hw/acpi/piix4.h             |    8 +
>> >  include/hw/i386/ich9.h              |    2 +
>> >  include/hw/i386/pc.h                |   27 +
>> >  include/hw/irq.h                    |    7 +
>> >  include/hw/loader.h                 |    8 +-
>> >  include/hw/nvram/fw_cfg.h           |    8 +-
>> >  include/hw/pci-host/q35.h           |    2 +
>> >  include/hw/pci/pci.h                |   40 +-
>> >  include/hw/pci/pci_bus.h            |    1 +
>> >  include/hw/pci/pcie.h               |   18 -
>> >  include/hw/pci/pcie_host.h          |   27 +
>> >  include/hw/sysbus.h                 |    2 +-
>> >  include/hw/timer/hpet.h             |    2 +
>> >  include/qom/object.h                |   73 +-
>> >  hw/acpi/core.c                      |   40 +
>> >  hw/acpi/ich9.c                      |   24 +
>> >  hw/acpi/pcihp.c                     |  312 ++
>> >  hw/acpi/piix4.c                     |  125 +-
>> >  hw/audio/ac97.c                     |    4 +-
>> >  hw/audio/es1370.c                   |    4 +-
>> >  hw/audio/intel-hda.c                |    2 +-
>> >  hw/block/nvme.c                     |    2 +-
>> >  hw/char/serial-pci.c                |    5 +-
>> >  hw/char/tpci200.c                   |    8 +-
>> >  hw/core/irq.c                       |   16 +
>> >  hw/core/loader.c                    |   31 +-
>> >  hw/core/sysbus.c                    |    4 +-
>> >  hw/display/qxl.c                    |    2 +-
>> >  hw/i386/acpi-build.c                | 1420 +++++++
>> >  hw/i386/bios-linker-loader.c        |  158 +
>> >  hw/i386/pc.c                        |   25 +-
>> >  hw/i386/pc_piix.c                   |    5 +
>> >  hw/i386/pc_q35.c                    |    3 +
>> >  hw/ide/cmd646.c                     |    2 +-
>> >  hw/ide/ich.c                        |    3 +-
>> >  hw/isa/lpc_ich9.c                   |   40 +
>> >  hw/isa/vt82c686.c                   |    2 +-
>> >  hw/misc/ivshmem.c                   |    2 +-
>> >  hw/misc/pvpanic.c                   |   13 +-
>> >  hw/misc/vfio.c                      |   11 +-
>> >  hw/net/e1000.c                      |    2 +-
>> >  hw/net/eepro100.c                   |    4 +-
>> >  hw/net/ne2000.c                     |    3 +-
>> >  hw/net/pcnet-pci.c                  |    3 +-
>> >  hw/net/rtl8139.c                    |    2 +-
>> >  hw/net/vmxnet3.c                    |   13 +-
>> >  hw/nvram/fw_cfg.c                   |   33 +-
>> >  hw/pci-bridge/pci_bridge_dev.c      |    2 +-
>> >  hw/pci-host/piix.c                  |    8 +
>> >  hw/pci-host/q35.c                   |   26 +-
>> >  hw/pci/pci.c                        |  100 +-
>> >  hw/pci/pcie.c                       |    4 +-
>> >  hw/pci/pcie_aer.c                   |    4 +-
>> >  hw/pci/pcie_host.c                  |   24 -
>> >  hw/pci/shpc.c                       |    2 +-
>> >  hw/scsi/esp-pci.c                   |    3 +-
>> >  hw/scsi/lsi53c895a.c                |    2 +-
>> >  hw/scsi/megasas.c                   |    6 +-
>> >  hw/scsi/vmw_pvscsi.c                |    2 +-
>> >  hw/timer/hpet.c                     |    5 +
>> >  hw/usb/hcd-ehci-pci.c               |    2 +-
>> >  hw/usb/hcd-ohci.c                   |    2 +-
>> >  hw/usb/hcd-uhci.c                   |    6 +-
>> >  hw/usb/hcd-xhci.c                   |    7 +-
>> >  hw/virtio/virtio-pci.c              |    4 +-
>> >  memory.c                            |    4 +-
>> >  qom/object.c                        |   60 +
>> >  vl.c                                |    3 +
>> >  docs/memory.txt                     |    4 +
>> >  hw/acpi/Makefile.objs               |    2 +-
>> >  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               |  341 ++
>> >  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              |   50 +
>> >  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 +
>> >  98 files changed, 17366 insertions(+), 183 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/pcihp.h
>> >  create mode 100644 include/hw/acpi/piix4.h
>> >  create mode 100644 hw/acpi/pcihp.c
>> >  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