| Message ID | 20210721141610.139310-1-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/acpi: some cosmetic improvements to existing code | expand |
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 > >
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 */
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 */ > >
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 */ > > > > >
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.
> > > > 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.
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 */
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(+)