Re: [PATCH v4 14/16] hw/acpi/pcihp.c: convert ACPI_PCIHP_PROP_BSEL from object prop to class prop

[Markus Armbruster <armbru@redhat.com>]

Mark Cave-Ayland <mark.caveayland@nutanix.com> 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 <mark.caveayland@nutanix.com>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>   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?

[...]