1. Patchew
  2. QEMU
  3. View series

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

Ani Sinha posted 1 patch 2 years, 9 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210721141610.139310-1-ani@anisinha.ca
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
hw/acpi/ich9.c  | 1 +
hw/acpi/pcihp.c | 5 +++++
2 files changed, 6 insertions(+)
[PATCH] hw/acpi: some cosmetic improvements to existing code
Posted by Ani Sinha 2 years, 9 months ago 
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
Posted by Ani Sinha 2 years, 9 months ago 
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
Posted by Igor Mammedov 2 years, 9 months ago 
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
Posted by Ani Sinha 2 years, 9 months ago 

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
Posted by Igor Mammedov 2 years, 9 months ago 
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
Posted by Ani Sinha 2 years, 9 months ago 

> > > > 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
Posted by Ani Sinha 2 years, 9 months ago 

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.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.