[PATCH] hw/acpi: some cosmetic improvements to existing code

[PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

All existing code using acpi_get_i386_pci_host() checks for a non-null
return from this function call. This change brings the same check to
acpi_pcihp_disable_root_bus() function. Also adds a comment describing
why we unconditionally pass a truth value to the last argument when calling
acpi_pcihp_reset() from ich9 platform.

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

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/acpi/ich9.c  | 1 +
 hw/acpi/pcihp.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 778e27b659..58d8430eb9 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
     pm->smi_en_wmask = ~0;
 
     if (pm->use_acpi_hotplug_bridge) {
+        /* on root PCIE bus, we always use native or SHPC based hotplug */
         acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
     }
 
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index f4d706e47d..856c6e1b47 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
         return;
     }
 
+    if (!host) {
+        root_hp_disabled = true;
+        return;
+    }
+
     bus = PCI_HOST_BRIDGE(host)->bus;
     if (bus) {
         /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
-- 
2.25.1

Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

ping ...

On Wed, 21 Jul 2021, Ani Sinha wrote:

> All existing code using acpi_get_i386_pci_host() checks for a non-null
> return from this function call. This change brings the same check to
> acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> why we unconditionally pass a truth value to the last argument when calling
> acpi_pcihp_reset() from ich9 platform.
>
> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/acpi/ich9.c  | 1 +
>  hw/acpi/pcihp.c | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 778e27b659..58d8430eb9 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
>      pm->smi_en_wmask = ~0;
>
>      if (pm->use_acpi_hotplug_bridge) {
> +        /* on root PCIE bus, we always use native or SHPC based hotplug */
>          acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
>      }
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f4d706e47d..856c6e1b47 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
>          return;
>      }
>
> +    if (!host) {
> +        root_hp_disabled = true;
> +        return;
> +    }
> +
>      bus = PCI_HOST_BRIDGE(host)->bus;
>      if (bus) {
>          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> --
> 2.25.1
>
>

Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Igor Mammedov]

On Wed, 21 Jul 2021 19:46:10 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> All existing code using acpi_get_i386_pci_host() checks for a non-null
> return from this function call. This change brings the same check to
> acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> why we unconditionally pass a truth value to the last argument when calling
> acpi_pcihp_reset() from ich9 platform.
> 
> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/acpi/ich9.c  | 1 +
>  hw/acpi/pcihp.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 778e27b659..58d8430eb9 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
>      pm->smi_en_wmask = ~0;
>  
>      if (pm->use_acpi_hotplug_bridge) {
> +        /* on root PCIE bus, we always use native or SHPC based hotplug */
I had an impression that root bus doesn't support hotplug at all,
and to have hotplug there one should attach a root-port to root bus at
start up time.

>          acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
>      }
>  
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f4d706e47d..856c6e1b47 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
>          return;
>      }
>  
> +    if (!host) {
> +        root_hp_disabled = true;
> +        return;
> +    }
It should be a separate patch,
when this could return NULL?
If it should never be null then assert here would be better.
 
> +
>      bus = PCI_HOST_BRIDGE(host)->bus;
>      if (bus) {
>          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */

Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

On Mon, 26 Jul 2021, Igor Mammedov wrote:

> On Wed, 21 Jul 2021 19:46:10 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > All existing code using acpi_get_i386_pci_host() checks for a non-null
> > return from this function call. This change brings the same check to
> > acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> > why we unconditionally pass a truth value to the last argument when calling
> > acpi_pcihp_reset() from ich9 platform.
> >
> > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/acpi/ich9.c  | 1 +
> >  hw/acpi/pcihp.c | 5 +++++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 778e27b659..58d8430eb9 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
> >      pm->smi_en_wmask = ~0;
> >
> >      if (pm->use_acpi_hotplug_bridge) {
> > +        /* on root PCIE bus, we always use native or SHPC based hotplug */
> I had an impression that root bus doesn't support hotplug at all,
> and to have hotplug there one should attach a root-port to root bus at
> start up time.

I think you are right. However, I will let others confirm this and update
the comment accordingly in a separate patch.


>
> >          acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
> >      }
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index f4d706e47d..856c6e1b47 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
> >          return;
> >      }
> >
> > +    if (!host) {
> > +        root_hp_disabled = true;
> > +        return;
> > +    }
> It should be a separate patch,
> when this could return NULL?
> If it should never be null then assert here would be better.

I have sent a v2 without the comment addition. I left the code this way
because everywhere else, the code checking host for NULL value is similar.
I wanted to keep the symmetry. However, if you strongly feel about the
assertion, I will send a v3.


>
> > +
> >      bus = PCI_HOST_BRIDGE(host)->bus;
> >      if (bus) {
> >          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
>
>

Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Igor Mammedov]

On Mon, 26 Jul 2021 18:26:28 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Mon, 26 Jul 2021, Igor Mammedov wrote:
> 
> > On Wed, 21 Jul 2021 19:46:10 +0530
> > Ani Sinha <ani@anisinha.ca> wrote:
> >  
> > > All existing code using acpi_get_i386_pci_host() checks for a non-null
> > > return from this function call. This change brings the same check to
> > > acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> > > why we unconditionally pass a truth value to the last argument when calling
> > > acpi_pcihp_reset() from ich9 platform.
> > >
> > > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > ---
> > >  hw/acpi/ich9.c  | 1 +
> > >  hw/acpi/pcihp.c | 5 +++++
> > >  2 files changed, 6 insertions(+)
> > >
> > > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > > index 778e27b659..58d8430eb9 100644
> > > --- a/hw/acpi/ich9.c
> > > +++ b/hw/acpi/ich9.c
> > > @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
> > >      pm->smi_en_wmask = ~0;
> > >
> > >      if (pm->use_acpi_hotplug_bridge) {
> > > +        /* on root PCIE bus, we always use native or SHPC based hotplug */  
> > I had an impression that root bus doesn't support hotplug at all,
> > and to have hotplug there one should attach a root-port to root bus at
> > start up time.  
> 
> I think you are right. However, I will let others confirm this and update
> the comment accordingly in a separate patch.
> 
> 
> >  
> > >          acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
> > >      }
> > >
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index f4d706e47d..856c6e1b47 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
> > >          return;
> > >      }
> > >
> > > +    if (!host) {
> > > +        root_hp_disabled = true;
> > > +        return;
> > > +    }  
> > It should be a separate patch,
> > when this could return NULL?
> > If it should never be null then assert here would be better.  
> 
> I have sent a v2 without the comment addition. I left the code this way
> because everywhere else, the code checking host for NULL value is similar.
> I wanted to keep the symmetry. However, if you strongly feel about the
> assertion, I will send a v3.

So the first thing is to confirm if NULL return value is valid or not.
When it clear we can decide whether copy existing check like you do or
replace all such checks with asserts.

> 
> 
> >  
> > > +
> > >      bus = PCI_HOST_BRIDGE(host)->bus;
> > >      if (bus) {
> > >          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */  
> >
> >  
> 

Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index f4d706e47d..856c6e1b47 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
> > > >          return;
> > > >      }
> > > >
> > > > +    if (!host) {
> > > > +        root_hp_disabled = true;
> > > > +        return;
> > > > +    }
> > > It should be a separate patch,
> > > when this could return NULL?
> > > If it should never be null then assert here would be better.
> >
> > I have sent a v2 without the comment addition. I left the code this way
> > because everywhere else, the code checking host for NULL value is similar.
> > I wanted to keep the symmetry. However, if you strongly feel about the
> > assertion, I will send a v3.
>
> So the first thing is to confirm if NULL return value is valid or not.
> When it clear we can decide whether copy existing check like you do or
> replace all such checks with asserts.

I thought about this a little and I am now inclined to put assertions
everywhere. On i386 we have either q35 or i440fx host bridges. Having a
null host bridge does not sound right. I will make the change, build and
make check before sending out v3 with assertions.

Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

On Mon, 26 Jul 2021, Igor Mammedov wrote:

> On Wed, 21 Jul 2021 19:46:10 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > All existing code using acpi_get_i386_pci_host() checks for a non-null
> > return from this function call. This change brings the same check to
> > acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> > why we unconditionally pass a truth value to the last argument when calling
> > acpi_pcihp_reset() from ich9 platform.
> >
> > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/acpi/ich9.c  | 1 +
> >  hw/acpi/pcihp.c | 5 +++++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 778e27b659..58d8430eb9 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
> >      pm->smi_en_wmask = ~0;
> >
> >      if (pm->use_acpi_hotplug_bridge) {
> > +        /* on root PCIE bus, we always use native or SHPC based hotplug */
> I had an impression that root bus doesn't support hotplug at all,
> and to have hotplug there one should attach a root-port to root bus at
> start up time.

Yes this is correct.
https://github.com/qemu/qemu/blob/master/docs/pcie.txt
https://libvirt.org/pci-hotplug.html

I will update the comment accordingly.

[PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

From: Ani Sinha <anirban.sinha@nokia.com>

All existing code using acpi_get_i386_pci_host() checks for a non-null
return from this function call. This change brings the same check to
acpi_pcihp_disable_root_bus() function. Also adds a comment describing
why we unconditionally pass a truth value to the last argument when calling
acpi_pcihp_reset() from ich9 platform.

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

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/acpi/ich9.c  | 1 +
 hw/acpi/pcihp.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 778e27b659..58d8430eb9 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
     pm->smi_en_wmask = ~0;
 
     if (pm->use_acpi_hotplug_bridge) {
+        /* on root PCIE bus, we always use native or SHPC based hotplug */
         acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
     }
 
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index f4d706e47d..856c6e1b47 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
         return;
     }
 
+    if (!host) {
+        root_hp_disabled = true;
+        return;
+    }
+
     bus = PCI_HOST_BRIDGE(host)->bus;
     if (bus) {
         /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
-- 
2.25.1

Re: [PATCH] hw/acpi: some cosmetic improvements to existing code

[Ani Sinha]

Please scratch this. I will resend with my nokia email removed. This work
does not involve Nokia.

Ani


On Wed, 21 Jul 2021, Ani Sinha wrote:

> From: Ani Sinha <anirban.sinha@nokia.com>
>
> All existing code using acpi_get_i386_pci_host() checks for a non-null
> return from this function call. This change brings the same check to
> acpi_pcihp_disable_root_bus() function. Also adds a comment describing
> why we unconditionally pass a truth value to the last argument when calling
> acpi_pcihp_reset() from ich9 platform.
>
> Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/acpi/ich9.c  | 1 +
>  hw/acpi/pcihp.c | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 778e27b659..58d8430eb9 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -281,6 +281,7 @@ static void pm_reset(void *opaque)
>      pm->smi_en_wmask = ~0;
>
>      if (pm->use_acpi_hotplug_bridge) {
> +        /* on root PCIE bus, we always use native or SHPC based hotplug */
>          acpi_pcihp_reset(&pm->acpi_pci_hotplug, true);
>      }
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index f4d706e47d..856c6e1b47 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -136,6 +136,11 @@ static void acpi_pcihp_disable_root_bus(void)
>          return;
>      }
>
> +    if (!host) {
> +        root_hp_disabled = true;
> +        return;
> +    }
> +
>      bus = PCI_HOST_BRIDGE(host)->bus;
>      if (bus) {
>          /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> --
> 2.25.1
>
>