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

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

[Ani Sinha]

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.

Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/acpi/pcihp.c      |  8 ++++++++
 hw/i386/acpi-build.c | 15 ++++++---------
 2 files changed, 14 insertions(+), 9 deletions(-)

changelog:
v1: initial patch
v2: removed comment addition - that can be sent as a separate patch.
v3: added assertion for null host values for all cases except one.

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index f4d706e47d..054ee8cbc5 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
     bsel_is_set = true;
 
     if (!host) {
+        /*
+         * This function can be eventually called from
+         * qemu_devices_reset() -> acpi_pcihp_reset() even
+         * for architectures other than i386. Hence, we need
+         * to ignore null values for host here.
+         */
         return;
     }
 
@@ -136,6 +142,8 @@ static void acpi_pcihp_disable_root_bus(void)
         return;
     }
 
+    assert(host);
+
     bus = PCI_HOST_BRIDGE(host)->bus;
     if (bus) {
         /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 17836149fe..83fb1d55c0 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -321,9 +321,7 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
 
     pci_host = acpi_get_i386_pci_host();
 
-    if (!pci_host) {
-        return;
-    }
+    assert(pci_host);
 
     range_set_bounds1(hole,
                       object_property_get_uint(pci_host,
@@ -1769,9 +1767,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
 
         pci_host = acpi_get_i386_pci_host();
 
-        if (pci_host) {
-            bus = PCI_HOST_BRIDGE(pci_host)->bus;
-        }
+        assert(pci_host);
+
+        bus = PCI_HOST_BRIDGE(pci_host)->bus;
 
         if (bus) {
             Aml *scope = aml_scope("PCI0");
@@ -2389,9 +2387,8 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
     QObject *o;
 
     pci_host = acpi_get_i386_pci_host();
-    if (!pci_host) {
-        return false;
-    }
+
+    assert(pci_host);
 
     o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
     if (!o) {
-- 
2.25.1

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

[Michael S. Tsirkin]

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.

> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>


Frankly I don't see this as a useful cleanup.
assert is generally a last resort thing.

> ---
>  hw/acpi/pcihp.c      |  8 ++++++++
>  hw/i386/acpi-build.c | 15 ++++++---------
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> changelog:
> v1: initial patch
> v2: removed comment addition - that can be sent as a separate patch.
> v3: added assertion for null host values for all cases except one.
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f4d706e47d..054ee8cbc5 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
>      bsel_is_set = true;
>  
>      if (!host) {
> +        /*
> +         * This function can be eventually called from
> +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> +         * for architectures other than i386. Hence, we need

why call out i386 here? well because currently host
is only non-null for q35 and i440fx which are both i386.
all the above is not a given and we won't remember to update
the comment if we change it. Generally graceful failure
is the default or should be.



> +         * to ignore null values for host here.
> +         */
>          return;
>      }
>  
> @@ -136,6 +142,8 @@ static void acpi_pcihp_disable_root_bus(void)
>          return;
>      }
>  
> +    assert(host);
> +
>      bus = PCI_HOST_BRIDGE(host)->bus;
>      if (bus) {
>          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 17836149fe..83fb1d55c0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -321,9 +321,7 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
>  
>      pci_host = acpi_get_i386_pci_host();
>  
> -    if (!pci_host) {
> -        return;
> -    }
> +    assert(pci_host);
>  
>      range_set_bounds1(hole,
>                        object_property_get_uint(pci_host,
> @@ -1769,9 +1767,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  
>          pci_host = acpi_get_i386_pci_host();
>  
> -        if (pci_host) {
> -            bus = PCI_HOST_BRIDGE(pci_host)->bus;
> -        }
> +        assert(pci_host);
> +
> +        bus = PCI_HOST_BRIDGE(pci_host)->bus;
>  
>          if (bus) {
>              Aml *scope = aml_scope("PCI0");
> @@ -2389,9 +2387,8 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      QObject *o;
>  
>      pci_host = acpi_get_i386_pci_host();
> -    if (!pci_host) {
> -        return false;
> -    }
> +
> +    assert(pci_host);
>  
>      o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
>      if (!o) {
> -- 
> 2.25.1

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

[Ani Sinha]

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.
>
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
>
>
> Frankly I don't see this as a useful cleanup.
> assert is generally a last resort thing.
>

Igor pushed in the direction of assertion. Otherwise, see my v2.

> > ---
> >  hw/acpi/pcihp.c      |  8 ++++++++
> >  hw/i386/acpi-build.c | 15 ++++++---------
> >  2 files changed, 14 insertions(+), 9 deletions(-)
> >
> > changelog:
> > v1: initial patch
> > v2: removed comment addition - that can be sent as a separate patch.
> > v3: added assertion for null host values for all cases except one.
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index f4d706e47d..054ee8cbc5 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> >      bsel_is_set = true;
> >
> >      if (!host) {
> > +        /*
> > +         * This function can be eventually called from
> > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > +         * for architectures other than i386. Hence, we need
>
> why call out i386 here? well because currently host
> is only non-null for q35 and i440fx which are both i386.
> all the above is not a given and we won't remember to update
> the comment if we change it. Generally graceful failure
> is the default or should be.

Hmm. there is much debate to be had about graceful and unfraceful
failures :-) Some might say ungraceful failures helps to catch issues
earlier before the state is messed up.

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

[Ani Sinha]

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.

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.

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

[Ani Sinha]

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

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

[Igor Mammedov]

On Mon, 26 Jul 2021 22:27:43 +0530
Ani Sinha <ani@anisinha.ca> 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.
> 
> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/acpi/pcihp.c      |  8 ++++++++
>  hw/i386/acpi-build.c | 15 ++++++---------
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> changelog:
> v1: initial patch
> v2: removed comment addition - that can be sent as a separate patch.
> v3: added assertion for null host values for all cases except one.
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f4d706e47d..054ee8cbc5 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
>      bsel_is_set = true;
>  
>      if (!host) {
> +        /*
> +         * This function can be eventually called from
> +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> +         * for architectures other than i386. Hence, we need
> +         * to ignore null values for host here.
> +         */
>          return;
>      }

I suspect it's a MIPS target that call this code unnecessarily.
It would be better to get rid of this condition altogether.
Frr that I can suggest to make acpi_pcihp_reset() stub and
replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
for MIPS.

then a bunch of asserts/ifs won't be necessary,
just one in acpi_get_i386_pci_host() will be sufficient.

  
> @@ -136,6 +142,8 @@ static void acpi_pcihp_disable_root_bus(void)
>          return;
>      }
>  
> +    assert(host);
> +
>      bus = PCI_HOST_BRIDGE(host)->bus;
>      if (bus) {
>          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 17836149fe..83fb1d55c0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -321,9 +321,7 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
>  
>      pci_host = acpi_get_i386_pci_host();
>  
> -    if (!pci_host) {
> -        return;
> -    }
> +    assert(pci_host);
>  
>      range_set_bounds1(hole,
>                        object_property_get_uint(pci_host,
> @@ -1769,9 +1767,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  
>          pci_host = acpi_get_i386_pci_host();
>  
> -        if (pci_host) {
> -            bus = PCI_HOST_BRIDGE(pci_host)->bus;
> -        }
> +        assert(pci_host);
> +
> +        bus = PCI_HOST_BRIDGE(pci_host)->bus;
>  
>          if (bus) {
>              Aml *scope = aml_scope("PCI0");
> @@ -2389,9 +2387,8 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      QObject *o;
>  
>      pci_host = acpi_get_i386_pci_host();
> -    if (!pci_host) {
> -        return false;
> -    }
> +
> +    assert(pci_host);
>  
>      o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
>      if (!o) {

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

[Ani Sinha]

On Thu, 5 Aug 2021, Igor Mammedov wrote:

> On Mon, 26 Jul 2021 22:27:43 +0530
> Ani Sinha <ani@anisinha.ca> 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.
> >
> > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/acpi/pcihp.c      |  8 ++++++++
> >  hw/i386/acpi-build.c | 15 ++++++---------
> >  2 files changed, 14 insertions(+), 9 deletions(-)
> >
> > changelog:
> > v1: initial patch
> > v2: removed comment addition - that can be sent as a separate patch.
> > v3: added assertion for null host values for all cases except one.
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index f4d706e47d..054ee8cbc5 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> >      bsel_is_set = true;
> >
> >      if (!host) {
> > +        /*
> > +         * This function can be eventually called from
> > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > +         * for architectures other than i386. Hence, we need
> > +         * to ignore null values for host here.
> > +         */
> >          return;
> >      }
>
> I suspect it's a MIPS target that call this code unnecessarily.
> It would be better to get rid of this condition altogether.
> Frr that I can suggest to make acpi_pcihp_reset() stub and
> replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> for MIPS.
>
> then a bunch of asserts/ifs won't be necessary,
> just one in acpi_get_i386_pci_host() will be sufficient.
>

OK this is a good idea.
I can see that mips-softmmu-config-devices.h has
CONFIG_ACPI_X86 turned on for mips. This does not seem right.

The issue here is:

$ grep -R CONFIG_ACPI_X86 *
devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y

So after

-CONFIG_ACPI_X86=y
-CONFIG_PIIX4=y

(the second one is needed because after removing first one we get:

/usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
/home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
reference to `piix4_pm_init'

This is because in hw/acpi/meson.build, piix4.c is conditional on
CONFIG_ACPI_X86. )

/usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
function `gt64120_pci_set_irq':
/home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
undefined reference to `piix4_dev'
/usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
`mips_malta_init':
/home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
reference to `piix4_create'

So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?

How to handle this?

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

[Ani Sinha]

On Thu, 5 Aug 2021, Ani Sinha wrote:

>
>
> On Thu, 5 Aug 2021, Igor Mammedov wrote:
>
> > On Mon, 26 Jul 2021 22:27:43 +0530
> > Ani Sinha <ani@anisinha.ca> 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.
> > >
> > > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > ---
> > >  hw/acpi/pcihp.c      |  8 ++++++++
> > >  hw/i386/acpi-build.c | 15 ++++++---------
> > >  2 files changed, 14 insertions(+), 9 deletions(-)
> > >
> > > changelog:
> > > v1: initial patch
> > > v2: removed comment addition - that can be sent as a separate patch.
> > > v3: added assertion for null host values for all cases except one.
> > >
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index f4d706e47d..054ee8cbc5 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> > >      bsel_is_set = true;
> > >
> > >      if (!host) {
> > > +        /*
> > > +         * This function can be eventually called from
> > > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > > +         * for architectures other than i386. Hence, we need
> > > +         * to ignore null values for host here.
> > > +         */
> > >          return;
> > >      }
> >
> > I suspect it's a MIPS target that call this code unnecessarily.
> > It would be better to get rid of this condition altogether.
> > Frr that I can suggest to make acpi_pcihp_reset() stub and
> > replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> > for MIPS.
> >
> > then a bunch of asserts/ifs won't be necessary,
> > just one in acpi_get_i386_pci_host() will be sufficient.
> >
>
> OK this is a good idea.
> I can see that mips-softmmu-config-devices.h has
> CONFIG_ACPI_X86 turned on for mips. This does not seem right.
>
> The issue here is:
>
> $ grep -R CONFIG_ACPI_X86 *
> devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
>
> So after
>
> -CONFIG_ACPI_X86=y
> -CONFIG_PIIX4=y
>
> (the second one is needed because after removing first one we get:
>
> /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
> /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> reference to `piix4_pm_init'
>
> This is because in hw/acpi/meson.build, piix4.c is conditional on
> CONFIG_ACPI_X86. )
>
> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> function `gt64120_pci_set_irq':
> /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> undefined reference to `piix4_dev'
> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> `mips_malta_init':
> /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> reference to `piix4_create'
>
> So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?

Apparently this is by design:
https://qemu.readthedocs.io/en/stable/system/target-mips.html

which means mips malta will continue to use the x86 specific functions
like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
will result in a double symbol definition because CONFIG_PC is off for
mips.

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

[Ani Sinha]

On Thu, 5 Aug 2021, Ani Sinha wrote:

>
>
> On Thu, 5 Aug 2021, Ani Sinha wrote:
>
> >
> >
> > On Thu, 5 Aug 2021, Igor Mammedov wrote:
> >
> > > On Mon, 26 Jul 2021 22:27:43 +0530
> > > Ani Sinha <ani@anisinha.ca> 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.
> > > >
> > > > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> > > >
> > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > ---
> > > >  hw/acpi/pcihp.c      |  8 ++++++++
> > > >  hw/i386/acpi-build.c | 15 ++++++---------
> > > >  2 files changed, 14 insertions(+), 9 deletions(-)
> > > >
> > > > changelog:
> > > > v1: initial patch
> > > > v2: removed comment addition - that can be sent as a separate patch.
> > > > v3: added assertion for null host values for all cases except one.
> > > >
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index f4d706e47d..054ee8cbc5 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> > > >      bsel_is_set = true;
> > > >
> > > >      if (!host) {
> > > > +        /*
> > > > +         * This function can be eventually called from
> > > > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > > > +         * for architectures other than i386. Hence, we need
> > > > +         * to ignore null values for host here.
> > > > +         */
> > > >          return;
> > > >      }
> > >
> > > I suspect it's a MIPS target that call this code unnecessarily.
> > > It would be better to get rid of this condition altogether.
> > > Frr that I can suggest to make acpi_pcihp_reset() stub and
> > > replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> > > for MIPS.
> > >
> > > then a bunch of asserts/ifs won't be necessary,
> > > just one in acpi_get_i386_pci_host() will be sufficient.
> > >
> >
> > OK this is a good idea.
> > I can see that mips-softmmu-config-devices.h has
> > CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> >
> > The issue here is:
> >
> > $ grep -R CONFIG_ACPI_X86 *
> > devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> >
> > So after
> >
> > -CONFIG_ACPI_X86=y
> > -CONFIG_PIIX4=y
> >
> > (the second one is needed because after removing first one we get:
> >
> > /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
> > /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> > reference to `piix4_pm_init'
> >
> > This is because in hw/acpi/meson.build, piix4.c is conditional on
> > CONFIG_ACPI_X86. )
> >
> > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> > function `gt64120_pci_set_irq':
> > /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> > undefined reference to `piix4_dev'
> > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> > `mips_malta_init':
> > /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> > reference to `piix4_create'
> >
> > So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?
>
> Apparently this is by design:
> https://qemu.readthedocs.io/en/stable/system/target-mips.html
>
> which means mips malta will continue to use the x86 specific functions
> like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
> will result in a double symbol definition because CONFIG_PC is off for
> mips.
>

Also to be noted that there is a stub for acpi_get_i386_pci_host() which
simply returns NULL. This activates when CONFIG_PC is disabled. It is this
stub that gets called for mips and hence the check for non-null host is
needed in acpi_set_pci_info() function.

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

[Michael S. Tsirkin]

On Thu, Aug 05, 2021 at 07:42:35PM +0530, Ani Sinha wrote:
> Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> stub that gets called for mips and hence the check for non-null host is
> needed in acpi_set_pci_info() function.
> 

Frankly this is generating more discussion that it's worth imho.
IMHO these tweaks will bring little benefit.
Igor do you feel differently?

-- 
MST

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

[Igor Mammedov]

On Thu, 5 Aug 2021 18:13:21 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Aug 05, 2021 at 07:42:35PM +0530, Ani Sinha wrote:
> > Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> > simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> > stub that gets called for mips and hence the check for non-null host is
> > needed in acpi_set_pci_info() function.
> >   
> 
> Frankly this is generating more discussion that it's worth imho.
> IMHO these tweaks will bring little benefit.
> Igor do you feel differently?
replacing 'ifs' with assert is a bit cleaner, but if we are cleaning
it up we should clean up it properly without leaving unnecessary
code around.
That's why I suggested to separate x86 specific parts from piix4.
Philippe (CCed) even have reported issues ACPI parts were
causing on MIPS machines, so if someone is willing to untangle
the mess it might be worth the shot.

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

[Igor Mammedov]

On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Thu, 5 Aug 2021, Ani Sinha wrote:
> 
> >
> >
> > On Thu, 5 Aug 2021, Ani Sinha wrote:
> >  
> > >
> > >
> > > On Thu, 5 Aug 2021, Igor Mammedov wrote:
> > >  
> > > > On Mon, 26 Jul 2021 22:27:43 +0530
> > > > Ani Sinha <ani@anisinha.ca> 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.
> > > > >
> > > > > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> > > > >
> > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > > ---
> > > > >  hw/acpi/pcihp.c      |  8 ++++++++
> > > > >  hw/i386/acpi-build.c | 15 ++++++---------
> > > > >  2 files changed, 14 insertions(+), 9 deletions(-)
> > > > >
> > > > > changelog:
> > > > > v1: initial patch
> > > > > v2: removed comment addition - that can be sent as a separate patch.
> > > > > v3: added assertion for null host values for all cases except one.
> > > > >
> > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > index f4d706e47d..054ee8cbc5 100644
> > > > > --- a/hw/acpi/pcihp.c
> > > > > +++ b/hw/acpi/pcihp.c
> > > > > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> > > > >      bsel_is_set = true;
> > > > >
> > > > >      if (!host) {
> > > > > +        /*
> > > > > +         * This function can be eventually called from
> > > > > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > > > > +         * for architectures other than i386. Hence, we need
> > > > > +         * to ignore null values for host here.
> > > > > +         */
> > > > >          return;
> > > > >      }  
> > > >
> > > > I suspect it's a MIPS target that call this code unnecessarily.
> > > > It would be better to get rid of this condition altogether.
> > > > Frr that I can suggest to make acpi_pcihp_reset() stub and
> > > > replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> > > > for MIPS.
> > > >
> > > > then a bunch of asserts/ifs won't be necessary,
> > > > just one in acpi_get_i386_pci_host() will be sufficient.
> > > >  
> > >
> > > OK this is a good idea.
> > > I can see that mips-softmmu-config-devices.h has
> > > CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> > >
> > > The issue here is:
> > >
> > > $ grep -R CONFIG_ACPI_X86 *
> > > devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> > >
> > > So after
> > >
> > > -CONFIG_ACPI_X86=y
> > > -CONFIG_PIIX4=y
> > >
> > > (the second one is needed because after removing first one we get:
> > >
> > > /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
> > > /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> > > reference to `piix4_pm_init'
> > >
> > > This is because in hw/acpi/meson.build, piix4.c is conditional on
> > > CONFIG_ACPI_X86. )
> > >
> > > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> > > function `gt64120_pci_set_irq':
> > > /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> > > undefined reference to `piix4_dev'
> > > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> > > `mips_malta_init':
> > > /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> > > reference to `piix4_create'
> > >
> > > So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?  
> >
> > Apparently this is by design:
> > https://qemu.readthedocs.io/en/stable/system/target-mips.html
> >
> > which means mips malta will continue to use the x86 specific functions
> > like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
> > will result in a double symbol definition because CONFIG_PC is off for
> > mips.
> >  
> 
> Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> stub that gets called for mips and hence the check for non-null host is
> needed in acpi_set_pci_info() function.
that were half measures to deal around code that shouldn't be called,
now with pcihp being used by both pc and q35 we don't have reason to
keep around null checks modulo mips calling code that shouldn't be
called there to begin with.

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

[Ani Sinha]

On Fri, 6 Aug 2021, Igor Mammedov wrote:

> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Thu, 5 Aug 2021, Ani Sinha wrote:
> >
> > >
> > >
> > > On Thu, 5 Aug 2021, Ani Sinha wrote:
> > >
> > > >
> > > >
> > > > On Thu, 5 Aug 2021, Igor Mammedov wrote:
> > > >
> > > > > On Mon, 26 Jul 2021 22:27:43 +0530
> > > > > Ani Sinha <ani@anisinha.ca> 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.
> > > > > >
> > > > > > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> > > > > >
> > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > > > ---
> > > > > >  hw/acpi/pcihp.c      |  8 ++++++++
> > > > > >  hw/i386/acpi-build.c | 15 ++++++---------
> > > > > >  2 files changed, 14 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > changelog:
> > > > > > v1: initial patch
> > > > > > v2: removed comment addition - that can be sent as a separate patch.
> > > > > > v3: added assertion for null host values for all cases except one.
> > > > > >
> > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > > index f4d706e47d..054ee8cbc5 100644
> > > > > > --- a/hw/acpi/pcihp.c
> > > > > > +++ b/hw/acpi/pcihp.c
> > > > > > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> > > > > >      bsel_is_set = true;
> > > > > >
> > > > > >      if (!host) {
> > > > > > +        /*
> > > > > > +         * This function can be eventually called from
> > > > > > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > > > > > +         * for architectures other than i386. Hence, we need
> > > > > > +         * to ignore null values for host here.
> > > > > > +         */
> > > > > >          return;
> > > > > >      }
> > > > >
> > > > > I suspect it's a MIPS target that call this code unnecessarily.
> > > > > It would be better to get rid of this condition altogether.
> > > > > Frr that I can suggest to make acpi_pcihp_reset() stub and
> > > > > replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> > > > > for MIPS.
> > > > >
> > > > > then a bunch of asserts/ifs won't be necessary,
> > > > > just one in acpi_get_i386_pci_host() will be sufficient.
> > > > >
> > > >
> > > > OK this is a good idea.
> > > > I can see that mips-softmmu-config-devices.h has
> > > > CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> > > >
> > > > The issue here is:
> > > >
> > > > $ grep -R CONFIG_ACPI_X86 *
> > > > devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> > > >
> > > > So after
> > > >
> > > > -CONFIG_ACPI_X86=y
> > > > -CONFIG_PIIX4=y
> > > >
> > > > (the second one is needed because after removing first one we get:
> > > >
> > > > /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
> > > > /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> > > > reference to `piix4_pm_init'
> > > >
> > > > This is because in hw/acpi/meson.build, piix4.c is conditional on
> > > > CONFIG_ACPI_X86. )
> > > >
> > > > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> > > > function `gt64120_pci_set_irq':
> > > > /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> > > > undefined reference to `piix4_dev'
> > > > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> > > > `mips_malta_init':
> > > > /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> > > > reference to `piix4_create'
> > > >
> > > > So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?
> > >
> > > Apparently this is by design:
> > > https://qemu.readthedocs.io/en/stable/system/target-mips.html
> > >
> > > which means mips malta will continue to use the x86 specific functions
> > > like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
> > > will result in a double symbol definition because CONFIG_PC is off for
> > > mips.
> > >
> >
> > Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> > simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> > stub that gets called for mips and hence the check for non-null host is
> > needed in acpi_set_pci_info() function.
> that were half measures to deal around code that shouldn't be called,
> now with pcihp being used by both pc and q35 we don't have reason to
> keep around null checks modulo mips calling code that shouldn't be
> called there to begin with.

So malta mips does not need ACPI hotplug? In that case, maybe we should
not make pcihp.c dependent on CONFIG_ACPI_X86. Ideas welcome.

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

[Philippe Mathieu-Daudé]

On 8/6/21 12:52 PM, Ani Sinha wrote:
> On Fri, 6 Aug 2021, Igor Mammedov wrote:
>> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
>> Ani Sinha <ani@anisinha.ca> wrote:
>>> On Thu, 5 Aug 2021, Ani Sinha wrote:
>>>> On Thu, 5 Aug 2021, Ani Sinha wrote:
>>>>> On Thu, 5 Aug 2021, Igor Mammedov wrote:
>>>>>> On Mon, 26 Jul 2021 22:27:43 +0530
>>>>>> Ani Sinha <ani@anisinha.ca> 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.
>>>>>>>
>>>>>>> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
>>>>>>>
>>>>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>>>>>>> ---
>>>>>>>  hw/acpi/pcihp.c      |  8 ++++++++
>>>>>>>  hw/i386/acpi-build.c | 15 ++++++---------
>>>>>>>  2 files changed, 14 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> changelog:
>>>>>>> v1: initial patch
>>>>>>> v2: removed comment addition - that can be sent as a separate patch.
>>>>>>> v3: added assertion for null host values for all cases except one.
>>>>>>>
>>>>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>>>>>>> index f4d706e47d..054ee8cbc5 100644
>>>>>>> --- a/hw/acpi/pcihp.c
>>>>>>> +++ b/hw/acpi/pcihp.c
>>>>>>> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
>>>>>>>      bsel_is_set = true;
>>>>>>>
>>>>>>>      if (!host) {
>>>>>>> +        /*
>>>>>>> +         * This function can be eventually called from
>>>>>>> +         * qemu_devices_reset() -> acpi_pcihp_reset() even
>>>>>>> +         * for architectures other than i386. Hence, we need
>>>>>>> +         * to ignore null values for host here.
>>>>>>> +         */
>>>>>>>          return;
>>>>>>>      }
>>>>>>
>>>>>> I suspect it's a MIPS target that call this code unnecessarily.
>>>>>> It would be better to get rid of this condition altogether.
>>>>>> Frr that I can suggest to make acpi_pcihp_reset() stub and
>>>>>> replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
>>>>>> for MIPS.
>>>>>>
>>>>>> then a bunch of asserts/ifs won't be necessary,
>>>>>> just one in acpi_get_i386_pci_host() will be sufficient.
>>>>>>
>>>>>
>>>>> OK this is a good idea.
>>>>> I can see that mips-softmmu-config-devices.h has
>>>>> CONFIG_ACPI_X86 turned on for mips. This does not seem right.
>>>>>
>>>>> The issue here is:
>>>>>
>>>>> $ grep -R CONFIG_ACPI_X86 *
>>>>> devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
>>>>>
>>>>> So after
>>>>>
>>>>> -CONFIG_ACPI_X86=y
>>>>> -CONFIG_PIIX4=y
>>>>>
>>>>> (the second one is needed because after removing first one we get:
>>>>>
>>>>> /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
>>>>> /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
>>>>> reference to `piix4_pm_init'
>>>>>
>>>>> This is because in hw/acpi/meson.build, piix4.c is conditional on
>>>>> CONFIG_ACPI_X86. )
>>>>>
>>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
>>>>> function `gt64120_pci_set_irq':
>>>>> /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
>>>>> undefined reference to `piix4_dev'
>>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
>>>>> `mips_malta_init':
>>>>> /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
>>>>> reference to `piix4_create'
>>>>>
>>>>> So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?

PIIX, PIIX3 and PIIX4 are generic chipsets, not X86-specific.

QEMU's PIIX3 is a Frankenstein to support virtualization to a chipset
not designed for it.
If you look at it, the X86 machine use a PIIX3 but the PIIX3 doesn't
even provide an ACPI function. It appeared in the PIIX4. The kludge is
to instanciate the PIIX4.acpi from the PIIX3 and X86 ppl are happy with
it, but it makes it ugly for the other architectures.

>>>> Apparently this is by design:
>>>> https://qemu.readthedocs.io/en/stable/system/target-mips.html

What do you mean "by design"? The Malta uses a PIIX4 chipset for its
southbridge indeed.

>>>> which means mips malta will continue to use the x86 specific functions
>>>> like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
>>>> will result in a double symbol definition because CONFIG_PC is off for
>>>> mips.
>>>>
>>>
>>> Also to be noted that there is a stub for acpi_get_i386_pci_host() which
>>> simply returns NULL. This activates when CONFIG_PC is disabled. It is this
>>> stub that gets called for mips and hence the check for non-null host is
>>> needed in acpi_set_pci_info() function.
>> that were half measures to deal around code that shouldn't be called,
>> now with pcihp being used by both pc and q35 we don't have reason to
>> keep around null checks modulo mips calling code that shouldn't be
>> called there to begin with.
> 
> So malta mips does not need ACPI hotplug? In that case, maybe we should
> not make pcihp.c dependent on CONFIG_ACPI_X86. Ideas welcome.

Linux on Malta does use the ACPI features from the PIIX4.

Please dig in the archives, Igor / myself already argued enough about
this topic 2 years ago. The consensus was "yes, it is badly implemented,
but it works and we don't have time to get it cleaner, pc machine is
way more used than the malta one, so let not break the pc machines."

See:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg613194.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg690435.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg725504.html

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

[Philippe Mathieu-Daudé]

On 8/6/21 4:01 PM, Philippe Mathieu-Daudé wrote:
> On 8/6/21 12:52 PM, Ani Sinha wrote:
>> On Fri, 6 Aug 2021, Igor Mammedov wrote:
>>> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
>>> Ani Sinha <ani@anisinha.ca> wrote:
>>>> On Thu, 5 Aug 2021, Ani Sinha wrote:
>>>>> On Thu, 5 Aug 2021, Ani Sinha wrote:
>>>>>> On Thu, 5 Aug 2021, Igor Mammedov wrote:
>>>>>>> On Mon, 26 Jul 2021 22:27:43 +0530
>>>>>>> Ani Sinha <ani@anisinha.ca> 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.
>>>>>>>>
>>>>>>>> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
>>>>>>>>
>>>>>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>>>>>>>> ---
>>>>>>>>  hw/acpi/pcihp.c      |  8 ++++++++
>>>>>>>>  hw/i386/acpi-build.c | 15 ++++++---------
>>>>>>>>  2 files changed, 14 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> changelog:
>>>>>>>> v1: initial patch
>>>>>>>> v2: removed comment addition - that can be sent as a separate patch.
>>>>>>>> v3: added assertion for null host values for all cases except one.
>>>>>>>>
>>>>>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>>>>>>>> index f4d706e47d..054ee8cbc5 100644
>>>>>>>> --- a/hw/acpi/pcihp.c
>>>>>>>> +++ b/hw/acpi/pcihp.c
>>>>>>>> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
>>>>>>>>      bsel_is_set = true;
>>>>>>>>
>>>>>>>>      if (!host) {
>>>>>>>> +        /*
>>>>>>>> +         * This function can be eventually called from
>>>>>>>> +         * qemu_devices_reset() -> acpi_pcihp_reset() even
>>>>>>>> +         * for architectures other than i386. Hence, we need
>>>>>>>> +         * to ignore null values for host here.
>>>>>>>> +         */
>>>>>>>>          return;
>>>>>>>>      }
>>>>>>>
>>>>>>> I suspect it's a MIPS target that call this code unnecessarily.
>>>>>>> It would be better to get rid of this condition altogether.
>>>>>>> Frr that I can suggest to make acpi_pcihp_reset() stub and
>>>>>>> replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
>>>>>>> for MIPS.
>>>>>>>
>>>>>>> then a bunch of asserts/ifs won't be necessary,
>>>>>>> just one in acpi_get_i386_pci_host() will be sufficient.
>>>>>>>
>>>>>>
>>>>>> OK this is a good idea.
>>>>>> I can see that mips-softmmu-config-devices.h has
>>>>>> CONFIG_ACPI_X86 turned on for mips. This does not seem right.
>>>>>>
>>>>>> The issue here is:
>>>>>>
>>>>>> $ grep -R CONFIG_ACPI_X86 *
>>>>>> devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
>>>>>>
>>>>>> So after
>>>>>>
>>>>>> -CONFIG_ACPI_X86=y
>>>>>> -CONFIG_PIIX4=y
>>>>>>
>>>>>> (the second one is needed because after removing first one we get:
>>>>>>
>>>>>> /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
>>>>>> /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
>>>>>> reference to `piix4_pm_init'
>>>>>>
>>>>>> This is because in hw/acpi/meson.build, piix4.c is conditional on
>>>>>> CONFIG_ACPI_X86. )
>>>>>>
>>>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
>>>>>> function `gt64120_pci_set_irq':
>>>>>> /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
>>>>>> undefined reference to `piix4_dev'
>>>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
>>>>>> `mips_malta_init':
>>>>>> /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
>>>>>> reference to `piix4_create'
>>>>>>
>>>>>> So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?
> 
> PIIX, PIIX3 and PIIX4 are generic chipsets, not X86-specific.
> 
> QEMU's PIIX3 is a Frankenstein to support virtualization to a chipset
> not designed for it.
> If you look at it, the X86 machine use a PIIX3 but the PIIX3 doesn't
> even provide an ACPI function. It appeared in the PIIX4. The kludge is
> to instanciate the PIIX4.acpi from the PIIX3 and X86 ppl are happy with
> it, but it makes it ugly for the other architectures.
> 
>>>>> Apparently this is by design:
>>>>> https://qemu.readthedocs.io/en/stable/system/target-mips.html
> 
> What do you mean "by design"? The Malta uses a PIIX4 chipset for its
> southbridge indeed.
> 
>>>>> which means mips malta will continue to use the x86 specific functions
>>>>> like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
>>>>> will result in a double symbol definition because CONFIG_PC is off for
>>>>> mips.
>>>>>
>>>>
>>>> Also to be noted that there is a stub for acpi_get_i386_pci_host() which
>>>> simply returns NULL. This activates when CONFIG_PC is disabled. It is this
>>>> stub that gets called for mips and hence the check for non-null host is
>>>> needed in acpi_set_pci_info() function.
>>> that were half measures to deal around code that shouldn't be called,
>>> now with pcihp being used by both pc and q35 we don't have reason to
>>> keep around null checks modulo mips calling code that shouldn't be
>>> called there to begin with.
>>
>> So malta mips does not need ACPI hotplug? In that case, maybe we should
>> not make pcihp.c dependent on CONFIG_ACPI_X86. Ideas welcome.
> 
> Linux on Malta does use the ACPI features from the PIIX4.
> 
> Please dig in the archives, Igor / myself already argued enough about
> this topic 2 years ago. The consensus was "yes, it is badly implemented,
> but it works and we don't have time to get it cleaner, pc machine is
> way more used than the malta one, so let not break the pc machines."
> 
> See:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg613194.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg690435.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg725504.html

Also:
https://gitlab.com/qemu-project/qemu/-/issues/193
https://gitlab.com/qemu-project/qemu/-/issues/221

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

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 6452 bytes --]



On Fri, 6 Aug 2021, Philippe Mathieu-Daudé wrote:

> On 8/6/21 4:01 PM, Philippe Mathieu-Daudé wrote:
> > On 8/6/21 12:52 PM, Ani Sinha wrote:
> >> On Fri, 6 Aug 2021, Igor Mammedov wrote:
> >>> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
> >>> Ani Sinha <ani@anisinha.ca> wrote:
> >>>> On Thu, 5 Aug 2021, Ani Sinha wrote:
> >>>>> On Thu, 5 Aug 2021, Ani Sinha wrote:
> >>>>>> On Thu, 5 Aug 2021, Igor Mammedov wrote:
> >>>>>>> On Mon, 26 Jul 2021 22:27:43 +0530
> >>>>>>> Ani Sinha <ani@anisinha.ca> 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.
> >>>>>>>>
> >>>>>>>> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >>>>>>>>
> >>>>>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >>>>>>>> ---
> >>>>>>>>  hw/acpi/pcihp.c      |  8 ++++++++
> >>>>>>>>  hw/i386/acpi-build.c | 15 ++++++---------
> >>>>>>>>  2 files changed, 14 insertions(+), 9 deletions(-)
> >>>>>>>>
> >>>>>>>> changelog:
> >>>>>>>> v1: initial patch
> >>>>>>>> v2: removed comment addition - that can be sent as a separate patch.
> >>>>>>>> v3: added assertion for null host values for all cases except one.
> >>>>>>>>
> >>>>>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >>>>>>>> index f4d706e47d..054ee8cbc5 100644
> >>>>>>>> --- a/hw/acpi/pcihp.c
> >>>>>>>> +++ b/hw/acpi/pcihp.c
> >>>>>>>> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> >>>>>>>>      bsel_is_set = true;
> >>>>>>>>
> >>>>>>>>      if (!host) {
> >>>>>>>> +        /*
> >>>>>>>> +         * This function can be eventually called from
> >>>>>>>> +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> >>>>>>>> +         * for architectures other than i386. Hence, we need
> >>>>>>>> +         * to ignore null values for host here.
> >>>>>>>> +         */
> >>>>>>>>          return;
> >>>>>>>>      }
> >>>>>>>
> >>>>>>> I suspect it's a MIPS target that call this code unnecessarily.
> >>>>>>> It would be better to get rid of this condition altogether.
> >>>>>>> Frr that I can suggest to make acpi_pcihp_reset() stub and
> >>>>>>> replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> >>>>>>> for MIPS.
> >>>>>>>
> >>>>>>> then a bunch of asserts/ifs won't be necessary,
> >>>>>>> just one in acpi_get_i386_pci_host() will be sufficient.
> >>>>>>>
> >>>>>>
> >>>>>> OK this is a good idea.
> >>>>>> I can see that mips-softmmu-config-devices.h has
> >>>>>> CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> >>>>>>
> >>>>>> The issue here is:
> >>>>>>
> >>>>>> $ grep -R CONFIG_ACPI_X86 *
> >>>>>> devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> >>>>>>
> >>>>>> So after
> >>>>>>
> >>>>>> -CONFIG_ACPI_X86=y
> >>>>>> -CONFIG_PIIX4=y
> >>>>>>
> >>>>>> (the second one is needed because after removing first one we get:
> >>>>>>
> >>>>>> /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
> >>>>>> /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> >>>>>> reference to `piix4_pm_init'
> >>>>>>
> >>>>>> This is because in hw/acpi/meson.build, piix4.c is conditional on
> >>>>>> CONFIG_ACPI_X86. )
> >>>>>>
> >>>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> >>>>>> function `gt64120_pci_set_irq':
> >>>>>> /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> >>>>>> undefined reference to `piix4_dev'
> >>>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> >>>>>> `mips_malta_init':
> >>>>>> /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> >>>>>> reference to `piix4_create'
> >>>>>>
> >>>>>> So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?
> >
> > PIIX, PIIX3 and PIIX4 are generic chipsets, not X86-specific.
> >
> > QEMU's PIIX3 is a Frankenstein to support virtualization to a chipset
> > not designed for it.
> > If you look at it, the X86 machine use a PIIX3 but the PIIX3 doesn't
> > even provide an ACPI function. It appeared in the PIIX4. The kludge is
> > to instanciate the PIIX4.acpi from the PIIX3 and X86 ppl are happy with
> > it, but it makes it ugly for the other architectures.
> >
> >>>>> Apparently this is by design:
> >>>>> https://qemu.readthedocs.io/en/stable/system/target-mips.html
> >
> > What do you mean "by design"? The Malta uses a PIIX4 chipset for its
> > southbridge indeed.
> >
> >>>>> which means mips malta will continue to use the x86 specific functions
> >>>>> like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
> >>>>> will result in a double symbol definition because CONFIG_PC is off for
> >>>>> mips.
> >>>>>
> >>>>
> >>>> Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> >>>> simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> >>>> stub that gets called for mips and hence the check for non-null host is
> >>>> needed in acpi_set_pci_info() function.
> >>> that were half measures to deal around code that shouldn't be called,
> >>> now with pcihp being used by both pc and q35 we don't have reason to
> >>> keep around null checks modulo mips calling code that shouldn't be
> >>> called there to begin with.
> >>
> >> So malta mips does not need ACPI hotplug? In that case, maybe we should
> >> not make pcihp.c dependent on CONFIG_ACPI_X86. Ideas welcome.
> >
> > Linux on Malta does use the ACPI features from the PIIX4.
> >
> > Please dig in the archives, Igor / myself already argued enough about
> > this topic 2 years ago. The consensus was "yes, it is badly implemented,
> > but it works and we don't have time to get it cleaner, pc machine is
> > way more used than the malta one, so let not break the pc machines."
> >
> > See:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg613194.html
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg690435.html
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg725504.html
>
> Also:
> https://gitlab.com/qemu-project/qemu/-/issues/193
> https://gitlab.com/qemu-project/qemu/-/issues/221

Thanks Phil for the contexts. I will go through them. For now, should we
simply go with my v2 then?

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

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 6488 bytes --]



On Fri, 6 Aug 2021, Philippe Mathieu-Daudé wrote:

> On 8/6/21 12:52 PM, Ani Sinha wrote:
> > On Fri, 6 Aug 2021, Igor Mammedov wrote:
> >> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
> >> Ani Sinha <ani@anisinha.ca> wrote:
> >>> On Thu, 5 Aug 2021, Ani Sinha wrote:
> >>>> On Thu, 5 Aug 2021, Ani Sinha wrote:
> >>>>> On Thu, 5 Aug 2021, Igor Mammedov wrote:
> >>>>>> On Mon, 26 Jul 2021 22:27:43 +0530
> >>>>>> Ani Sinha <ani@anisinha.ca> 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.
> >>>>>>>
> >>>>>>> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >>>>>>>
> >>>>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >>>>>>> ---
> >>>>>>>  hw/acpi/pcihp.c      |  8 ++++++++
> >>>>>>>  hw/i386/acpi-build.c | 15 ++++++---------
> >>>>>>>  2 files changed, 14 insertions(+), 9 deletions(-)
> >>>>>>>
> >>>>>>> changelog:
> >>>>>>> v1: initial patch
> >>>>>>> v2: removed comment addition - that can be sent as a separate patch.
> >>>>>>> v3: added assertion for null host values for all cases except one.
> >>>>>>>
> >>>>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >>>>>>> index f4d706e47d..054ee8cbc5 100644
> >>>>>>> --- a/hw/acpi/pcihp.c
> >>>>>>> +++ b/hw/acpi/pcihp.c
> >>>>>>> @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> >>>>>>>      bsel_is_set = true;
> >>>>>>>
> >>>>>>>      if (!host) {
> >>>>>>> +        /*
> >>>>>>> +         * This function can be eventually called from
> >>>>>>> +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> >>>>>>> +         * for architectures other than i386. Hence, we need
> >>>>>>> +         * to ignore null values for host here.
> >>>>>>> +         */
> >>>>>>>          return;
> >>>>>>>      }
> >>>>>>
> >>>>>> I suspect it's a MIPS target that call this code unnecessarily.
> >>>>>> It would be better to get rid of this condition altogether.
> >>>>>> Frr that I can suggest to make acpi_pcihp_reset() stub and
> >>>>>> replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> >>>>>> for MIPS.
> >>>>>>
> >>>>>> then a bunch of asserts/ifs won't be necessary,
> >>>>>> just one in acpi_get_i386_pci_host() will be sufficient.
> >>>>>>
> >>>>>
> >>>>> OK this is a good idea.
> >>>>> I can see that mips-softmmu-config-devices.h has
> >>>>> CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> >>>>>
> >>>>> The issue here is:
> >>>>>
> >>>>> $ grep -R CONFIG_ACPI_X86 *
> >>>>> devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> >>>>>
> >>>>> So after
> >>>>>
> >>>>> -CONFIG_ACPI_X86=y
> >>>>> -CONFIG_PIIX4=y
> >>>>>
> >>>>> (the second one is needed because after removing first one we get:
> >>>>>
> >>>>> /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function `piix4_create':
> >>>>> /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> >>>>> reference to `piix4_pm_init'
> >>>>>
> >>>>> This is because in hw/acpi/meson.build, piix4.c is conditional on
> >>>>> CONFIG_ACPI_X86. )
> >>>>>
> >>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> >>>>> function `gt64120_pci_set_irq':
> >>>>> /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> >>>>> undefined reference to `piix4_dev'
> >>>>> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> >>>>> `mips_malta_init':
> >>>>> /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> >>>>> reference to `piix4_create'
> >>>>>
> >>>>> So should mips be doing piix stuff anyway? Is Piix4 etc not x86 specific?
>
> PIIX, PIIX3 and PIIX4 are generic chipsets, not X86-specific.
>
> QEMU's PIIX3 is a Frankenstein to support virtualization to a chipset
> not designed for it.
> If you look at it, the X86 machine use a PIIX3 but the PIIX3 doesn't
> even provide an ACPI function. It appeared in the PIIX4. The kludge is
> to instanciate the PIIX4.acpi from the PIIX3 and X86 ppl are happy with
> it, but it makes it ugly for the other architectures.
>
> >>>> Apparently this is by design:
> >>>> https://qemu.readthedocs.io/en/stable/system/target-mips.html
>
> What do you mean "by design"? The Malta uses a PIIX4 chipset for its
> southbridge indeed.

I meant it was intentional and not by accident.

>
> >>>> which means mips malta will continue to use the x86 specific functions
> >>>> like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
> >>>> will result in a double symbol definition because CONFIG_PC is off for
> >>>> mips.
> >>>>
> >>>
> >>> Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> >>> simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> >>> stub that gets called for mips and hence the check for non-null host is
> >>> needed in acpi_set_pci_info() function.
> >> that were half measures to deal around code that shouldn't be called,
> >> now with pcihp being used by both pc and q35 we don't have reason to
> >> keep around null checks modulo mips calling code that shouldn't be
> >> called there to begin with.
> >
> > So malta mips does not need ACPI hotplug? In that case, maybe we should
> > not make pcihp.c dependent on CONFIG_ACPI_X86. Ideas welcome.
>
> Linux on Malta does use the ACPI features from the PIIX4.
>
> Please dig in the archives, Igor / myself already argued enough about
> this topic 2 years ago. The consensus was "yes, it is badly implemented,
> but it works and we don't have time to get it cleaner, pc machine is
> way more used than the malta one, so let not break the pc machines."
>
> See:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg613194.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg690435.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg725504.html

Ok I see what you were trying to do. I wanted to stub out the cpu and
memory hotplug bits for non x86 from piix4_acpi_system_hot_add_init().
Howwver, sadly, arm uses memory hotplug for its GED device:

cff51ac978c4fa0b3d0de0fd ("hw/arm/virt: Enable device memory cold/hot plug
with ACPI boot")

This makes things messy and complicated.
Anyways, I am trying something and will send out a patch if I am
successful.