On 25/08/2026 08:17, Markus Armbruster wrote:
Mark Cave-Ayland <[email protected]> writes:
On 24/08/2026 14:04, Daniel P. Berrangé wrote:
On Fri, Aug 21, 2026 at 12:14:26PM +0100, Mark Cave-Ayland wrote:
Move the ACPI_PCIHP_PROP_BSEL property to the PCIBus object and update all
callers accordingly.
Since the existing logic checks for the existence of the ACPI_PCIHP_PROP_BSEL
property to enable the relevant ACPI changes, set the type of the underlying
variable to uint32_t with a default value of UINT32_MAX indicating that the
property has not been set.
Signed-off-by: Mark Cave-Ayland <[email protected]>
Reviewed-by: Marc-André Lureau <[email protected]>
---
include/hw/pci/pci_bus.h | 2 ++
hw/acpi/pci-bridge.c | 9 ++++++++-
hw/acpi/pcihp.c | 30 ++++++++++++++----------------
hw/arm/virt-acpi-build.c | 7 ++++++-
hw/i386/acpi-build.c | 7 ++++++-
hw/pci/pci.c | 7 +++++++
6 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index c738446788..7ecb23d13c 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -56,6 +56,8 @@ struct PCIBus {
int *irq_count;
Notifier machine_done;
+
+ uint32_t acpi_pcihp_bsel_val;
};
static inline bool pci_bus_is_cxl(PCIBus *bus)
diff --git a/hw/acpi/pci-bridge.c b/hw/acpi/pci-bridge.c
index 394a919479..1bfc5431f2 100644
--- a/hw/acpi/pci-bridge.c
+++ b/hw/acpi/pci-bridge.c
@@ -23,6 +23,8 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
@@ -23,6 +23,8 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
if (!DEVICE(br)->hotplugged) {
PCIBus *sec_bus = pci_bridge_get_sec_bus(br);
+ Error *local_err = NULL;
+ uint32_t bsel;
build_append_pci_bus_devices(scope, sec_bus);
@@ -30,9 +32,14 @@ void build_pci_bridge_aml(AcpiDevAmlIf *adev, Aml *scope)
* generate hotplug slots descriptors if
* bridge has ACPI PCI hotplug attached,
*/
- if (object_property_find(OBJECT(sec_bus), ACPI_PCIHP_PROP_BSEL)) {
+ bsel = object_property_get_uint(OBJECT(sec_bus), ACPI_PCIHP_PROP_BSEL,
+ &local_err);
+
+ if (local_err == NULL && bsel != UINT32_MAX) {
build_append_pcihp_slots(scope, sec_bus);
}
+
+ error_free(local_err);
IIUC, in this new approach the property should always exist and so I'd
expect object_property_get_uint to always succeed.
IOW, is something stopping us using &error_abort and merely checking
if (bsel != UINT32_MAX)
?
I think that would work, although in the back of my mind I have a memory that
we should try avoiding error_abort where at all possible.
Markus, any thoughts/comments on error_abort usage?
&error_abort is an assertion, just like assert().
Assertions are strictly for programming errors. When the program
detects such an error, it is defective and needs fixing.
Commonly, the program is in an invalid state then, and continuing
execution is unsafe. Aborting it is the only sane option.
Assertions conveniently combine the error check and the abort, and also
scream "this is a a programming error" both to readers of the code and
unfortunate users tripping them. Good!
I particularly dislike the pattern
if (programming error detected) {
error recovery that should never be reached
}
where the code doesn't actually *tell* me it's a programming error.
This matters! The error recovery needs to be treated with extreme
suspicion then, because it's not testable and almost certainly unsafe.
The pattern can also make me go "huh, I wonder how this error can
happen, let me figure that out", wasting my time.
Another gripe of mine: we often pass NULL where we should pass
&error_abort.
Questions?
Always :) Seriously though, for this particular case I think
error_abort is reasonable since if you've managed to get a non-PCIBus
from pci_bridge_get_sec_bus() then things are generally looking pretty bad.
Thanks for the pointers as it's something else to add to my set of notes
as to how we should best model these things in QEMU. I wonder if there
is a suitable place in the documentation we could add this?
ATB,
Mark.
