[PATCH] Introduce global piix flag to disable PCI hotplug

[PATCH] Introduce global piix flag to disable PCI hotplug

[Ani Sinha]

We introduce a new global flag for PIIX with which we can
turn on or off PCI device hotplug. This flag can be used
to prevent all PCI devices from getting hotplugged/unplugged
on the PCI bus. The new options disables all hotpluh HW
initialization code as well as the ACPI AMLs.

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

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 26bac4f..8b13e86 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
 
     AcpiPciHpState acpi_pci_hotplug;
     bool use_acpi_hotplug_bridge;
+    bool use_acpi_pci_hotplug;
 
     uint8_t disable_s3;
     uint8_t disable_s4;
@@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                           "acpi-gpe0", GPE_LEN);
     memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
 
-    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
-                    s->use_acpi_hotplug_bridge);
+    if (s->use_acpi_pci_hotplug)
+        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
+                        s->use_acpi_hotplug_bridge);
 
     s->cpu_hotplug_legacy = true;
     object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
@@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
     DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
     DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
                      use_acpi_hotplug_bridge, true),
+    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
+                     use_acpi_pci_hotplug, true),
     DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
                      acpi_memory_hotplug.is_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b7bcbbb..343b9b6 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
     bool s3_disabled;
     bool s4_disabled;
     bool pcihp_bridge_en;
+    bool pcihp_en;
     uint8_t s4_val;
     AcpiFadtData fadt;
     uint16_t cpu_hp_io_base;
@@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
     pm->pcihp_bridge_en =
         object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
                                  NULL);
+    pm->pcihp_en =
+        object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
+
 }
 
 static void acpi_get_misc_info(AcpiMiscInfo *info)
@@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot)
 }
 
 static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
-                                         bool pcihp_bridge_en)
+                                         bool pcihp_bridge_en, bool pcihp_en)
 {
     Aml *dev, *notify_method = NULL, *method;
-    QObject *bsel;
+    QObject *bsel = NULL;
     PCIBus *sec;
     int i;
 
-    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
+    if (pcihp_en)
+        bsel = object_property_get_qobject(OBJECT(bus),
+                                           ACPI_PCIHP_PROP_BSEL, NULL);
     if (bsel) {
         uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
 
@@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
              */
             PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
 
-            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
+            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en,
+                                         pcihp_en);
         }
         /* slot descriptor has been composed, add it into parent context */
         aml_append(parent_scope, dev);
@@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
     }
 
     /* Notify about child bus events in any case */
-    if (pcihp_bridge_en) {
+    if (pcihp_bridge_en && pcihp_en) {
         QLIST_FOREACH(sec, &bus->child, sibling) {
             int32_t devfn = sec->parent_dev->devfn;
 
@@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         if (bus) {
             Aml *scope = aml_scope("PCI0");
             /* Scan all PCI buses. Generate tables to support hotplug. */
-            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
+            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en,
+                                         pm->pcihp_en);
 
             if (TPM_IS_TIS_ISA(tpm)) {
                 if (misc->tpm_version == TPM_VERSION_2_0) {
-- 
2.7.4

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Ani Sinha]

This is related to the discussion we had a few weeks ago:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg705212.html

Igor, could you please comment if this will do. I have not tested it
since I do not have a setup handy. Sending this out for a quick
review.

thanks
ani

On Mon, Aug 10, 2020 at 4:59 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> We introduce a new global flag for PIIX with which we can
> turn on or off PCI device hotplug. This flag can be used
> to prevent all PCI devices from getting hotplugged/unplugged
> on the PCI bus. The new options disables all hotpluh HW
> initialization code as well as the ACPI AMLs.
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/acpi/piix4.c      |  8 ++++++--
>  hw/i386/acpi-build.c | 20 ++++++++++++++------
>  2 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 26bac4f..8b13e86 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
>
>      AcpiPciHpState acpi_pci_hotplug;
>      bool use_acpi_hotplug_bridge;
> +    bool use_acpi_pci_hotplug;
>
>      uint8_t disable_s3;
>      uint8_t disable_s4;
> @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                            "acpi-gpe0", GPE_LEN);
>      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>
> -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> -                    s->use_acpi_hotplug_bridge);
> +    if (s->use_acpi_pci_hotplug)
> +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> +                        s->use_acpi_hotplug_bridge);
>
>      s->cpu_hotplug_legacy = true;
>      object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
>      DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
>      DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
>                       use_acpi_hotplug_bridge, true),
> +    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
> +                     use_acpi_pci_hotplug, true),
>      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>                       acpi_memory_hotplug.is_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b7bcbbb..343b9b6 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
>      bool s3_disabled;
>      bool s4_disabled;
>      bool pcihp_bridge_en;
> +    bool pcihp_en;
>      uint8_t s4_val;
>      AcpiFadtData fadt;
>      uint16_t cpu_hp_io_base;
> @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>      pm->pcihp_bridge_en =
>          object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
>                                   NULL);
> +    pm->pcihp_en =
> +        object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
> +
>  }
>
>  static void acpi_get_misc_info(AcpiMiscInfo *info)
> @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot)
>  }
>
>  static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> -                                         bool pcihp_bridge_en)
> +                                         bool pcihp_bridge_en, bool pcihp_en)
>  {
>      Aml *dev, *notify_method = NULL, *method;
> -    QObject *bsel;
> +    QObject *bsel = NULL;
>      PCIBus *sec;
>      int i;
>
> -    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> +    if (pcihp_en)
> +        bsel = object_property_get_qobject(OBJECT(bus),
> +                                           ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel) {
>          uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
>
> @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>               */
>              PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>
> -            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
> +            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en,
> +                                         pcihp_en);
>          }
>          /* slot descriptor has been composed, add it into parent context */
>          aml_append(parent_scope, dev);
> @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>      }
>
>      /* Notify about child bus events in any case */
> -    if (pcihp_bridge_en) {
> +    if (pcihp_bridge_en && pcihp_en) {
>          QLIST_FOREACH(sec, &bus->child, sibling) {
>              int32_t devfn = sec->parent_dev->devfn;
>
> @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          if (bus) {
>              Aml *scope = aml_scope("PCI0");
>              /* Scan all PCI buses. Generate tables to support hotplug. */
> -            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
> +            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en,
> +                                         pm->pcihp_en);
>
>              if (TPM_IS_TIS_ISA(tpm)) {
>                  if (misc->tpm_version == TPM_VERSION_2_0) {
> --
> 2.7.4
>

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Michael S. Tsirkin]

On Mon, Aug 10, 2020 at 04:59:41PM +0530, Ani Sinha wrote:
> We introduce a new global flag for PIIX with which we can
> turn on or off PCI device hotplug. This flag can be used
> to prevent all PCI devices from getting hotplugged/unplugged
> on the PCI bus. The new options disables all hotpluh HW
> initialization code as well as the ACPI AMLs.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

Well we have a flag like this for pci bridges, right?
So all that's left is an option to disable hotplug
for the pci root, right?
Wouldn't that be better than disabling it globally?

> ---
>  hw/acpi/piix4.c      |  8 ++++++--
>  hw/i386/acpi-build.c | 20 ++++++++++++++------
>  2 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 26bac4f..8b13e86 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
>  
>      AcpiPciHpState acpi_pci_hotplug;
>      bool use_acpi_hotplug_bridge;
> +    bool use_acpi_pci_hotplug;
>  
>      uint8_t disable_s3;
>      uint8_t disable_s4;
> @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>                            "acpi-gpe0", GPE_LEN);
>      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>  
> -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> -                    s->use_acpi_hotplug_bridge);
> +    if (s->use_acpi_pci_hotplug)
> +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> +                        s->use_acpi_hotplug_bridge);
>  
>      s->cpu_hotplug_legacy = true;
>      object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
>      DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
>      DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
>                       use_acpi_hotplug_bridge, true),
> +    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
> +                     use_acpi_pci_hotplug, true),
>      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>                       acpi_memory_hotplug.is_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b7bcbbb..343b9b6 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
>      bool s3_disabled;
>      bool s4_disabled;
>      bool pcihp_bridge_en;
> +    bool pcihp_en;
>      uint8_t s4_val;
>      AcpiFadtData fadt;
>      uint16_t cpu_hp_io_base;
> @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>      pm->pcihp_bridge_en =
>          object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
>                                   NULL);
> +    pm->pcihp_en =
> +        object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
> +
>  }
>  
>  static void acpi_get_misc_info(AcpiMiscInfo *info)
> @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot)
>  }
>  
>  static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> -                                         bool pcihp_bridge_en)
> +                                         bool pcihp_bridge_en, bool pcihp_en)
>  {
>      Aml *dev, *notify_method = NULL, *method;
> -    QObject *bsel;
> +    QObject *bsel = NULL;
>      PCIBus *sec;
>      int i;
>  
> -    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> +    if (pcihp_en)
> +        bsel = object_property_get_qobject(OBJECT(bus),
> +                                           ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel) {
>          uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
>  
> @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>               */
>              PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>  
> -            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
> +            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en,
> +                                         pcihp_en);
>          }
>          /* slot descriptor has been composed, add it into parent context */
>          aml_append(parent_scope, dev);
> @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>      }
>  
>      /* Notify about child bus events in any case */
> -    if (pcihp_bridge_en) {
> +    if (pcihp_bridge_en && pcihp_en) {
>          QLIST_FOREACH(sec, &bus->child, sibling) {
>              int32_t devfn = sec->parent_dev->devfn;
>  
> @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          if (bus) {
>              Aml *scope = aml_scope("PCI0");
>              /* Scan all PCI buses. Generate tables to support hotplug. */
> -            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
> +            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en,
> +                                         pm->pcihp_en);
>  
>              if (TPM_IS_TIS_ISA(tpm)) {
>                  if (misc->tpm_version == TPM_VERSION_2_0) {
> -- 
> 2.7.4

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Ani Sinha]

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

On Mon, Aug 10, 2020 at 8:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Aug 10, 2020 at 04:59:41PM +0530, Ani Sinha wrote:
> > We introduce a new global flag for PIIX with which we can
> > turn on or off PCI device hotplug. This flag can be used
> > to prevent all PCI devices from getting hotplugged/unplugged
> > on the PCI bus. The new options disables all hotpluh HW
> > initialization code as well as the ACPI AMLs.
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
>
> Well we have a flag like this for pci bridges, right?
> So all that's left is an option to disable hotplug
> for the pci root, right?
> Wouldn't that be better than disabling it globally?


The idea is to have just one option to disable all hotplug globally. But if
you want to have two flags one for the bridges and one for the pci root, we
can certainly look into it.


>
> > ---
> >  hw/acpi/piix4.c      |  8 ++++++--
> >  hw/i386/acpi-build.c | 20 ++++++++++++++------
> >  2 files changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 26bac4f..8b13e86 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
> >
> >      AcpiPciHpState acpi_pci_hotplug;
> >      bool use_acpi_hotplug_bridge;
> > +    bool use_acpi_pci_hotplug;
> >
> >      uint8_t disable_s3;
> >      uint8_t disable_s4;
> > @@ -595,8 +596,9 @@ static void
> piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> >                            "acpi-gpe0", GPE_LEN);
> >      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> >
> > -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > -                    s->use_acpi_hotplug_bridge);
> > +    if (s->use_acpi_pci_hotplug)
> > +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > +                        s->use_acpi_hotplug_bridge);
> >
> >      s->cpu_hotplug_legacy = true;
> >      object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
> >      DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
> >      DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support",
> PIIX4PMState,
> >                       use_acpi_hotplug_bridge, true),
> > +    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
> > +                     use_acpi_pci_hotplug, true),
> >      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> >                       acpi_memory_hotplug.is_enabled, true),
> >      DEFINE_PROP_END_OF_LIST(),
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b7bcbbb..343b9b6 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> >      bool s3_disabled;
> >      bool s4_disabled;
> >      bool pcihp_bridge_en;
> > +    bool pcihp_en;
> >      uint8_t s4_val;
> >      AcpiFadtData fadt;
> >      uint16_t cpu_hp_io_base;
> > @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine,
> AcpiPmInfo *pm)
> >      pm->pcihp_bridge_en =
> >          object_property_get_bool(obj,
> "acpi-pci-hotplug-with-bridge-support",
> >                                   NULL);
> > +    pm->pcihp_en =
> > +        object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
> > +
> >  }
> >
> >  static void acpi_get_misc_info(AcpiMiscInfo *info)
> > @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry(Aml
> *method, int slot)
> >  }
> >
> >  static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > -                                         bool pcihp_bridge_en)
> > +                                         bool pcihp_bridge_en, bool
> pcihp_en)
> >  {
> >      Aml *dev, *notify_method = NULL, *method;
> > -    QObject *bsel;
> > +    QObject *bsel = NULL;
> >      PCIBus *sec;
> >      int i;
> >
> > -    bsel = object_property_get_qobject(OBJECT(bus),
> ACPI_PCIHP_PROP_BSEL, NULL);
> > +    if (pcihp_en)
> > +        bsel = object_property_get_qobject(OBJECT(bus),
> > +                                           ACPI_PCIHP_PROP_BSEL, NULL);
> >      if (bsel) {
> >          uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
> >
> > @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> >               */
> >              PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> >
> > -            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
> > +            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en,
> > +                                         pcihp_en);
> >          }
> >          /* slot descriptor has been composed, add it into parent
> context */
> >          aml_append(parent_scope, dev);
> > @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> >      }
> >
> >      /* Notify about child bus events in any case */
> > -    if (pcihp_bridge_en) {
> > +    if (pcihp_bridge_en && pcihp_en) {
> >          QLIST_FOREACH(sec, &bus->child, sibling) {
> >              int32_t devfn = sec->parent_dev->devfn;
> >
> > @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          if (bus) {
> >              Aml *scope = aml_scope("PCI0");
> >              /* Scan all PCI buses. Generate tables to support hotplug.
> */
> > -            build_append_pci_bus_devices(scope, bus,
> pm->pcihp_bridge_en);
> > +            build_append_pci_bus_devices(scope, bus,
> pm->pcihp_bridge_en,
> > +                                         pm->pcihp_en);
> >
> >              if (TPM_IS_TIS_ISA(tpm)) {
> >                  if (misc->tpm_version == TPM_VERSION_2_0) {
> > --
> > 2.7.4
>
>

[-- Attachment #2: Type: text/html, Size: 7710 bytes --]

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Michael S. Tsirkin]

On Mon, Aug 10, 2020 at 08:24:53PM +0530, Ani Sinha wrote:
> 
> 
> On Mon, Aug 10, 2020 at 8:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Mon, Aug 10, 2020 at 04:59:41PM +0530, Ani Sinha wrote:
>     > We introduce a new global flag for PIIX with which we can
>     > turn on or off PCI device hotplug. This flag can be used
>     > to prevent all PCI devices from getting hotplugged/unplugged
>     > on the PCI bus. The new options disables all hotpluh HW
>     > initialization code as well as the ACPI AMLs.
>     >
>     > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> 
>     Well we have a flag like this for pci bridges, right?
>     So all that's left is an option to disable hotplug
>     for the pci root, right?
>     Wouldn't that be better than disabling it globally?
> 
> 
> The idea is to have just one option to disable all hotplug globally. But if you
> want to have two flags one for the bridges and one for the pci root, we can
> certainly look into it.
> 

I can certainly see the attraction of a global flag.

However, with the interface we have for bridges right now, how are we
going to resolve the conflict if hotplug is disabled globally but
enabled for a given bridge?

A reasonable way would be to change the default value for bridges,
have a user-specified value override it.
This patch doesn't do it, though.




> 
> 
>     > ---
>     >  hw/acpi/piix4.c      |  8 ++++++--
>     >  hw/i386/acpi-build.c | 20 ++++++++++++++------
>     >  2 files changed, 20 insertions(+), 8 deletions(-)
>     >
>     > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>     > index 26bac4f..8b13e86 100644
>     > --- a/hw/acpi/piix4.c
>     > +++ b/hw/acpi/piix4.c
>     > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
>     > 
>     >      AcpiPciHpState acpi_pci_hotplug;
>     >      bool use_acpi_hotplug_bridge;
>     > +    bool use_acpi_pci_hotplug;
>     > 
>     >      uint8_t disable_s3;
>     >      uint8_t disable_s4;
>     > @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init
>     (MemoryRegion *parent,
>     >                            "acpi-gpe0", GPE_LEN);
>     >      memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>     > 
>     > -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
>     > -                    s->use_acpi_hotplug_bridge);
>     > +    if (s->use_acpi_pci_hotplug)
>     > +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
>     > +                        s->use_acpi_hotplug_bridge);
>     > 
>     >      s->cpu_hotplug_legacy = true;
>     >      object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
>     > @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
>     >      DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
>     >      DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support",
>     PIIX4PMState,
>     >                       use_acpi_hotplug_bridge, true),
>     > +    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
>     > +                     use_acpi_pci_hotplug, true),
>     >      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>     >                       acpi_memory_hotplug.is_enabled, true),
>     >      DEFINE_PROP_END_OF_LIST(),
>     > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>     > index b7bcbbb..343b9b6 100644
>     > --- a/hw/i386/acpi-build.c
>     > +++ b/hw/i386/acpi-build.c
>     > @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
>     >      bool s3_disabled;
>     >      bool s4_disabled;
>     >      bool pcihp_bridge_en;
>     > +    bool pcihp_en;
>     >      uint8_t s4_val;
>     >      AcpiFadtData fadt;
>     >      uint16_t cpu_hp_io_base;
>     > @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine,
>     AcpiPmInfo *pm)
>     >      pm->pcihp_bridge_en =
>     >          object_property_get_bool(obj,
>     "acpi-pci-hotplug-with-bridge-support",
>     >                                   NULL);
>     > +    pm->pcihp_en =
>     > +        object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
>     > +
>     >  }
>     > 
>     >  static void acpi_get_misc_info(AcpiMiscInfo *info)
>     > @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry(Aml
>     *method, int slot)
>     >  }
>     > 
>     >  static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>     > -                                         bool pcihp_bridge_en)
>     > +                                         bool pcihp_bridge_en, bool
>     pcihp_en)
>     >  {
>     >      Aml *dev, *notify_method = NULL, *method;
>     > -    QObject *bsel;
>     > +    QObject *bsel = NULL;
>     >      PCIBus *sec;
>     >      int i;
>     > 
>     > -    bsel = object_property_get_qobject(OBJECT(bus),
>     ACPI_PCIHP_PROP_BSEL, NULL);
>     > +    if (pcihp_en)
>     > +        bsel = object_property_get_qobject(OBJECT(bus),
>     > +                                           ACPI_PCIHP_PROP_BSEL, NULL);
>     >      if (bsel) {
>     >          uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
>     > 
>     > @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml
>     *parent_scope, PCIBus *bus,
>     >               */
>     >              PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>     > 
>     > -            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
>     > +            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en,
>     > +                                         pcihp_en);
>     >          }
>     >          /* slot descriptor has been composed, add it into parent context
>     */
>     >          aml_append(parent_scope, dev);
>     > @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml
>     *parent_scope, PCIBus *bus,
>     >      }
>     > 
>     >      /* Notify about child bus events in any case */
>     > -    if (pcihp_bridge_en) {
>     > +    if (pcihp_bridge_en && pcihp_en) {
>     >          QLIST_FOREACH(sec, &bus->child, sibling) {
>     >              int32_t devfn = sec->parent_dev->devfn;
>     > 
>     > @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>     >          if (bus) {
>     >              Aml *scope = aml_scope("PCI0");
>     >              /* Scan all PCI buses. Generate tables to support hotplug. *
>     /
>     > -            build_append_pci_bus_devices(scope, bus, pm->
>     pcihp_bridge_en);
>     > +            build_append_pci_bus_devices(scope, bus, pm->
>     pcihp_bridge_en,
>     > +                                         pm->pcihp_en);
>     > 
>     >              if (TPM_IS_TIS_ISA(tpm)) {
>     >                  if (misc->tpm_version == TPM_VERSION_2_0) {
>     > --
>     > 2.7.4
> 
> 

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Ani Sinha]

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


Ani
On Aug 10, 2020, 20:30 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
> On Mon, Aug 10, 2020 at 08:24:53PM +0530, Ani Sinha wrote:
> >
> >
> > On Mon, Aug 10, 2020 at 8:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Aug 10, 2020 at 04:59:41PM +0530, Ani Sinha wrote:
> > > We introduce a new global flag for PIIX with which we can
> > > turn on or off PCI device hotplug. This flag can be used
> > > to prevent all PCI devices from getting hotplugged/unplugged
> > > on the PCI bus. The new options disables all hotpluh HW
> > > initialization code as well as the ACPI AMLs.
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >
> > Well we have a flag like this for pci bridges, right?
> > So all that's left is an option to disable hotplug
> > for the pci root, right?
> > Wouldn't that be better than disabling it globally?
> >
> >
> > The idea is to have just one option to disable all hotplug globally. But if you
> > want to have two flags one for the bridges and one for the pci root, we can
> > certainly look into it.
> >
>
> I can certainly see the attraction of a global flag.
>
> However, with the interface we have for bridges right now, how are we
> going to resolve the conflict if hotplug is disabled globally but
> enabled for a given bridge?
>
> A reasonable way would be to change the default value for bridges,
> have a user-specified value override it.
> This patch doesn't do it, though.

I think the global option should override the bridge option. So if the global option disables hotplug, the bridge option should be a noop.
>
>
>
>
> >
> >
> > > ---
> > >   hw/acpi/piix4.c      |  8 ++++++--
> > >   hw/i386/acpi-build.c | 20 ++++++++++++++------
> > >   2 files changed, 20 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index 26bac4f..8b13e86 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
> > >
> > >       AcpiPciHpState acpi_pci_hotplug;
> > >       bool use_acpi_hotplug_bridge;
> > > +    bool use_acpi_pci_hotplug;
> > >
> > >       uint8_t disable_s3;
> > >       uint8_t disable_s4;
> > > @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init
> > (MemoryRegion *parent,
> > >                             "acpi-gpe0", GPE_LEN);
> > >       memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> > >
> > > -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > > -                    s->use_acpi_hotplug_bridge);
> > > +    if (s->use_acpi_pci_hotplug)
> > > +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > > +                        s->use_acpi_hotplug_bridge);
> > >
> > >       s->cpu_hotplug_legacy = true;
> > >       object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > > @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
> > >       DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
> > >       DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support",
> > PIIX4PMState,
> > >                        use_acpi_hotplug_bridge, true),
> > > +    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
> > > +                     use_acpi_pci_hotplug, true),
> > >       DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > >                        acpi_memory_hotplug.is_enabled, true),
> > >       DEFINE_PROP_END_OF_LIST(),
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index b7bcbbb..343b9b6 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> > >       bool s3_disabled;
> > >       bool s4_disabled;
> > >       bool pcihp_bridge_en;
> > > +    bool pcihp_en;
> > >       uint8_t s4_val;
> > >       AcpiFadtData fadt;
> > >       uint16_t cpu_hp_io_base;
> > > @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine,
> > AcpiPmInfo *pm)
> > >       pm->pcihp_bridge_en =
> > >           object_property_get_bool(obj,
> > "acpi-pci-hotplug-with-bridge-support",
> > >                                    NULL);
> > > +    pm->pcihp_en =
> > > +        object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
> > > +
> > >   }
> > >
> > >   static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry(Aml
> > *method, int slot)
> > >   }
> > >
> > >   static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > > -                                         bool pcihp_bridge_en)
> > > +                                         bool pcihp_bridge_en, bool
> > pcihp_en)
> > >   {
> > >       Aml *dev, *notify_method = NULL, *method;
> > > -    QObject *bsel;
> > > +    QObject *bsel = NULL;
> > >       PCIBus *sec;
> > >       int i;
> > >
> > > -    bsel = object_property_get_qobject(OBJECT(bus),
> > ACPI_PCIHP_PROP_BSEL, NULL);
> > > +    if (pcihp_en)
> > > +        bsel = object_property_get_qobject(OBJECT(bus),
> > > +                                           ACPI_PCIHP_PROP_BSEL, NULL);
> > >       if (bsel) {
> > >           uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
> > >
> > > @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml
> > *parent_scope, PCIBus *bus,
> > >                */
> > >               PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> > >
> > > -            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
> > > +            build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en,
> > > +                                         pcihp_en);
> > >           }
> > >           /* slot descriptor has been composed, add it into parent context
> > */
> > >           aml_append(parent_scope, dev);
> > > @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml
> > *parent_scope, PCIBus *bus,
> > >       }
> > >
> > >       /* Notify about child bus events in any case */
> > > -    if (pcihp_bridge_en) {
> > > +    if (pcihp_bridge_en && pcihp_en) {
> > >           QLIST_FOREACH(sec, &bus->child, sibling) {
> > >               int32_t devfn = sec->parent_dev->devfn;
> > >
> > > @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >           if (bus) {
> > >               Aml *scope = aml_scope("PCI0");
> > >               /* Scan all PCI buses. Generate tables to support hotplug. *
> > /
> > > -            build_append_pci_bus_devices(scope, bus, pm->
> > pcihp_bridge_en);
> > > +            build_append_pci_bus_devices(scope, bus, pm->
> > pcihp_bridge_en,
> > > +                                         pm->pcihp_en);
> > >
> > >               if (TPM_IS_TIS_ISA(tpm)) {
> > >                   if (misc->tpm_version == TPM_VERSION_2_0) {
> > > --
> > > 2.7.4
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 84309 bytes --]

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Michael S. Tsirkin]

On Mon, Aug 10, 2020 at 08:35:56PM +0530, Ani Sinha wrote:
> 
> Ani
> On Aug 10, 2020, 20:30 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
> 
>     On Mon, Aug 10, 2020 at 08:24:53PM +0530, Ani Sinha wrote:
> 
> 
> 
> 
> 
>         On Mon, Aug 10, 2020 at 8:17 PM Michael S. Tsirkin <mst@redhat.com>
>         wrote:
> 
> 
> 
>         On Mon, Aug 10, 2020 at 04:59:41PM +0530, Ani Sinha wrote:
> 
>             We introduce a new global flag for PIIX with which we can
> 
>             turn on or off PCI device hotplug. This flag can be used
> 
>             to prevent all PCI devices from getting hotplugged/unplugged
> 
>             on the PCI bus. The new options disables all hotpluh HW
> 
>             initialization code as well as the ACPI AMLs.
> 
> 
> 
>             Signed-off-by: Ani Sinha <ani@anisinha.ca>
> 
> 
> 
>         Well we have a flag like this for pci bridges, right?
> 
>         So all that's left is an option to disable hotplug
> 
>         for the pci root, right?
> 
>         Wouldn't that be better than disabling it globally?
> 
> 
> 
> 
> 
>         The idea is to have just one option to disable all hotplug globally.
>         But if you
> 
>         want to have two flags one for the bridges and one for the pci root, we
>         can
> 
>         certainly look into it.
> 
> 
> 
> 
> 
>     I can certainly see the attraction of a global flag.
> 
> 
> 
>     However, with the interface we have for bridges right now, how are we
> 
>     going to resolve the conflict if hotplug is disabled globally but
> 
>     enabled for a given bridge?
> 
> 
> 
>     A reasonable way would be to change the default value for bridges,
> 
>     have a user-specified value override it.
> 
>     This patch doesn't do it, though.
> 
> 
> I think the global option should override the bridge option. So if the global
> option disables hotplug, the bridge option should be a noop.
> 

Well just ignoring inconsistent user input does not sound great.
Tends to create support problems.

> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
>             ---
> 
>               hw/acpi/piix4.c      |  8 ++++++--
> 
>               hw/i386/acpi-build.c | 20 ++++++++++++++------
> 
>               2 files changed, 20 insertions(+), 8 deletions(-)
> 
> 
> 
>             diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> 
>             index 26bac4f..8b13e86 100644
> 
>             --- a/hw/acpi/piix4.c
> 
>             +++ b/hw/acpi/piix4.c
> 
>             @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
> 
>              
> 
>                   AcpiPciHpState acpi_pci_hotplug;
> 
>                   bool use_acpi_hotplug_bridge;
> 
>             +    bool use_acpi_pci_hotplug;
> 
>              
> 
>                   uint8_t disable_s3;
> 
>                   uint8_t disable_s4;
> 
>             @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init
> 
>         (MemoryRegion *parent,
> 
>                                         "acpi-gpe0", GPE_LEN);
> 
>                   memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> 
>              
> 
>             -    acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> 
>             -                    s->use_acpi_hotplug_bridge);
> 
>             +    if (s->use_acpi_pci_hotplug)
> 
>             +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus,
>             parent,
> 
>             +                        s->use_acpi_hotplug_bridge);
> 
>              
> 
>                   s->cpu_hotplug_legacy = true;
> 
>                   object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> 
>             @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
> 
>                   DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val,
>             2),
> 
>                   DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support",
> 
>         PIIX4PMState,
> 
>                                    use_acpi_hotplug_bridge, true),
> 
>             +    DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
> 
>             +                     use_acpi_pci_hotplug, true),
> 
>                   DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> 
>                                    acpi_memory_hotplug.is_enabled, true),
> 
>                   DEFINE_PROP_END_OF_LIST(),
> 
>             diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> 
>             index b7bcbbb..343b9b6 100644
> 
>             --- a/hw/i386/acpi-build.c
> 
>             +++ b/hw/i386/acpi-build.c
> 
>             @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> 
>                   bool s3_disabled;
> 
>                   bool s4_disabled;
> 
>                   bool pcihp_bridge_en;
> 
>             +    bool pcihp_en;
> 
>                   uint8_t s4_val;
> 
>                   AcpiFadtData fadt;
> 
>                   uint16_t cpu_hp_io_base;
> 
>             @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState
>             *machine,
> 
>         AcpiPmInfo *pm)
> 
>                   pm->pcihp_bridge_en =
> 
>                       object_property_get_bool(obj,
> 
>         "acpi-pci-hotplug-with-bridge-support",
> 
>                                                NULL);
> 
>             +    pm->pcihp_en =
> 
>             +        object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
> 
>             +
> 
>               }
> 
>              
> 
>               static void acpi_get_misc_info(AcpiMiscInfo *info)
> 
>             @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry
>             (Aml
> 
>         *method, int slot)
> 
>               }
> 
>              
> 
>               static void build_append_pci_bus_devices(Aml *parent_scope,
>             PCIBus *bus,
> 
>             -                                         bool pcihp_bridge_en)
> 
>             +                                         bool pcihp_bridge_en,
>             bool
> 
>         pcihp_en)
> 
>               {
> 
>                   Aml *dev, *notify_method = NULL, *method;
> 
>             -    QObject *bsel;
> 
>             +    QObject *bsel = NULL;
> 
>                   PCIBus *sec;
> 
>                   int i;
> 
>              
> 
>             -    bsel = object_property_get_qobject(OBJECT(bus),
> 
>         ACPI_PCIHP_PROP_BSEL, NULL);
> 
>             +    if (pcihp_en)
> 
>             +        bsel = object_property_get_qobject(OBJECT(bus),
> 
>             +                                           ACPI_PCIHP_PROP_BSEL,
>             NULL);
> 
>                   if (bsel) {
> 
>                       uint64_t bsel_val = qnum_get_uint(qobject_to(QNum,
>             bsel));
> 
>              
> 
>             @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml
> 
>         *parent_scope, PCIBus *bus,
> 
>                            */
> 
>                           PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE
>             (pdev));
> 
>              
> 
>             -            build_append_pci_bus_devices(dev, sec_bus,
>             pcihp_bridge_en);
> 
>             +            build_append_pci_bus_devices(dev, sec_bus,
>             pcihp_bridge_en,
> 
>             +                                         pcihp_en);
> 
>                       }
> 
>                       /* slot descriptor has been composed, add it into parent
>             context
> 
>         */
> 
>                       aml_append(parent_scope, dev);
> 
>             @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml
> 
>         *parent_scope, PCIBus *bus,
> 
>                   }
> 
>              
> 
>                   /* Notify about child bus events in any case */
> 
>             -    if (pcihp_bridge_en) {
> 
>             +    if (pcihp_bridge_en && pcihp_en) {
> 
>                       QLIST_FOREACH(sec, &bus->child, sibling) {
> 
>                           int32_t devfn = sec->parent_dev->devfn;
> 
>              
> 
>             @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker
>             *linker,
> 
>                       if (bus) {
> 
>                           Aml *scope = aml_scope("PCI0");
> 
>                           /* Scan all PCI buses. Generate tables to support
>             hotplug. *
> 
>         /
> 
>             -            build_append_pci_bus_devices(scope, bus, pm->
> 
>         pcihp_bridge_en);
> 
>             +            build_append_pci_bus_devices(scope, bus, pm->
> 
>         pcihp_bridge_en,
> 
>             +                                         pm->pcihp_en);
> 
>              
> 
>                           if (TPM_IS_TIS_ISA(tpm)) {
> 
>                               if (misc->tpm_version == TPM_VERSION_2_0) {
> 
>             --
> 
>             2.7.4
> 
> 
> 
> 
> 
> 
> 

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Ani Sinha]

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


Ani
On Aug 10, 2020, 20:49 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
> On Mon, Aug 10, 2020 at 08:35:56PM +0530, Ani Sinha wrote:
> >
> > Ani
> > On Aug 10, 2020, 20:30 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
> >
> > On Mon, Aug 10, 2020 at 08:24:53PM +0530, Ani Sinha wrote:
> >
> >
> >
> >
> >
> > On Mon, Aug 10, 2020 at 8:17 PM Michael S. Tsirkin <mst@redhat.com>
> > wrote:
> >
> >
> >
> > On Mon, Aug 10, 2020 at 04:59:41PM +0530, Ani Sinha wrote:
> >
> > We introduce a new global flag for PIIX with which we can
> >
> > turn on or off PCI device hotplug. This flag can be used
> >
> > to prevent all PCI devices from getting hotplugged/unplugged
> >
> > on the PCI bus. The new options disables all hotpluh HW
> >
> > initialization code as well as the ACPI AMLs.
> >
> >
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >
> >
> >
> > Well we have a flag like this for pci bridges, right?
> >
> > So all that's left is an option to disable hotplug
> >
> > for the pci root, right?
> >
> > Wouldn't that be better than disabling it globally?
> >
> >
> >
> >
> >
> > The idea is to have just one option to disable all hotplug globally.
> > But if you
> >
> > want to have two flags one for the bridges and one for the pci root, we
> > can
> >
> > certainly look into it.
> >
> >
> >
> >
> >
> > I can certainly see the attraction of a global flag.
> >
> >
> >
> > However, with the interface we have for bridges right now, how are we
> >
> > going to resolve the conflict if hotplug is disabled globally but
> >
> > enabled for a given bridge?
> >
> >
> >
> > A reasonable way would be to change the default value for bridges,
> >
> > have a user-specified value override it.
> >
> > This patch doesn't do it, though.
> >
> >
> > I think the global option should override the bridge option. So if the global
> > option disables hotplug, the bridge option should be a noop.
> >
>
> Well just ignoring inconsistent user input does not sound great.
> Tends to create support problems.

Then I suggest we take your original idea - have the new option just disable hotplug on the pci root bus.

This would be cleaner and would introduce less confusion.  Also the patch should be simple.
>
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > ---
> >
> > hw/acpi/piix4.c | 8 ++++++--
> >
> > hw/i386/acpi-build.c | 20 ++++++++++++++------
> >
> > 2 files changed, 20 insertions(+), 8 deletions(-)
> >
> >
> >
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >
> > index 26bac4f..8b13e86 100644
> >
> > --- a/hw/acpi/piix4.c
> >
> > +++ b/hw/acpi/piix4.c
> >
> > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
> >
> >
> >
> > AcpiPciHpState acpi_pci_hotplug;
> >
> > bool use_acpi_hotplug_bridge;
> >
> > + bool use_acpi_pci_hotplug;
> >
> >
> >
> > uint8_t disable_s3;
> >
> > uint8_t disable_s4;
> >
> > @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init
> >
> > (MemoryRegion *parent,
> >
> > "acpi-gpe0", GPE_LEN);
> >
> > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> >
> >
> >
> > - acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> >
> > - s->use_acpi_hotplug_bridge);
> >
> > + if (s->use_acpi_pci_hotplug)
> >
> > + acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus,
> > parent,
> >
> > + s->use_acpi_hotplug_bridge);
> >
> >
> >
> > s->cpu_hotplug_legacy = true;
> >
> > object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> >
> > @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
> >
> > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val,
> > 2),
> >
> > DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support",
> >
> > PIIX4PMState,
> >
> > use_acpi_hotplug_bridge, true),
> >
> > + DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
> >
> > + use_acpi_pci_hotplug, true),
> >
> > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> >
> > acpi_memory_hotplug.is_enabled, true),
> >
> > DEFINE_PROP_END_OF_LIST(),
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >
> > index b7bcbbb..343b9b6 100644
> >
> > --- a/hw/i386/acpi-build.c
> >
> > +++ b/hw/i386/acpi-build.c
> >
> > @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> >
> > bool s3_disabled;
> >
> > bool s4_disabled;
> >
> > bool pcihp_bridge_en;
> >
> > + bool pcihp_en;
> >
> > uint8_t s4_val;
> >
> > AcpiFadtData fadt;
> >
> > uint16_t cpu_hp_io_base;
> >
> > @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState
> > *machine,
> >
> > AcpiPmInfo *pm)
> >
> > pm->pcihp_bridge_en =
> >
> > object_property_get_bool(obj,
> >
> > "acpi-pci-hotplug-with-bridge-support",
> >
> > NULL);
> >
> > + pm->pcihp_en =
> >
> > + object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
> >
> > +
> >
> > }
> >
> >
> >
> > static void acpi_get_misc_info(AcpiMiscInfo *info)
> >
> > @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry
> > (Aml
> >
> > *method, int slot)
> >
> > }
> >
> >
> >
> > static void build_append_pci_bus_devices(Aml *parent_scope,
> > PCIBus *bus,
> >
> > - bool pcihp_bridge_en)
> >
> > + bool pcihp_bridge_en,
> > bool
> >
> > pcihp_en)
> >
> > {
> >
> > Aml *dev, *notify_method = NULL, *method;
> >
> > - QObject *bsel;
> >
> > + QObject *bsel = NULL;
> >
> > PCIBus *sec;
> >
> > int i;
> >
> >
> >
> > - bsel = object_property_get_qobject(OBJECT(bus),
> >
> > ACPI_PCIHP_PROP_BSEL, NULL);
> >
> > + if (pcihp_en)
> >
> > + bsel = object_property_get_qobject(OBJECT(bus),
> >
> > + ACPI_PCIHP_PROP_BSEL,
> > NULL);
> >
> > if (bsel) {
> >
> > uint64_t bsel_val = qnum_get_uint(qobject_to(QNum,
> > bsel));
> >
> >
> >
> > @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml
> >
> > *parent_scope, PCIBus *bus,
> >
> > */
> >
> > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE
> > (pdev));
> >
> >
> >
> > - build_append_pci_bus_devices(dev, sec_bus,
> > pcihp_bridge_en);
> >
> > + build_append_pci_bus_devices(dev, sec_bus,
> > pcihp_bridge_en,
> >
> > + pcihp_en);
> >
> > }
> >
> > /* slot descriptor has been composed, add it into parent
> > context
> >
> > */
> >
> > aml_append(parent_scope, dev);
> >
> > @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml
> >
> > *parent_scope, PCIBus *bus,
> >
> > }
> >
> >
> >
> > /* Notify about child bus events in any case */
> >
> > - if (pcihp_bridge_en) {
> >
> > + if (pcihp_bridge_en && pcihp_en) {
> >
> > QLIST_FOREACH(sec, &bus->child, sibling) {
> >
> > int32_t devfn = sec->parent_dev->devfn;
> >
> >
> >
> > @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker
> > *linker,
> >
> > if (bus) {
> >
> > Aml *scope = aml_scope("PCI0");
> >
> > /* Scan all PCI buses. Generate tables to support
> > hotplug. *
> >
> > /
> >
> > - build_append_pci_bus_devices(scope, bus, pm->
> >
> > pcihp_bridge_en);
> >
> > + build_append_pci_bus_devices(scope, bus, pm->
> >
> > pcihp_bridge_en,
> >
> > + pm->pcihp_en);
> >
> >
> >
> > if (TPM_IS_TIS_ISA(tpm)) {
> >
> > if (misc->tpm_version == TPM_VERSION_2_0) {
> >
> > --
> >
> > 2.7.4
> >
> >
> >
> >
> >
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 135265 bytes --]

Re: [PATCH] Introduce global piix flag to disable PCI hotplug

[Ani Sinha]

On Mon, Aug 10, 2020 at 9:11 PM Ani Sinha <ani@anisinha.ca> wrote:
>
>
> Ani
> On Aug 10, 2020, 20:49 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
>
> On Mon, Aug 10, 2020 at 08:35:56PM +0530, Ani Sinha wrote:
>
>
> Ani
>
> On Aug 10, 2020, 20:30 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
>
>
> On Mon, Aug 10, 2020 at 08:24:53PM +0530, Ani Sinha wrote:
>
>
>
>
>
>
> On Mon, Aug 10, 2020 at 8:17 PM Michael S. Tsirkin <mst@redhat.com>
>
> wrote:
>
>
>
>
> On Mon, Aug 10, 2020 at 04:59:41PM +0530, Ani Sinha wrote:
>
>
> We introduce a new global flag for PIIX with which we can
>
>
> turn on or off PCI device hotplug. This flag can be used
>
>
> to prevent all PCI devices from getting hotplugged/unplugged
>
>
> on the PCI bus. The new options disables all hotpluh HW
>
>
> initialization code as well as the ACPI AMLs.
>
>
>
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>
>
>
>
> Well we have a flag like this for pci bridges, right?
>
>
> So all that's left is an option to disable hotplug
>
>
> for the pci root, right?
>
>
> Wouldn't that be better than disabling it globally?
>
>
>
>
>
>
> The idea is to have just one option to disable all hotplug globally.
>
> But if you
>
>
> want to have two flags one for the bridges and one for the pci root, we
>
> can
>
>
> certainly look into it.
>
>
>
>
>
>
> I can certainly see the attraction of a global flag.
>
>
>
>
> However, with the interface we have for bridges right now, how are we
>
>
> going to resolve the conflict if hotplug is disabled globally but
>
>
> enabled for a given bridge?
>
>
>
>
> A reasonable way would be to change the default value for bridges,
>
>
> have a user-specified value override it.
>
>
> This patch doesn't do it, though.
>
>
>
> I think the global option should override the bridge option. So if the global
>
> option disables hotplug, the bridge option should be a noop.
>
>
>
> Well just ignoring inconsistent user input does not sound great.
>
> Tends to create support problems.
>
>
> Then I suggest we take your original idea - have the new option just disable hotplug on the pci root bus.
>
> This would be cleaner and would introduce less confusion.  Also the patch should be simple.

I have sent a patch to this effect.

>
>
>
>
>
>
>
>
>
>
>
>
>
> ---
>
>
> hw/acpi/piix4.c | 8 ++++++--
>
>
> hw/i386/acpi-build.c | 20 ++++++++++++++------
>
>
> 2 files changed, 20 insertions(+), 8 deletions(-)
>
>
>
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>
>
> index 26bac4f..8b13e86 100644
>
>
> --- a/hw/acpi/piix4.c
>
>
> +++ b/hw/acpi/piix4.c
>
>
> @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
>
>
>
>
> AcpiPciHpState acpi_pci_hotplug;
>
>
> bool use_acpi_hotplug_bridge;
>
>
> + bool use_acpi_pci_hotplug;
>
>
>
>
> uint8_t disable_s3;
>
>
> uint8_t disable_s4;
>
>
> @@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init
>
>
> (MemoryRegion *parent,
>
>
> "acpi-gpe0", GPE_LEN);
>
>
> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
>
>
>
>
> - acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
>
>
> - s->use_acpi_hotplug_bridge);
>
>
> + if (s->use_acpi_pci_hotplug)
>
>
> + acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus,
>
> parent,
>
>
> + s->use_acpi_hotplug_bridge);
>
>
>
>
> s->cpu_hotplug_legacy = true;
>
>
> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
>
>
> @@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
>
>
> DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val,
>
> 2),
>
>
> DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support",
>
>
> PIIX4PMState,
>
>
> use_acpi_hotplug_bridge, true),
>
>
> + DEFINE_PROP_BOOL("acpi-pci-hotplug", PIIX4PMState,
>
>
> + use_acpi_pci_hotplug, true),
>
>
> DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>
>
> acpi_memory_hotplug.is_enabled, true),
>
>
> DEFINE_PROP_END_OF_LIST(),
>
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>
>
> index b7bcbbb..343b9b6 100644
>
>
> --- a/hw/i386/acpi-build.c
>
>
> +++ b/hw/i386/acpi-build.c
>
>
> @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
>
>
> bool s3_disabled;
>
>
> bool s4_disabled;
>
>
> bool pcihp_bridge_en;
>
>
> + bool pcihp_en;
>
>
> uint8_t s4_val;
>
>
> AcpiFadtData fadt;
>
>
> uint16_t cpu_hp_io_base;
>
>
> @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState
>
> *machine,
>
>
> AcpiPmInfo *pm)
>
>
> pm->pcihp_bridge_en =
>
>
> object_property_get_bool(obj,
>
>
> "acpi-pci-hotplug-with-bridge-support",
>
>
> NULL);
>
>
> + pm->pcihp_en =
>
>
> + object_property_get_bool(obj, "acpi-pci-hotplug", NULL);
>
>
> +
>
>
> }
>
>
>
>
> static void acpi_get_misc_info(AcpiMiscInfo *info)
>
>
> @@ -337,14 +341,16 @@ static void build_append_pcihp_notify_entry
>
> (Aml
>
>
> *method, int slot)
>
>
> }
>
>
>
>
> static void build_append_pci_bus_devices(Aml *parent_scope,
>
> PCIBus *bus,
>
>
> - bool pcihp_bridge_en)
>
>
> + bool pcihp_bridge_en,
>
> bool
>
>
> pcihp_en)
>
>
> {
>
>
> Aml *dev, *notify_method = NULL, *method;
>
>
> - QObject *bsel;
>
>
> + QObject *bsel = NULL;
>
>
> PCIBus *sec;
>
>
> int i;
>
>
>
>
> - bsel = object_property_get_qobject(OBJECT(bus),
>
>
> ACPI_PCIHP_PROP_BSEL, NULL);
>
>
> + if (pcihp_en)
>
>
> + bsel = object_property_get_qobject(OBJECT(bus),
>
>
> + ACPI_PCIHP_PROP_BSEL,
>
> NULL);
>
>
> if (bsel) {
>
>
> uint64_t bsel_val = qnum_get_uint(qobject_to(QNum,
>
> bsel));
>
>
>
>
> @@ -439,7 +445,8 @@ static void build_append_pci_bus_devices(Aml
>
>
> *parent_scope, PCIBus *bus,
>
>
> */
>
>
> PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE
>
> (pdev));
>
>
>
>
> - build_append_pci_bus_devices(dev, sec_bus,
>
> pcihp_bridge_en);
>
>
> + build_append_pci_bus_devices(dev, sec_bus,
>
> pcihp_bridge_en,
>
>
> + pcihp_en);
>
>
> }
>
>
> /* slot descriptor has been composed, add it into parent
>
> context
>
>
> */
>
>
> aml_append(parent_scope, dev);
>
>
> @@ -468,7 +475,7 @@ static void build_append_pci_bus_devices(Aml
>
>
> *parent_scope, PCIBus *bus,
>
>
> }
>
>
>
>
> /* Notify about child bus events in any case */
>
>
> - if (pcihp_bridge_en) {
>
>
> + if (pcihp_bridge_en && pcihp_en) {
>
>
> QLIST_FOREACH(sec, &bus->child, sibling) {
>
>
> int32_t devfn = sec->parent_dev->devfn;
>
>
>
>
> @@ -1818,7 +1825,8 @@ build_dsdt(GArray *table_data, BIOSLinker
>
> *linker,
>
>
> if (bus) {
>
>
> Aml *scope = aml_scope("PCI0");
>
>
> /* Scan all PCI buses. Generate tables to support
>
> hotplug. *
>
>
> /
>
>
> - build_append_pci_bus_devices(scope, bus, pm->
>
>
> pcihp_bridge_en);
>
>
> + build_append_pci_bus_devices(scope, bus, pm->
>
>
> pcihp_bridge_en,
>
>
> + pm->pcihp_en);
>
>
>
>
> if (TPM_IS_TIS_ISA(tpm)) {
>
>
> if (misc->tpm_version == TPM_VERSION_2_0) {
>
>
> --
>
>
> 2.7.4
>
>
>
>
>
>
>
>
>