Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host

[Ani Sinha]

On Thu, 29 Jul 2021, Ani Sinha wrote:

>
>
> On Wed, 28 Jul 2021, Michael S. Tsirkin wrote:
>
> > On Mon, Jul 26, 2021 at 10:27:43PM +0530, Ani Sinha wrote:
> > > All existing code using acpi_get_i386_pci_host() checks for a non-null
> > > return value from this function call. Instead of returning early when the 
> > > value
> > > returned is NULL, assert instead. Since there are only two possible host 
> > > buses
> > > for i386 - q35 and i440fx, a null value return from the function does not 
> > > make
> > > sense in most cases and is likely an error situation.
> >
> > add "on i386"?
> >
> > > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >
> > This that seems inappropriate, this is not a bugfix.
> >
>
> Forgot to answer this. I started this patch because I saw a gap that was
> introduced with the above patch. In acpi_pcihp_disable_root_bus(), Julia's
> code did not check for null return value from acpi_get_i386_pci_host().
> See v2. Hence, I added the fixes tag. Then Igor suggested that I assert
> instead and I also thought perhaps assertion is a better idea. Hence v3. I
> am not conflicted after reading your argument. We should assert only when
> a certain invariant is always respected. Otherwise we should not assert.
> If you think acpi_get_i386_pci_host() can be called from non-i386 path as
> well, maybe v2 approach is better.

Also I should point out that at this moment, only ich9 and piix4 end up
calling acpi_pcihp_disable_root_bus(). Hence, we are ok either way for
now. In the future, if other archs end of calling this function, then the
question is, do we gracefully fail by simply returning in case of null
host bridge or do we assert? In its current form, it will ungracefully
crash somewhere.