| Message ID | 20200924070013.165026-1-jusual@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use ACPI PCI hot-plug for Q35 | expand |
Patchew URL: https://patchew.org/QEMU/20200924070013.165026-1-jusual@redhat.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===
Host machine cpu: x86_64
Target machine cpu family: x86
Target machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
The manual pages are in docs.
/usr/lib/gcc/x86_64-w64-mingw32/9.2.1/../../../../x86_64-w64-mingw32/bin/ld: libcommon.fa.p/hw_pci_pcie.c.obj: in function `acpi_pcihp_enabled':
/tmp/qemu-test/build/../src/hw/pci/pcie.c:522: undefined reference to `object_resolve_type_unambiguous'
collect2: error: ld returned 1 exit status
make: *** [Makefile.ninja:1878: qemu-system-aarch64w.exe] Error 1
make: *** Waiting for unfinished jobs....
/usr/lib/gcc/x86_64-w64-mingw32/9.2.1/../../../../x86_64-w64-mingw32/bin/ld: libcommon.fa.p/hw_pci_pcie.c.obj: in function `acpi_pcihp_enabled':
/tmp/qemu-test/build/../src/hw/pci/pcie.c:522: undefined reference to `object_resolve_type_unambiguous'
collect2: error: ld returned 1 exit status
make: *** [Makefile.ninja:1876: qemu-system-aarch64.exe] Error 1
Traceback (most recent call last):
File "./tests/docker/docker.py", line 709, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=342bc147a50847f886da9b01c0ca5ec9', '-u', '1003', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-vmsaoknh/src/docker-src.2020-09-24-04.53.10.30016:/var/tmp/qemu:z,ro', 'qemu/fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=342bc147a50847f886da9b01c0ca5ec9
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-vmsaoknh/src'
make: *** [docker-run-test-mingw@fedora] Error 2
real 4m42.837s
user 0m18.829s
The full log is available at
http://patchew.org/logs/20200924070013.165026-1-jusual@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20200924070013.165026-1-jusual@redhat.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
C linker for the host machine: cc ld.bfd 2.27-43
Host machine cpu family: x86_64
Host machine cpu: x86_64
../src/meson.build:10: WARNING: Module unstable-keyval has no backwards or forwards compatibility and might not exist in future releases.
Program sh found: YES
Program python3 found: YES (/usr/bin/python3)
Configuring ninjatool using configuration
---
Linking target tests/test-replication
libcommon.fa.p/hw_pci_pcie.c.o: In function `acpi_pcihp_enabled':
/tmp/qemu-test/build/../src/hw/pci/pcie.c:522: undefined reference to `object_resolve_type_unambiguous'
collect2: error: ld returned 1 exit status
make: *** [qemu-system-aarch64] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 709, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--rm', '--label', 'com.qemu.instance.uuid=38644690a9934226b7e7108ea4390530', '-u', '1003', '--security-opt', 'seccomp=unconfined', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-2jqqex0j/src/docker-src.2020-09-24-04.59.06.4662:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=38644690a9934226b7e7108ea4390530
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-2jqqex0j/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 4m43.237s
user 0m20.603s
The full log is available at
http://patchew.org/logs/20200924070013.165026-1-jusual@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Thu, Sep 24, 2020 at 09:00:06AM +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 > > This way maintainers can decide which way to choose without breaking > the patch set. > > 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: > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > Cons: > * lose per-port control over hot-plug (can be resolved) > * no access to possible features presented in slot capabilities > (this is only surprise removal AFAIK) something I don't quite get is whether with this one can still add new pcie bridges and then hotplug into these with native hotplug. the challenge to answering this with certainty is that right now qemu does not allow hotplugging complex devices such as bridges at all, we only have a hack for multifunction devices. Maybe adding a bridge as function 1 on command line, then hotplugging another device as function 0 will work to test this. > 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/pci/pcie: Do not initialize slot capability if acpihp is used > hw/acpi/ich9: Enable ACPI PCI hot-plug > bios-tables-test: Allow changes in DSDT ACPI tables > hw/acpi/ich9: Set ACPI PCI hot-plug as default > bios-tables-test: Update golden binaries > > hw/i386/acpi-build.h | 7 ++++ > include/hw/acpi/ich9.h | 5 +++ > include/hw/acpi/pcihp.h | 3 +- > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > hw/acpi/pcihp.c | 16 ++++--- > hw/acpi/piix4.c | 4 +- > hw/i386/acpi-build.c | 31 ++++++++------ > hw/i386/pc.c | 1 + > hw/pci/pcie.c | 16 +++++++ > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > 19 files changed, 129 insertions(+), 21 deletions(-) > > -- > 2.25.4
On Thu, 24 Sep 2020 09:00:06 +0200 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 > > This way maintainers can decide which way to choose without breaking > the patch set. > > 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. if I recall right, you had a idea about keeping pending removal request to distinguish between eject and poweroff (i.e. eject if mgmt asked for removal and otherwise it could be poweroff|nop) Why it didn't work out in the end? > * 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: > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > Cons: > * lose per-port control over hot-plug (can be resolved) > * no access to possible features presented in slot capabilities > (this is only surprise removal AFAIK) > > 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/pci/pcie: Do not initialize slot capability if acpihp is used > hw/acpi/ich9: Enable ACPI PCI hot-plug > bios-tables-test: Allow changes in DSDT ACPI tables > hw/acpi/ich9: Set ACPI PCI hot-plug as default > bios-tables-test: Update golden binaries > > hw/i386/acpi-build.h | 7 ++++ > include/hw/acpi/ich9.h | 5 +++ > include/hw/acpi/pcihp.h | 3 +- > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > hw/acpi/pcihp.c | 16 ++++--- > hw/acpi/piix4.c | 4 +- > hw/i386/acpi-build.c | 31 ++++++++------ > hw/i386/pc.c | 1 + > hw/pci/pcie.c | 16 +++++++ > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > 19 files changed, 129 insertions(+), 21 deletions(-) >
On Thu, Sep 24, 2020 at 11:30:00AM +0200, Igor Mammedov wrote: > On Thu, 24 Sep 2020 09:00:06 +0200 > 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 > > > > This way maintainers can decide which way to choose without breaking > > the patch set. > > > > 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. > > if I recall right, you had a idea about > keeping pending removal request to distinguish between eject and poweroff > (i.e. eject if mgmt asked for removal and otherwise it could be poweroff|nop) > Why it didn't work out in the end? For better or worse guests expect to eject and have it happen, without also asking management to do it ... > > * 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: > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > > > Cons: > > * lose per-port control over hot-plug (can be resolved) > > * no access to possible features presented in slot capabilities > > (this is only surprise removal AFAIK) > > > > 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/pci/pcie: Do not initialize slot capability if acpihp is used > > hw/acpi/ich9: Enable ACPI PCI hot-plug > > bios-tables-test: Allow changes in DSDT ACPI tables > > hw/acpi/ich9: Set ACPI PCI hot-plug as default > > bios-tables-test: Update golden binaries > > > > hw/i386/acpi-build.h | 7 ++++ > > include/hw/acpi/ich9.h | 5 +++ > > include/hw/acpi/pcihp.h | 3 +- > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > > hw/acpi/pcihp.c | 16 ++++--- > > hw/acpi/piix4.c | 4 +- > > hw/i386/acpi-build.c | 31 ++++++++------ > > hw/i386/pc.c | 1 + > > hw/pci/pcie.c | 16 +++++++ > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > > 19 files changed, 129 insertions(+), 21 deletions(-) > >
On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Sep 24, 2020 at 09:00:06AM +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 > > > > This way maintainers can decide which way to choose without breaking > > the patch set. > > > > 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: > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > > > Cons: > > * lose per-port control over hot-plug (can be resolved) > > * no access to possible features presented in slot capabilities > > (this is only surprise removal AFAIK) > > something I don't quite get is whether with this one can still add > new pcie bridges and then hotplug into these with native > hotplug. Right now I disable native if there is acpihp anywhere, but even if you enable it for hotplugged devices, native hot-plug will not work. > the challenge to answering this with certainty is that right now qemu > does not allow hotplugging complex devices such as bridges at all, > we only have a hack for multifunction devices. > Maybe adding a bridge as function 1 on command line, then hotplugging another device as > function 0 will work to test this. > > > > > 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/pci/pcie: Do not initialize slot capability if acpihp is used > > hw/acpi/ich9: Enable ACPI PCI hot-plug > > bios-tables-test: Allow changes in DSDT ACPI tables > > hw/acpi/ich9: Set ACPI PCI hot-plug as default > > bios-tables-test: Update golden binaries > > > > hw/i386/acpi-build.h | 7 ++++ > > include/hw/acpi/ich9.h | 5 +++ > > include/hw/acpi/pcihp.h | 3 +- > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > > hw/acpi/pcihp.c | 16 ++++--- > > hw/acpi/piix4.c | 4 +- > > hw/i386/acpi-build.c | 31 ++++++++------ > > hw/i386/pc.c | 1 + > > hw/pci/pcie.c | 16 +++++++ > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > > 19 files changed, 129 insertions(+), 21 deletions(-) > > > > -- > > 2.25.4 >
On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote: > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Sep 24, 2020 at 09:00:06AM +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 > > > > > > This way maintainers can decide which way to choose without breaking > > > the patch set. > > > > > > 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: > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > > > > > Cons: > > > * lose per-port control over hot-plug (can be resolved) > > > * no access to possible features presented in slot capabilities > > > (this is only surprise removal AFAIK) > > > > something I don't quite get is whether with this one can still add > > new pcie bridges and then hotplug into these with native > > hotplug. > > Right now I disable native if there is acpihp anywhere, but even if > you enable it for hotplugged devices, native hot-plug will not work. So that's a minor regression in functionality, right? Why is that the case? Because you disable it in ACPI? What if we don't? > > the challenge to answering this with certainty is that right now qemu > > does not allow hotplugging complex devices such as bridges at all, > > we only have a hack for multifunction devices. > > Maybe adding a bridge as function 1 on command line, then hotplugging another device as > > function 0 will work to test this. > > > > > > > > > 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/pci/pcie: Do not initialize slot capability if acpihp is used > > > hw/acpi/ich9: Enable ACPI PCI hot-plug > > > bios-tables-test: Allow changes in DSDT ACPI tables > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default > > > bios-tables-test: Update golden binaries > > > > > > hw/i386/acpi-build.h | 7 ++++ > > > include/hw/acpi/ich9.h | 5 +++ > > > include/hw/acpi/pcihp.h | 3 +- > > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > > > hw/acpi/pcihp.c | 16 ++++--- > > > hw/acpi/piix4.c | 4 +- > > > hw/i386/acpi-build.c | 31 ++++++++------ > > > hw/i386/pc.c | 1 + > > > hw/pci/pcie.c | 16 +++++++ > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > > > 19 files changed, 129 insertions(+), 21 deletions(-) > > > > > > -- > > > 2.25.4 > >
On Thu, Oct 1, 2020 at 17:10 Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote: > > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> > wrote: > > > > > > On Thu, Sep 24, 2020 at 09:00:06AM +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 > > > > > > > > This way maintainers can decide which way to choose without breaking > > > > the patch set. > > > > > > > > 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: > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > > > > > > > Cons: > > > > * lose per-port control over hot-plug (can be resolved) > > > > * no access to possible features presented in slot capabilities > > > > (this is only surprise removal AFAIK) > > > > > > something I don't quite get is whether with this one can still add > > > new pcie bridges and then hotplug into these with native > > > hotplug. > > > > Right now I disable native if there is acpihp anywhere, but even if > > you enable it for hotplugged devices, native hot-plug will not work. > > So that's a minor regression in functionality, right? > Why is that the case? Because you disable it in ACPI? > What if we don't? Stupid question. If both native and acpi is enabled which one does OS chose? Does this choice vary between OSes and different flavours of the same OS like Windows? > > > > the challenge to answering this with certainty is that right now qemu > > > does not allow hotplugging complex devices such as bridges at all, > > > we only have a hack for multifunction devices. > > > Maybe adding a bridge as function 1 on command line, then hotplugging > another device as > > > function 0 will work to test this. > > > > > > > > > > > > > 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/pci/pcie: Do not initialize slot capability if acpihp is used > > > > hw/acpi/ich9: Enable ACPI PCI hot-plug > > > > bios-tables-test: Allow changes in DSDT ACPI tables > > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default > > > > bios-tables-test: Update golden binaries > > > > > > > > hw/i386/acpi-build.h | 7 ++++ > > > > include/hw/acpi/ich9.h | 5 +++ > > > > include/hw/acpi/pcihp.h | 3 +- > > > > hw/acpi/ich9.c | 67 > ++++++++++++++++++++++++++++++ > > > > hw/acpi/pcihp.c | 16 ++++--- > > > > hw/acpi/piix4.c | 4 +- > > > > hw/i386/acpi-build.c | 31 ++++++++------ > > > > hw/i386/pc.c | 1 + > > > > hw/pci/pcie.c | 16 +++++++ > > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > > > > 19 files changed, 129 insertions(+), 21 deletions(-) > > > > > > > > -- > > > > 2.25.4 > > > > >
On Thu, Oct 1, 2020 at 3:02 PM Ani Sinha <ani@anisinha.ca> wrote: > > > > On Thu, Oct 1, 2020 at 17:10 Michael S. Tsirkin <mst@redhat.com> wrote: >> >> On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote: >> > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote: >> > > >> > > On Thu, Sep 24, 2020 at 09:00:06AM +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 >> > > > >> > > > This way maintainers can decide which way to choose without breaking >> > > > the patch set. >> > > > >> > > > 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: >> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 >> > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 >> > > > >> > > > Cons: >> > > > * lose per-port control over hot-plug (can be resolved) >> > > > * no access to possible features presented in slot capabilities >> > > > (this is only surprise removal AFAIK) >> > > >> > > something I don't quite get is whether with this one can still add >> > > new pcie bridges and then hotplug into these with native >> > > hotplug. >> > >> > Right now I disable native if there is acpihp anywhere, but even if >> > you enable it for hotplugged devices, native hot-plug will not work. >> >> So that's a minor regression in functionality, right? >> Why is that the case? Because you disable it in ACPI? >> What if we don't? > > > Stupid question. If both native and acpi is enabled which one does OS chose? Does this choice vary between OSes and different flavours of the same OS like Windows? It will always choose native. >> >> >> > > the challenge to answering this with certainty is that right now qemu >> > > does not allow hotplugging complex devices such as bridges at all, >> > > we only have a hack for multifunction devices. >> > > Maybe adding a bridge as function 1 on command line, then hotplugging another device as >> > > function 0 will work to test this. >> > > >> > > >> > > >> > > > 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/pci/pcie: Do not initialize slot capability if acpihp is used >> > > > hw/acpi/ich9: Enable ACPI PCI hot-plug >> > > > bios-tables-test: Allow changes in DSDT ACPI tables >> > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default >> > > > bios-tables-test: Update golden binaries >> > > > >> > > > hw/i386/acpi-build.h | 7 ++++ >> > > > include/hw/acpi/ich9.h | 5 +++ >> > > > include/hw/acpi/pcihp.h | 3 +- >> > > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ >> > > > hw/acpi/pcihp.c | 16 ++++--- >> > > > hw/acpi/piix4.c | 4 +- >> > > > hw/i386/acpi-build.c | 31 ++++++++------ >> > > > hw/i386/pc.c | 1 + >> > > > hw/pci/pcie.c | 16 +++++++ >> > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes >> > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes >> > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes >> > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes >> > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes >> > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes >> > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes >> > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes >> > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes >> > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes >> > > > 19 files changed, 129 insertions(+), 21 deletions(-) >> > > > >> > > > -- >> > > > 2.25.4 >> > > >>
On Thu, Oct 1, 2020 at 1:40 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote: > > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Thu, Sep 24, 2020 at 09:00:06AM +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 > > > > > > > > This way maintainers can decide which way to choose without breaking > > > > the patch set. > > > > > > > > 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: > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > > > > > > > Cons: > > > > * lose per-port control over hot-plug (can be resolved) > > > > * no access to possible features presented in slot capabilities > > > > (this is only surprise removal AFAIK) > > > > > > something I don't quite get is whether with this one can still add > > > new pcie bridges and then hotplug into these with native > > > hotplug. > > > > Right now I disable native if there is acpihp anywhere, but even if > > you enable it for hotplugged devices, native hot-plug will not work. > > So that's a minor regression in functionality, right? > Why is that the case? Because you disable it in ACPI? > What if we don't? I meant that I disable slot hotplug capabilities, nothing in ACPI prevents native from working. Actually, I don't see if there's any regression at all. Configurations like hot-plugging downstream port or switch to another downstream port haven't worked before, and they don't work now. I can enable native for hotplugged bridges, but that doesn't make sense, because you won't be able to hot-plug anything to it. It's not an issue of ACPI, it's PCIe behaviour. Also, native-acpi combination may seem bizarre to os (slot enumeration is independent, that's why I suggested disabling pcie slot flags). > > > the challenge to answering this with certainty is that right now qemu > > > does not allow hotplugging complex devices such as bridges at all, > > > we only have a hack for multifunction devices. > > > Maybe adding a bridge as function 1 on command line, then hotplugging another device as > > > function 0 will work to test this. > > > > > > > > > > > > > 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/pci/pcie: Do not initialize slot capability if acpihp is used > > > > hw/acpi/ich9: Enable ACPI PCI hot-plug > > > > bios-tables-test: Allow changes in DSDT ACPI tables > > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default > > > > bios-tables-test: Update golden binaries > > > > > > > > hw/i386/acpi-build.h | 7 ++++ > > > > include/hw/acpi/ich9.h | 5 +++ > > > > include/hw/acpi/pcihp.h | 3 +- > > > > hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ > > > > hw/acpi/pcihp.c | 16 ++++--- > > > > hw/acpi/piix4.c | 4 +- > > > > hw/i386/acpi-build.c | 31 ++++++++------ > > > > hw/i386/pc.c | 1 + > > > > hw/pci/pcie.c | 16 +++++++ > > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > > > > 19 files changed, 129 insertions(+), 21 deletions(-) > > > > > > > > -- > > > > 2.25.4 > > > >
On Thu, Oct 1, 2020 at 9:24 PM Julia Suvorova <jusual@redhat.com> wrote: > On Thu, Oct 1, 2020 at 1:40 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Thu, Oct 01, 2020 at 10:55:35AM +0200, Julia Suvorova wrote: > > > > On Thu, Sep 24, 2020 at 11:20 AM Michael S. Tsirkin <mst@redhat.com> > wrote: > > > > > > > > > > On Thu, Sep 24, 2020 at 09:00:06AM +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 > > > > > > > > > > > > This way maintainers can decide which way to choose without > breaking > > > > > > the patch set. > > > > > > > > > > > > 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: > > > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 > > > > > > * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 > > > > > > > > > > > > Cons: > > > > > > * lose per-port control over hot-plug (can be resolved) > > > > > > * no access to possible features presented in slot capabilities > > > > > > (this is only surprise removal AFAIK) > > > > > > > > > > something I don't quite get is whether with this one can still add > > > > > new pcie bridges and then hotplug into these with native > > > > > hotplug. > > > > > > > > Right now I disable native if there is acpihp anywhere, but even if > > > > you enable it for hotplugged devices, native hot-plug will not work. > > > > > > So that's a minor regression in functionality, right? > > > Why is that the case? Because you disable it in ACPI? > > > What if we don't? > > > > I meant that I disable slot hotplug capabilities, nothing in ACPI > > prevents native from working. Actually, I don't see if there's any > > regression at all. Configurations like hot-plugging downstream port or > > switch to another downstream port haven't worked before, and they > > don't work now. I can enable native for hotplugged bridges, but that > > doesn't make sense, because you won't be able to hot-plug anything to > > it. Why is that? It's not an issue of ACPI, it's PCIe behaviour. Also, native-acpi > > combination may seem bizarre to os (slot enumeration is independent, > > that's why I suggested disabling pcie slot flags). > > > > > > > the challenge to answering this with certainty is that right now qemu > > > > > does not allow hotplugging complex devices such as bridges at all, > > > > > we only have a hack for multifunction devices. > > > > > Maybe adding a bridge as function 1 on command line, then > hotplugging another device as > > > > > function 0 will work to test this. > > > > > > > > > > > > > > > > > > > > > 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/pci/pcie: Do not initialize slot capability if acpihp is used > > > > > > hw/acpi/ich9: Enable ACPI PCI hot-plug > > > > > > bios-tables-test: Allow changes in DSDT ACPI tables > > > > > > hw/acpi/ich9: Set ACPI PCI hot-plug as default > > > > > > bios-tables-test: Update golden binaries > > > > > > > > > > > > hw/i386/acpi-build.h | 7 ++++ > > > > > > include/hw/acpi/ich9.h | 5 +++ > > > > > > include/hw/acpi/pcihp.h | 3 +- > > > > > > hw/acpi/ich9.c | 67 > ++++++++++++++++++++++++++++++ > > > > > > hw/acpi/pcihp.c | 16 ++++--- > > > > > > hw/acpi/piix4.c | 4 +- > > > > > > hw/i386/acpi-build.c | 31 ++++++++------ > > > > > > hw/i386/pc.c | 1 + > > > > > > hw/pci/pcie.c | 16 +++++++ > > > > > > tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes > > > > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes > > > > > > tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes > > > > > > tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes > > > > > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes > > > > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes > > > > > > tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes > > > > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes > > > > > > tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes > > > > > > tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes > > > > > > 19 files changed, 129 insertions(+), 21 deletions(-) > > > > > > > > > > > > -- > > > > > > 2.25.4 > > > > > > > > > > > >
On Thu, Oct 01, 2020 at 05:54:39PM +0200, Julia Suvorova wrote: > > > Right now I disable native if there is acpihp anywhere, but even if > > > you enable it for hotplugged devices, native hot-plug will not work. > > > > So that's a minor regression in functionality, right? > > Why is that the case? Because you disable it in ACPI? > > What if we don't? > > I meant that I disable slot hotplug capabilities, nothing in ACPI > prevents native from working. Actually, I don't see if there's any > regression at all. Configurations like hot-plugging downstream port or > switch to another downstream port haven't worked before, and they > don't work now. I can enable native for hotplugged bridges, but that > doesn't make sense, because you won't be able to hot-plug anything to > it. You can do the following hack right now: 1- add an upstream port as function 1 2- add a downstream port behind it 3- add some other device (e.g. another upstream port?) as function 0 As this point both ports should be detected. Going forward we can consider support for adding ports in a hidden state (not visible to guest) so one won't need an extra function. > It's not an issue of ACPI, it's PCIe behaviour. Also, native-acpi > combination may seem bizarre to os Maybe, maybe not ... Worth testing whether this works with existing guests. > (slot enumeration is independent, > that's why I suggested disabling pcie slot flags). Yes that part makes sense imho.
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 This way maintainers can decide which way to choose without breaking the patch set. 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: * https://bugzilla.redhat.com/show_bug.cgi?id=1752465 * https://bugzilla.redhat.com/show_bug.cgi?id=1690256 Cons: * lose per-port control over hot-plug (can be resolved) * no access to possible features presented in slot capabilities (this is only surprise removal AFAIK) 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/pci/pcie: Do not initialize slot capability if acpihp is used hw/acpi/ich9: Enable ACPI PCI hot-plug bios-tables-test: Allow changes in DSDT ACPI tables hw/acpi/ich9: Set ACPI PCI hot-plug as default bios-tables-test: Update golden binaries hw/i386/acpi-build.h | 7 ++++ include/hw/acpi/ich9.h | 5 +++ include/hw/acpi/pcihp.h | 3 +- hw/acpi/ich9.c | 67 ++++++++++++++++++++++++++++++ hw/acpi/pcihp.c | 16 ++++--- hw/acpi/piix4.c | 4 +- hw/i386/acpi-build.c | 31 ++++++++------ hw/i386/pc.c | 1 + hw/pci/pcie.c | 16 +++++++ tests/data/acpi/q35/DSDT | Bin 7678 -> 7950 bytes tests/data/acpi/q35/DSDT.acpihmat | Bin 9002 -> 9274 bytes tests/data/acpi/q35/DSDT.bridge | Bin 7695 -> 9865 bytes tests/data/acpi/q35/DSDT.cphp | Bin 8141 -> 8413 bytes tests/data/acpi/q35/DSDT.dimmpxm | Bin 9331 -> 9603 bytes tests/data/acpi/q35/DSDT.ipmibt | Bin 7753 -> 8025 bytes tests/data/acpi/q35/DSDT.memhp | Bin 9037 -> 9309 bytes tests/data/acpi/q35/DSDT.mmio64 | Bin 8808 -> 9080 bytes tests/data/acpi/q35/DSDT.numamem | Bin 7684 -> 7956 bytes tests/data/acpi/q35/DSDT.tis | Bin 8283 -> 8555 bytes 19 files changed, 129 insertions(+), 21 deletions(-)