| Message ID | 20210617190739.3673064-1-jusual@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use ACPI PCI hot-plug for Q35 | expand |
On Thu, Jun 17, 2021 at 09:07:32PM +0200, Julia Suvorova wrote: > The patch set consists of two parts: > patches 1-4: introduce new feature > 'acpi-pci-hotplug-with-bridge-support' on Q35 > patches 5-7: make the feature default along with changes in ACPI tables > > With the feature disabled Q35 falls back to the native hot-plug. > > Pros > * no racy behavior during boot (see 110c477c2ed) > * eject is possible - according to PCIe spec, attention button > press should lead to power off, and then the adapter should be > removed manually. As there is no power down state exists in QEMU, > we cannot distinguish between an eject and a power down > request. > * no delay during deleting - after the actual power off software > must wait at least 1 second before indicating about it. This case > is quite important for users, it even has its own bug: > https://bugzilla.redhat.com/show_bug.cgi?id=1594168 > * no timer-based behavior - in addition to the previous example, > the attention button has a 5-second waiting period, during which > the operation can be canceled with a second press. While this > looks fine for manual button control, automation will result in > the need to queue or drop events, and the software receiving > events in all sort of unspecified combinations of attention/power > indicator states, which is racy and uppredictable. > * fixes or reduces the likelihood of the bugs: > * https://bugzilla.redhat.com/show_bug.cgi?id=1833187 > * https://bugzilla.redhat.com/show_bug.cgi?id=1657077 > * https://bugzilla.redhat.com/show_bug.cgi?id=1669931 > * https://bugzilla.redhat.com/show_bug.cgi?id=1678290 > > Cons: > * no access to possible features presented in slot capabilities > (this is only surprise removal AFAIK) This all looks quite nice to me. Let's give people a bit of time for review, then I'll merge. > v5: > * make sugar property on TYPE_PCIE_SLOT > instead of old TYPE_MACHINE property [Igor] > * minor style changes > v4: > * regain per-port control over hot-plug > * rebased over acpi-index changes > * set property on machine type to > make pci code more generic [Igor, Michael] > > v3: > * drop change of _OSC to allow SHPC on hotplugged bridges > * use 'acpi-root-pci-hotplug' > * add migration states [Igor] > * minor style changes > > v2: > * new ioport range for acpiphp [Gerd] > * drop find_pci_host() [Igor] > * explain magic numbers in _OSC [Igor] > * drop build_q35_pci_hotplug() wrapper [Igor] > > Julia Suvorova (7): > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 > hw/acpi/ich9: Enable ACPI PCI hot-plug > hw/pci/pcie: Do not set HPC flag if acpihp is used > bios-tables-test: Allow changes in DSDT ACPI tables > hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35 > bios-tables-test: Update golden binaries > > hw/i386/acpi-build.h | 5 +++ > include/hw/acpi/ich9.h | 5 +++ > include/hw/acpi/pcihp.h | 3 +- > include/hw/pci/pcie_port.h | 5 ++- > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > hw/acpi/pcihp.c | 22 +++++++--- > hw/acpi/piix4.c | 4 +- > hw/core/machine.c | 1 - > hw/i386/acpi-build.c | 32 ++++++++------ > hw/i386/pc.c | 1 + > hw/i386/pc_q35.c | 11 +++++ > hw/pci/pcie.c | 8 +++- > hw/pci/pcie_port.c | 1 + > tests/data/acpi/q35/DSDT | Bin 7859 -> 8289 bytes > tests/data/acpi/q35/DSDT.acpihmat | Bin 9184 -> 9614 bytes > tests/data/acpi/q35/DSDT.bridge | Bin 7877 -> 11003 bytes > tests/data/acpi/q35/DSDT.cphp | Bin 8323 -> 8753 bytes > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9513 -> 9943 bytes > tests/data/acpi/q35/DSDT.ipmibt | Bin 7934 -> 8364 bytes > tests/data/acpi/q35/DSDT.memhp | Bin 9218 -> 9648 bytes > tests/data/acpi/q35/DSDT.mmio64 | Bin 8990 -> 9419 bytes > tests/data/acpi/q35/DSDT.nohpet | Bin 7717 -> 8147 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 7865 -> 8295 bytes > tests/data/acpi/q35/DSDT.tis | Bin 8465 -> 8894 bytes > 24 files changed, 143 insertions(+), 22 deletions(-) > > -- > 2.30.2
On Thu, Jun 17, 2021 at 09:07:32PM +0200, Julia Suvorova wrote: > The patch set consists of two parts: > patches 1-4: introduce new feature > 'acpi-pci-hotplug-with-bridge-support' on Q35 > patches 5-7: make the feature default along with changes in ACPI tables > > With the feature disabled Q35 falls back to the native hot-plug. OK I had to drop this because of issues on mips and friends. Note it's not just a build failure: the API acpi_get_i386_pci_host can not be stubbed out cleanly - we need to return a pci bus. > Pros > * no racy behavior during boot (see 110c477c2ed) > * eject is possible - according to PCIe spec, attention button > press should lead to power off, and then the adapter should be > removed manually. As there is no power down state exists in QEMU, > we cannot distinguish between an eject and a power down > request. > * no delay during deleting - after the actual power off software > must wait at least 1 second before indicating about it. This case > is quite important for users, it even has its own bug: > https://bugzilla.redhat.com/show_bug.cgi?id=1594168 > * no timer-based behavior - in addition to the previous example, > the attention button has a 5-second waiting period, during which > the operation can be canceled with a second press. While this > looks fine for manual button control, automation will result in > the need to queue or drop events, and the software receiving > events in all sort of unspecified combinations of attention/power > indicator states, which is racy and uppredictable. > * fixes or reduces the likelihood of the bugs: > * https://bugzilla.redhat.com/show_bug.cgi?id=1833187 > * https://bugzilla.redhat.com/show_bug.cgi?id=1657077 > * https://bugzilla.redhat.com/show_bug.cgi?id=1669931 > * https://bugzilla.redhat.com/show_bug.cgi?id=1678290 > > Cons: > * no access to possible features presented in slot capabilities > (this is only surprise removal AFAIK) > > v5: > * make sugar property on TYPE_PCIE_SLOT > instead of old TYPE_MACHINE property [Igor] > * minor style changes > v4: > * regain per-port control over hot-plug > * rebased over acpi-index changes > * set property on machine type to > make pci code more generic [Igor, Michael] > > v3: > * drop change of _OSC to allow SHPC on hotplugged bridges > * use 'acpi-root-pci-hotplug' > * add migration states [Igor] > * minor style changes > > v2: > * new ioport range for acpiphp [Gerd] > * drop find_pci_host() [Igor] > * explain magic numbers in _OSC [Igor] > * drop build_q35_pci_hotplug() wrapper [Igor] > > Julia Suvorova (7): > hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 > hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 > hw/acpi/ich9: Enable ACPI PCI hot-plug > hw/pci/pcie: Do not set HPC flag if acpihp is used > bios-tables-test: Allow changes in DSDT ACPI tables > hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35 > bios-tables-test: Update golden binaries > > hw/i386/acpi-build.h | 5 +++ > include/hw/acpi/ich9.h | 5 +++ > include/hw/acpi/pcihp.h | 3 +- > include/hw/pci/pcie_port.h | 5 ++- > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > hw/acpi/pcihp.c | 22 +++++++--- > hw/acpi/piix4.c | 4 +- > hw/core/machine.c | 1 - > hw/i386/acpi-build.c | 32 ++++++++------ > hw/i386/pc.c | 1 + > hw/i386/pc_q35.c | 11 +++++ > hw/pci/pcie.c | 8 +++- > hw/pci/pcie_port.c | 1 + > tests/data/acpi/q35/DSDT | Bin 7859 -> 8289 bytes > tests/data/acpi/q35/DSDT.acpihmat | Bin 9184 -> 9614 bytes > tests/data/acpi/q35/DSDT.bridge | Bin 7877 -> 11003 bytes > tests/data/acpi/q35/DSDT.cphp | Bin 8323 -> 8753 bytes > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9513 -> 9943 bytes > tests/data/acpi/q35/DSDT.ipmibt | Bin 7934 -> 8364 bytes > tests/data/acpi/q35/DSDT.memhp | Bin 9218 -> 9648 bytes > tests/data/acpi/q35/DSDT.mmio64 | Bin 8990 -> 9419 bytes > tests/data/acpi/q35/DSDT.nohpet | Bin 7717 -> 8147 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 7865 -> 8295 bytes > tests/data/acpi/q35/DSDT.tis | Bin 8465 -> 8894 bytes > 24 files changed, 143 insertions(+), 22 deletions(-) > > -- > 2.30.2
Hi Julia, On Thu, Jun 17, 2021 at 10:07 PM Julia Suvorova <jusual@redhat.com> wrote: > The patch set consists of two parts: > patches 1-4: introduce new feature > 'acpi-pci-hotplug-with-bridge-support' on Q35 > patches 5-7: make the feature default along with changes in ACPI tables > > I have tested the patches and I noticed 2 issues: 1. The changes allow to hotplug a device into the pcie.0 bus (device_add e1000) - The device can be seen using `info qtree` but is not visible in guest. pcie.0 bus should not support hotplug at all. Before the series we get: Error: Bus 'pcie.0' does not support hotplugging 2. The devices hotplugged into a pcie-root-port are not allocated IO space: device_add e1000,id=e1,bus=rp1 In dmesg I see: [ 24.699178] pci 0000:01:00.0: BAR 1: no space for [io size 0x0040] [ 24.699753] pci 0000:01:00.0: BAR 1: failed to assign [io size 0x0040] And in lspci I see: I/O ports at <unassigned> [disabled] Before the series: I/O ports at 1000 [size=64] Could you please check? Thanks, Marcel
The patch set consists of two parts: patches 1-4: introduce new feature 'acpi-pci-hotplug-with-bridge-support' on Q35 patches 5-7: make the feature default along with changes in ACPI tables With the feature disabled Q35 falls back to the native hot-plug. Pros * no racy behavior during boot (see 110c477c2ed) * eject is possible - according to PCIe spec, attention button press should lead to power off, and then the adapter should be removed manually. As there is no power down state exists in QEMU, we cannot distinguish between an eject and a power down request. * no delay during deleting - after the actual power off software must wait at least 1 second before indicating about it. This case is quite important for users, it even has its own bug: https://bugzilla.redhat.com/show_bug.cgi?id=1594168 * no timer-based behavior - in addition to the previous example, the attention button has a 5-second waiting period, during which the operation can be canceled with a second press. While this looks fine for manual button control, automation will result in the need to queue or drop events, and the software receiving events in all sort of unspecified combinations of attention/power indicator states, which is racy and uppredictable. * fixes or reduces the likelihood of the bugs: * https://bugzilla.redhat.com/show_bug.cgi?id=1833187 * https://bugzilla.redhat.com/show_bug.cgi?id=1657077 * https://bugzilla.redhat.com/show_bug.cgi?id=1669931 * https://bugzilla.redhat.com/show_bug.cgi?id=1678290 Cons: * no access to possible features presented in slot capabilities (this is only surprise removal AFAIK) v5: * make sugar property on TYPE_PCIE_SLOT instead of old TYPE_MACHINE property [Igor] * minor style changes v4: * regain per-port control over hot-plug * rebased over acpi-index changes * set property on machine type to make pci code more generic [Igor, Michael] v3: * drop change of _OSC to allow SHPC on hotplugged bridges * use 'acpi-root-pci-hotplug' * add migration states [Igor] * minor style changes v2: * new ioport range for acpiphp [Gerd] * drop find_pci_host() [Igor] * explain magic numbers in _OSC [Igor] * drop build_q35_pci_hotplug() wrapper [Igor] Julia Suvorova (7): hw/acpi/pcihp: Enhance acpi_pcihp_disable_root_bus() to support Q35 hw/i386/acpi-build: Add ACPI PCI hot-plug methods to Q35 hw/acpi/ich9: Enable ACPI PCI hot-plug hw/pci/pcie: Do not set HPC flag if acpihp is used bios-tables-test: Allow changes in DSDT ACPI tables hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35 bios-tables-test: Update golden binaries hw/i386/acpi-build.h | 5 +++ include/hw/acpi/ich9.h | 5 +++ include/hw/acpi/pcihp.h | 3 +- include/hw/pci/pcie_port.h | 5 ++- hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ hw/acpi/pcihp.c | 22 +++++++--- hw/acpi/piix4.c | 4 +- hw/core/machine.c | 1 - hw/i386/acpi-build.c | 32 ++++++++------ hw/i386/pc.c | 1 + hw/i386/pc_q35.c | 11 +++++ hw/pci/pcie.c | 8 +++- hw/pci/pcie_port.c | 1 + tests/data/acpi/q35/DSDT | Bin 7859 -> 8289 bytes tests/data/acpi/q35/DSDT.acpihmat | Bin 9184 -> 9614 bytes tests/data/acpi/q35/DSDT.bridge | Bin 7877 -> 11003 bytes tests/data/acpi/q35/DSDT.cphp | Bin 8323 -> 8753 bytes tests/data/acpi/q35/DSDT.dimmpxm | Bin 9513 -> 9943 bytes tests/data/acpi/q35/DSDT.ipmibt | Bin 7934 -> 8364 bytes tests/data/acpi/q35/DSDT.memhp | Bin 9218 -> 9648 bytes tests/data/acpi/q35/DSDT.mmio64 | Bin 8990 -> 9419 bytes tests/data/acpi/q35/DSDT.nohpet | Bin 7717 -> 8147 bytes tests/data/acpi/q35/DSDT.numamem | Bin 7865 -> 8295 bytes tests/data/acpi/q35/DSDT.tis | Bin 8465 -> 8894 bytes 24 files changed, 143 insertions(+), 22 deletions(-)