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

[Ani Sinha <ani@anisinha.ca>]

ping ...

On Thu, 29 Jul 2021, Ani Sinha wrote:

>
>
> 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.
>
>