virtio,pc,acpi: features, fixes

[PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Michael S. Tsirkin 5 years, 3 months ago

From: Ani Sinha <ani@anisinha.ca>

We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
we can turn on or off PCI device hotplug on the root bus. This flag can be
used to prevent all PCI devices from getting hotplugged or unplugged from the
root PCI bus.
This feature is targetted mostly towards Windows VMs. It is useful in cases
where some hypervisor admins want to deploy guest VMs in a way so that the
users of the guest OSes are not able to hot-eject certain PCI devices from
the Windows system tray. Laine has explained the use case here in detail:
https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html

Julia has resolved this issue for PCIE buses with the following commit:
530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")

This commit attempts to introduce similar behavior for PCI root buses used in
i440fx machine types (although in this case, we do not have a per-slot
capability to turn hotplug on or off).

Usage:
   -global PIIX4_PM.acpi-root-pci-hotplug=off

By default, this option is enabled which means that hotplug is turned on for
the PCI root bus.

The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
bridges remain as is and can be used along with this new flag to control PCI
hotplug on PCI bridges.

This change has been tested using a Windows 2012R2 server guest image and also
with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
master qemu from upstream.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/pcihp.h |  2 +-
 hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
 hw/acpi/piix4.c         |  5 ++++-
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 8bc4a4c01d..02f4665767 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                          Error **errp);
 
 /* Called on reset */
-void acpi_pcihp_reset(AcpiPciHpState *s);
+void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
 
 extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
 
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 9e31ab2da4..39b1f74442 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
     }
 }
 
+static void acpi_pcihp_disable_root_bus(void)
+{
+    static bool root_hp_disabled;
+    PCIBus *bus;
+
+    if (root_hp_disabled) {
+        return;
+    }
+
+    bus = find_i440fx();
+    if (bus) {
+        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
+        qbus_set_hotplug_handler(BUS(bus), NULL);
+    }
+    root_hp_disabled = true;
+    return;
+}
+
 static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
 {
     AcpiPciHpFind *find = opaque;
@@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
     }
 }
 
-void acpi_pcihp_reset(AcpiPciHpState *s)
+void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
 {
+    if (acpihp_root_off) {
+        acpi_pcihp_disable_root_bus();
+    }
     acpi_set_pci_info();
     acpi_pcihp_update(s);
 }
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
 
     uint8_t disable_s3;
     uint8_t disable_s4;
@@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
         pci_conf[0x5B] = 0x02;
     }
     pm_io_space_update(s);
-    acpi_pcihp_reset(&s->acpi_pci_hotplug);
+    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
 }
 
 static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
@@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
+                     use_acpi_root_pci_hotplug, true),
     DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
                      acpi_memory_hotplug.is_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
-- 
MST

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Igor Mammedov 5 years, 3 months ago

On Thu, 27 Aug 2020 09:40:34 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> From: Ani Sinha <ani@anisinha.ca>
> 
> We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> we can turn on or off PCI device hotplug on the root bus. This flag can be
> used to prevent all PCI devices from getting hotplugged or unplugged from the
> root PCI bus.
> This feature is targetted mostly towards Windows VMs. It is useful in cases
> where some hypervisor admins want to deploy guest VMs in a way so that the
> users of the guest OSes are not able to hot-eject certain PCI devices from
> the Windows system tray. Laine has explained the use case here in detail:
> https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> 
> Julia has resolved this issue for PCIE buses with the following commit:
> 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> 
> This commit attempts to introduce similar behavior for PCI root buses used in
> i440fx machine types (although in this case, we do not have a per-slot
> capability to turn hotplug on or off).
> 
> Usage:
>    -global PIIX4_PM.acpi-root-pci-hotplug=off
> 
> By default, this option is enabled which means that hotplug is turned on for
> the PCI root bus.
> 
> The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> bridges remain as is and can be used along with this new flag to control PCI
> hotplug on PCI bridges.
> 
> This change has been tested using a Windows 2012R2 server guest image and also
> with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> master qemu from upstream.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


> Tested-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
A glitch in scripts?
I didn't review nor tested this (v8) version

> ---
>  include/hw/acpi/pcihp.h |  2 +-
>  hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
>  hw/acpi/piix4.c         |  5 ++++-
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 8bc4a4c01d..02f4665767 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                           Error **errp);
>  
>  /* Called on reset */
> -void acpi_pcihp_reset(AcpiPciHpState *s);
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
>  
>  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
>  
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 9e31ab2da4..39b1f74442 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
>      }
>  }
>  
> +static void acpi_pcihp_disable_root_bus(void)
> +{
> +    static bool root_hp_disabled;
> +    PCIBus *bus;
> +
> +    if (root_hp_disabled) {
> +        return;
> +    }
> +
> +    bus = find_i440fx();
> +    if (bus) {
> +        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> +        qbus_set_hotplug_handler(BUS(bus), NULL);
> +    }
> +    root_hp_disabled = true;
> +    return;
> +}
> +
>  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
>  {
>      AcpiPciHpFind *find = opaque;
> @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
>      }
>  }
>  
> -void acpi_pcihp_reset(AcpiPciHpState *s)
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
>  {
> +    if (acpihp_root_off) {
> +        acpi_pcihp_disable_root_bus();
> +    }
>      acpi_set_pci_info();
>      acpi_pcihp_update(s);
>  }
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
>  
>      uint8_t disable_s3;
>      uint8_t disable_s4;
> @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
>          pci_conf[0x5B] = 0x02;
>      }
>      pm_io_space_update(s);
> -    acpi_pcihp_reset(&s->acpi_pci_hotplug);
> +    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
>  }
>  
>  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> +                     use_acpi_root_pci_hotplug, true),
>      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>                       acpi_memory_hotplug.is_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Ani Sinha 5 years, 3 months ago

On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 27 Aug 2020 09:40:34 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > From: Ani Sinha <ani@anisinha.ca>
> >
> > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > we can turn on or off PCI device hotplug on the root bus. This flag can be
> > used to prevent all PCI devices from getting hotplugged or unplugged from the
> > root PCI bus.
> > This feature is targetted mostly towards Windows VMs. It is useful in cases
> > where some hypervisor admins want to deploy guest VMs in a way so that the
> > users of the guest OSes are not able to hot-eject certain PCI devices from
> > the Windows system tray. Laine has explained the use case here in detail:
> > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> >
> > Julia has resolved this issue for PCIE buses with the following commit:
> > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> >
> > This commit attempts to introduce similar behavior for PCI root buses used in
> > i440fx machine types (although in this case, we do not have a per-slot
> > capability to turn hotplug on or off).
> >
> > Usage:
> >    -global PIIX4_PM.acpi-root-pci-hotplug=off
> >
> > By default, this option is enabled which means that hotplug is turned on for
> > the PCI root bus.
> >
> > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > bridges remain as is and can be used along with this new flag to control PCI
> > hotplug on PCI bridges.
> >
> > This change has been tested using a Windows 2012R2 server guest image and also
> > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > master qemu from upstream.
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> A glitch in scripts?
> I didn't review nor tested this (v8) version

oops! I am so eager to get this done and dusted :)

>
> > ---
> >  include/hw/acpi/pcihp.h |  2 +-
> >  hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
> >  hw/acpi/piix4.c         |  5 ++++-
> >  3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > index 8bc4a4c01d..02f4665767 100644
> > --- a/include/hw/acpi/pcihp.h
> > +++ b/include/hw/acpi/pcihp.h
> > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >                                           Error **errp);
> >
> >  /* Called on reset */
> > -void acpi_pcihp_reset(AcpiPciHpState *s);
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> >
> >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 9e31ab2da4..39b1f74442 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> >      }
> >  }
> >
> > +static void acpi_pcihp_disable_root_bus(void)
> > +{
> > +    static bool root_hp_disabled;
> > +    PCIBus *bus;
> > +
> > +    if (root_hp_disabled) {
> > +        return;
> > +    }
> > +
> > +    bus = find_i440fx();
> > +    if (bus) {
> > +        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> > +        qbus_set_hotplug_handler(BUS(bus), NULL);
> > +    }
> > +    root_hp_disabled = true;
> > +    return;
> > +}
> > +
> >  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> >  {
> >      AcpiPciHpFind *find = opaque;
> > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> >      }
> >  }
> >
> > -void acpi_pcihp_reset(AcpiPciHpState *s)
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> >  {
> > +    if (acpihp_root_off) {
> > +        acpi_pcihp_disable_root_bus();
> > +    }
> >      acpi_set_pci_info();
> >      acpi_pcihp_update(s);
> >  }
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> >
> >      uint8_t disable_s3;
> >      uint8_t disable_s4;
> > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> >          pci_conf[0x5B] = 0x02;
> >      }
> >      pm_io_space_update(s);
> > -    acpi_pcihp_reset(&s->acpi_pci_hotplug);
> > +    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> >  }
> >
> >  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> > +                     use_acpi_root_pci_hotplug, true),
> >      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> >                       acpi_memory_hotplug.is_enabled, true),
> >      DEFINE_PROP_END_OF_LIST(),
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Igor Mammedov 5 years, 3 months ago

On Thu, 27 Aug 2020 23:29:34 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >  
> > > From: Ani Sinha <ani@anisinha.ca>
> > >
> > > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > > we can turn on or off PCI device hotplug on the root bus. This flag can be
> > > used to prevent all PCI devices from getting hotplugged or unplugged from the
> > > root PCI bus.
> > > This feature is targetted mostly towards Windows VMs. It is useful in cases
> > > where some hypervisor admins want to deploy guest VMs in a way so that the
> > > users of the guest OSes are not able to hot-eject certain PCI devices from
> > > the Windows system tray. Laine has explained the use case here in detail:
> > > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> > >
> > > Julia has resolved this issue for PCIE buses with the following commit:
> > > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> > >
> > > This commit attempts to introduce similar behavior for PCI root buses used in
> > > i440fx machine types (although in this case, we do not have a per-slot
> > > capability to turn hotplug on or off).
> > >
> > > Usage:
> > >    -global PIIX4_PM.acpi-root-pci-hotplug=off
> > >
> > > By default, this option is enabled which means that hotplug is turned on for
> > > the PCI root bus.
> > >
> > > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > > bridges remain as is and can be used along with this new flag to control PCI
> > > hotplug on PCI bridges.
> > >
> > > This change has been tested using a Windows 2012R2 server guest image and also
> > > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > > master qemu from upstream.
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>  
> >
> >  
> > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>  
> > A glitch in scripts?
> > I didn't review nor tested this (v8) version  
> 
> oops! I am so eager to get this done and dusted :)
it's merged now,

can you add a test case for it please?

You can use test_acpi_piix4_tcg_bridge() as model.
See header comment at the top of bios-tables-test.c
for how to prepare and submit testcase.

> 
> >  
> > > ---
> > >  include/hw/acpi/pcihp.h |  2 +-
> > >  hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
> > >  hw/acpi/piix4.c         |  5 ++++-
> > >  3 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > index 8bc4a4c01d..02f4665767 100644
> > > --- a/include/hw/acpi/pcihp.h
> > > +++ b/include/hw/acpi/pcihp.h
> > > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > >                                           Error **errp);
> > >
> > >  /* Called on reset */
> > > -void acpi_pcihp_reset(AcpiPciHpState *s);
> > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > >
> > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > >
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 9e31ab2da4..39b1f74442 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> > >      }
> > >  }
> > >
> > > +static void acpi_pcihp_disable_root_bus(void)
> > > +{
> > > +    static bool root_hp_disabled;
> > > +    PCIBus *bus;
> > > +
> > > +    if (root_hp_disabled) {
> > > +        return;
> > > +    }
> > > +
> > > +    bus = find_i440fx();
> > > +    if (bus) {
> > > +        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> > > +        qbus_set_hotplug_handler(BUS(bus), NULL);
> > > +    }
> > > +    root_hp_disabled = true;
> > > +    return;
> > > +}
> > > +
> > >  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > >  {
> > >      AcpiPciHpFind *find = opaque;
> > > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> > >      }
> > >  }
> > >
> > > -void acpi_pcihp_reset(AcpiPciHpState *s)
> > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > >  {
> > > +    if (acpihp_root_off) {
> > > +        acpi_pcihp_disable_root_bus();
> > > +    }
> > >      acpi_set_pci_info();
> > >      acpi_pcihp_update(s);
> > >  }
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> > >
> > >      uint8_t disable_s3;
> > >      uint8_t disable_s4;
> > > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> > >          pci_conf[0x5B] = 0x02;
> > >      }
> > >      pm_io_space_update(s);
> > > -    acpi_pcihp_reset(&s->acpi_pci_hotplug);
> > > +    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> > >  }
> > >
> > >  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> > > +                     use_acpi_root_pci_hotplug, true),
> > >      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > >                       acpi_memory_hotplug.is_enabled, true),
> > >      DEFINE_PROP_END_OF_LIST(),  
> >  
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Ani Sinha 5 years, 3 months ago

Ani
On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> On Thu, 27 Aug 2020 23:29:34 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Thu, 27 Aug 2020 09:40:34 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > From: Ani Sinha <ani@anisinha.ca>
> > > >
> > > > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > > > we can turn on or off PCI device hotplug on the root bus. This flag can be
> > > > used to prevent all PCI devices from getting hotplugged or unplugged from the
> > > > root PCI bus.
> > > > This feature is targetted mostly towards Windows VMs. It is useful in cases
> > > > where some hypervisor admins want to deploy guest VMs in a way so that the
> > > > users of the guest OSes are not able to hot-eject certain PCI devices from
> > > > the Windows system tray. Laine has explained the use case here in detail:
> > > > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> > > >
> > > > Julia has resolved this issue for PCIE buses with the following commit:
> > > > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> > > >
> > > > This commit attempts to introduce similar behavior for PCI root buses used in
> > > > i440fx machine types (although in this case, we do not have a per-slot
> > > > capability to turn hotplug on or off).
> > > >
> > > > Usage:
> > > > -global PIIX4_PM.acpi-root-pci-hotplug=off
> > > >
> > > > By default, this option is enabled which means that hotplug is turned on for
> > > > the PCI root bus.
> > > >
> > > > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > > > bridges remain as is and can be used along with this new flag to control PCI
> > > > hotplug on PCI bridges.
> > > >
> > > > This change has been tested using a Windows 2012R2 server guest image and also
> > > > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > > > master qemu from upstream.
> > > >
> > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > A glitch in scripts?
> > > I didn't review nor tested this (v8) version
> >
> > oops! I am so eager to get this done and dusted :)
> it's merged now,

Wait it merged without your review?
>
> can you add a test case for it please?
>
> You can use test_acpi_piix4_tcg_bridge() as model.
> See header comment at the top of bios-tables-test.c
> for how to prepare and submit testcase.

Will get on it.
>
> >
> > >
> > > > ---
> > > > include/hw/acpi/pcihp.h | 2 +-
> > > > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > > > hw/acpi/piix4.c | 5 ++++-
> > > > 3 files changed, 27 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > > index 8bc4a4c01d..02f4665767 100644
> > > > --- a/include/hw/acpi/pcihp.h
> > > > +++ b/include/hw/acpi/pcihp.h
> > > > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > > Error **errp);
> > > >
> > > > /* Called on reset */
> > > > -void acpi_pcihp_reset(AcpiPciHpState *s);
> > > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > > >
> > > > extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > > >
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index 9e31ab2da4..39b1f74442 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> > > > }
> > > > }
> > > >
> > > > +static void acpi_pcihp_disable_root_bus(void)
> > > > +{
> > > > + static bool root_hp_disabled;
> > > > + PCIBus *bus;
> > > > +
> > > > + if (root_hp_disabled) {
> > > > + return;
> > > > + }
> > > > +
> > > > + bus = find_i440fx();
> > > > + if (bus) {
> > > > + /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> > > > + qbus_set_hotplug_handler(BUS(bus), NULL);
> > > > + }
> > > > + root_hp_disabled = true;
> > > > + return;
> > > > +}
> > > > +
> > > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > > > {
> > > > AcpiPciHpFind *find = opaque;
> > > > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> > > > }
> > > > }
> > > >
> > > > -void acpi_pcihp_reset(AcpiPciHpState *s)
> > > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > > > {
> > > > + if (acpihp_root_off) {
> > > > + acpi_pcihp_disable_root_bus();
> > > > + }
> > > > acpi_set_pci_info();
> > > > acpi_pcihp_update(s);
> > > > }
> > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> > > >
> > > > uint8_t disable_s3;
> > > > uint8_t disable_s4;
> > > > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> > > > pci_conf[0x5B] = 0x02;
> > > > }
> > > > pm_io_space_update(s);
> > > > - acpi_pcihp_reset(&s->acpi_pci_hotplug);
> > > > + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> > > > }
> > > >
> > > > static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > > > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> > > > + use_acpi_root_pci_hotplug, true),
> > > > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > > > acpi_memory_hotplug.is_enabled, true),
> > > > DEFINE_PROP_END_OF_LIST(),
> > >
> >
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Julia Suvorova 5 years, 3 months ago

On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
>
>
> Ani
> On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
>
> On Thu, 27 Aug 2020 23:29:34 +0530
>
> Ani Sinha <ani@anisinha.ca> wrote:
>
>
> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
>
> On Thu, 27 Aug 2020 09:40:34 -0400
>
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>
> From: Ani Sinha <ani@anisinha.ca>
>
>
> We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
>
> we can turn on or off PCI device hotplug on the root bus. This flag can be
>
> used to prevent all PCI devices from getting hotplugged or unplugged from the
>
> root PCI bus.
>
> This feature is targetted mostly towards Windows VMs. It is useful in cases
>
> where some hypervisor admins want to deploy guest VMs in a way so that the
>
> users of the guest OSes are not able to hot-eject certain PCI devices from
>
> the Windows system tray. Laine has explained the use case here in detail:
>
> https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
>
>
> Julia has resolved this issue for PCIE buses with the following commit:
>
> 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
>
>
> This commit attempts to introduce similar behavior for PCI root buses used in
>
> i440fx machine types (although in this case, we do not have a per-slot
>
> capability to turn hotplug on or off).
>
>
> Usage:
>
> -global PIIX4_PM.acpi-root-pci-hotplug=off
>
>
> By default, this option is enabled which means that hotplug is turned on for
>
> the PCI root bus.
>
>
> The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
>
> bridges remain as is and can be used along with this new flag to control PCI
>
> hotplug on PCI bridges.
>
>
> This change has been tested using a Windows 2012R2 server guest image and also
>
> with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
>
> master qemu from upstream.
>
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>
> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
>
> Tested-by: Igor Mammedov <imammedo@redhat.com>
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
> A glitch in scripts?
>
> I didn't review nor tested this (v8) version
>
>
> oops! I am so eager to get this done and dusted :)
>
> it's merged now,
>
>
> Wait it merged without your review?

Yeah, not only added into the pull request, but actually merged.

>
> can you add a test case for it please?
>
>
> You can use test_acpi_piix4_tcg_bridge() as model.
>
> See header comment at the top of bios-tables-test.c
>
> for how to prepare and submit testcase.
>
>
> Will get on it.

Also, while the whole approach seems good to me, it leaves the hotplug
registers initialized (see build_piix4_pci_hotplug()), even if both
root and bridges hotplug are disabled. Which you can exploit by
writing something like this to the io registers:

outl 0xae10 0
outl 0xae08 your_slot

And because of these quite interesting lines the device will be
successfully unplugged:

static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
{
...
    /* Make bsel 0 eject root bus if bsel property is not set,
     * for compatibility with non acpi setups.
     * TODO: really needed?
     */
    if (!bsel && !find.bus) {
        find.bus = s->root;
    }
    return find.bus;
}

Could you please cover both issues in the follow-up patches?

Best regards, Julia Suvorova.

>
>
>
> ---
>
> include/hw/acpi/pcihp.h | 2 +-
>
> hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
>
> hw/acpi/piix4.c | 5 ++++-
>
> 3 files changed, 27 insertions(+), 3 deletions(-)
>
>
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
>
> index 8bc4a4c01d..02f4665767 100644
>
> --- a/include/hw/acpi/pcihp.h
>
> +++ b/include/hw/acpi/pcihp.h
>
> @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>
> Error **errp);
>
>
> /* Called on reset */
>
> -void acpi_pcihp_reset(AcpiPciHpState *s);
>
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
>
>
> extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
>
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>
> index 9e31ab2da4..39b1f74442 100644
>
> --- a/hw/acpi/pcihp.c
>
> +++ b/hw/acpi/pcihp.c
>
> @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
>
> }
>
> }
>
>
> +static void acpi_pcihp_disable_root_bus(void)
>
> +{
>
> + static bool root_hp_disabled;
>
> + PCIBus *bus;
>
> +
>
> + if (root_hp_disabled) {
>
> + return;
>
> + }
>
> +
>
> + bus = find_i440fx();
>
> + if (bus) {
>
> + /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
>
> + qbus_set_hotplug_handler(BUS(bus), NULL);
>
> + }
>
> + root_hp_disabled = true;
>
> + return;
>
> +}
>
> +
>
> static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
>
> {
>
> AcpiPciHpFind *find = opaque;
>
> @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
>
> }
>
> }
>
>
> -void acpi_pcihp_reset(AcpiPciHpState *s)
>
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
>
> {
>
> + if (acpihp_root_off) {
>
> + acpi_pcihp_disable_root_bus();
>
> + }
>
> acpi_set_pci_info();
>
> acpi_pcihp_update(s);
>
> }
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>
> index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
>
>
> uint8_t disable_s3;
>
> uint8_t disable_s4;
>
> @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
>
> pci_conf[0x5B] = 0x02;
>
> }
>
> pm_io_space_update(s);
>
> - acpi_pcihp_reset(&s->acpi_pci_hotplug);
>
> + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
>
> }
>
>
> static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>
> @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
>
> + use_acpi_root_pci_hotplug, true),
>
> DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>
> acpi_memory_hotplug.is_enabled, true),
>
> DEFINE_PROP_END_OF_LIST(),
>
>
>
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Ani Sinha 5 years, 3 months ago

On Aug 28, 2020, 18:40 +0530, Julia Suvorova <jusual@redhat.com>, wrote:
> On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > Ani
> > On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> >
> > On Thu, 27 Aug 2020 23:29:34 +0530
> >
> > Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> >
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >
> > From: Ani Sinha <ani@anisinha.ca>
> >
> >
> > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> >
> > we can turn on or off PCI device hotplug on the root bus. This flag can be
> >
> > used to prevent all PCI devices from getting hotplugged or unplugged from the
> >
> > root PCI bus.
> >
> > This feature is targetted mostly towards Windows VMs. It is useful in cases
> >
> > where some hypervisor admins want to deploy guest VMs in a way so that the
> >
> > users of the guest OSes are not able to hot-eject certain PCI devices from
> >
> > the Windows system tray. Laine has explained the use case here in detail:
> >
> > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> >
> >
> > Julia has resolved this issue for PCIE buses with the following commit:
> >
> > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> >
> >
> > This commit attempts to introduce similar behavior for PCI root buses used in
> >
> > i440fx machine types (although in this case, we do not have a per-slot
> >
> > capability to turn hotplug on or off).
> >
> >
> > Usage:
> >
> > -global PIIX4_PM.acpi-root-pci-hotplug=off
> >
> >
> > By default, this option is enabled which means that hotplug is turned on for
> >
> > the PCI root bus.
> >
> >
> > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> >
> > bridges remain as is and can be used along with this new flag to control PCI
> >
> > hotplug on PCI bridges.
> >
> >
> > This change has been tested using a Windows 2012R2 server guest image and also
> >
> > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> >
> > master qemu from upstream.
> >
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> >
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> >
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >
> > A glitch in scripts?
> >
> > I didn't review nor tested this (v8) version
> >
> >
> > oops! I am so eager to get this done and dusted :)
> >
> > it's merged now,
> >
> >
> > Wait it merged without your review?
>
> Yeah, not only added into the pull request, but actually merged.
>
> >
> > can you add a test case for it please?
> >
> >
> > You can use test_acpi_piix4_tcg_bridge() as model.
> >
> > See header comment at the top of bios-tables-test.c
> >
> > for how to prepare and submit testcase.
> >
> >
> > Will get on it.
>
> Also, while the whole approach seems good to me, it leaves the hotplug
> registers initialized (see build_piix4_pci_hotplug()), even if both
> root and bridges hotplug are disabled. Which you can exploit by
> writing something like this to the io registers:
>
> outl 0xae10 0

You’re setting bsel 0 with this line correct?
> outl 0xae08 your_slot
>
> And because of these quite interesting lines the device will be
> successfully unplugged:
>
> static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
> {
> ...
> /* Make bsel 0 eject root bus if bsel property is not set,
> * for compatibility with non acpi setups.
> * TODO: really needed?
> */
> if (!bsel && !find.bus) {
> find.bus = s->root;
> }
> return find.bus;
> }
>
> Could you please cover both issues in the follow-up patches?

Can you please explain what do you mean by both issues? The unit test issue and leaving the registers initialized?

>
> Best regards, Julia Suvorova.
>
> >
> >
> >
> > ---
> >
> > include/hw/acpi/pcihp.h | 2 +-
> >
> > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> >
> > hw/acpi/piix4.c | 5 ++++-
> >
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> >
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> >
> > index 8bc4a4c01d..02f4665767 100644
> >
> > --- a/include/hw/acpi/pcihp.h
> >
> > +++ b/include/hw/acpi/pcihp.h
> >
> > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >
> > Error **errp);
> >
> >
> > /* Called on reset */
> >
> > -void acpi_pcihp_reset(AcpiPciHpState *s);
> >
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> >
> >
> > extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> >
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >
> > index 9e31ab2da4..39b1f74442 100644
> >
> > --- a/hw/acpi/pcihp.c
> >
> > +++ b/hw/acpi/pcihp.c
> >
> > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> >
> > }
> >
> > }
> >
> >
> > +static void acpi_pcihp_disable_root_bus(void)
> >
> > +{
> >
> > + static bool root_hp_disabled;
> >
> > + PCIBus *bus;
> >
> > +
> >
> > + if (root_hp_disabled) {
> >
> > + return;
> >
> > + }
> >
> > +
> >
> > + bus = find_i440fx();
> >
> > + if (bus) {
> >
> > + /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> >
> > + qbus_set_hotplug_handler(BUS(bus), NULL);
> >
> > + }
> >
> > + root_hp_disabled = true;
> >
> > + return;
> >
> > +}
> >
> > +
> >
> > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> >
> > {
> >
> > AcpiPciHpFind *find = opaque;
> >
> > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> >
> > }
> >
> > }
> >
> >
> > -void acpi_pcihp_reset(AcpiPciHpState *s)
> >
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> >
> > {
> >
> > + if (acpihp_root_off) {
> >
> > + acpi_pcihp_disable_root_bus();
> >
> > + }
> >
> > acpi_set_pci_info();
> >
> > acpi_pcihp_update(s);
> >
> > }
> >
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >
> > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> >
> >
> > uint8_t disable_s3;
> >
> > uint8_t disable_s4;
> >
> > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> >
> > pci_conf[0x5B] = 0x02;
> >
> > }
> >
> > pm_io_space_update(s);
> >
> > - acpi_pcihp_reset(&s->acpi_pci_hotplug);
> >
> > + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> >
> > }
> >
> >
> > static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> >
> > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> >
> > + use_acpi_root_pci_hotplug, true),
> >
> > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> >
> > acpi_memory_hotplug.is_enabled, true),
> >
> > DEFINE_PROP_END_OF_LIST(),
> >
> >
> >
> >
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Julia Suvorova 5 years, 3 months ago

On Fri, Aug 28, 2020 at 3:16 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Aug 28, 2020, 18:40 +0530, Julia Suvorova <jusual@redhat.com>, wrote:
>
> On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> Ani
>
> On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
>
>
> On Thu, 27 Aug 2020 23:29:34 +0530
>
>
> Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
>
>
> On Thu, 27 Aug 2020 09:40:34 -0400
>
>
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>
>
> From: Ani Sinha <ani@anisinha.ca>
>
>
>
> We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
>
>
> we can turn on or off PCI device hotplug on the root bus. This flag can be
>
>
> used to prevent all PCI devices from getting hotplugged or unplugged from the
>
>
> root PCI bus.
>
>
> This feature is targetted mostly towards Windows VMs. It is useful in cases
>
>
> where some hypervisor admins want to deploy guest VMs in a way so that the
>
>
> users of the guest OSes are not able to hot-eject certain PCI devices from
>
>
> the Windows system tray. Laine has explained the use case here in detail:
>
>
> https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
>
>
>
> Julia has resolved this issue for PCIE buses with the following commit:
>
>
> 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
>
>
>
> This commit attempts to introduce similar behavior for PCI root buses used in
>
>
> i440fx machine types (although in this case, we do not have a per-slot
>
>
> capability to turn hotplug on or off).
>
>
>
> Usage:
>
>
> -global PIIX4_PM.acpi-root-pci-hotplug=off
>
>
>
> By default, this option is enabled which means that hotplug is turned on for
>
>
> the PCI root bus.
>
>
>
> The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
>
>
> bridges remain as is and can be used along with this new flag to control PCI
>
>
> hotplug on PCI bridges.
>
>
>
> This change has been tested using a Windows 2012R2 server guest image and also
>
>
> with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
>
>
> master qemu from upstream.
>
>
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>
>
> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
>
>
>
> Tested-by: Igor Mammedov <imammedo@redhat.com>
>
>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
>
> A glitch in scripts?
>
>
> I didn't review nor tested this (v8) version
>
>
>
> oops! I am so eager to get this done and dusted :)
>
>
> it's merged now,
>
>
>
> Wait it merged without your review?
>
>
> Yeah, not only added into the pull request, but actually merged.
>
>
>
> can you add a test case for it please?
>
>
>
> You can use test_acpi_piix4_tcg_bridge() as model.
>
>
> See header comment at the top of bios-tables-test.c
>
>
> for how to prepare and submit testcase.
>
>
>
> Will get on it.
>
>
> Also, while the whole approach seems good to me, it leaves the hotplug
>
> registers initialized (see build_piix4_pci_hotplug()), even if both
>
> root and bridges hotplug are disabled. Which you can exploit by
>
> writing something like this to the io registers:
>
>
> outl 0xae10 0
>
>
> You’re setting bsel 0 with this line correct?

Yes, it selects bsel.

> outl 0xae08 your_slot
>
>
> And because of these quite interesting lines the device will be
>
> successfully unplugged:
>
>
> static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
>
> {
>
> ...
>
> /* Make bsel 0 eject root bus if bsel property is not set,
>
> * for compatibility with non acpi setups.
>
> * TODO: really needed?
>
> */
>
> if (!bsel && !find.bus) {
>
> find.bus = s->root;
>
> }
>
> return find.bus;
>
> }
>
>
> Could you please cover both issues in the follow-up patches?
>
>
> Can you please explain what do you mean by both issues? The unit test issue and leaving the registers initialized?

No, I mean initializing registers and returning root as default in
acpi_pcihp_find_hotplug_bus() in addition to Igor's notes.
Returning NULL if hotplug is disabled for root should be fine, unless
Michael confirms that we can remove this check completely.

> Best regards, Julia Suvorova.
>
>
>
>
>
> ---
>
>
> include/hw/acpi/pcihp.h | 2 +-
>
>
> hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
>
>
> hw/acpi/piix4.c | 5 ++++-
>
>
> 3 files changed, 27 insertions(+), 3 deletions(-)
>
>
>
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
>
>
> index 8bc4a4c01d..02f4665767 100644
>
>
> --- a/include/hw/acpi/pcihp.h
>
>
> +++ b/include/hw/acpi/pcihp.h
>
>
> @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>
>
> Error **errp);
>
>
>
> /* Called on reset */
>
>
> -void acpi_pcihp_reset(AcpiPciHpState *s);
>
>
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
>
>
>
> extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
>
>
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>
>
> index 9e31ab2da4..39b1f74442 100644
>
>
> --- a/hw/acpi/pcihp.c
>
>
> +++ b/hw/acpi/pcihp.c
>
>
> @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
>
>
> }
>
>
> }
>
>
>
> +static void acpi_pcihp_disable_root_bus(void)
>
>
> +{
>
>
> + static bool root_hp_disabled;
>
>
> + PCIBus *bus;
>
>
> +
>
>
> + if (root_hp_disabled) {
>
>
> + return;
>
>
> + }
>
>
> +
>
>
> + bus = find_i440fx();
>
>
> + if (bus) {
>
>
> + /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
>
>
> + qbus_set_hotplug_handler(BUS(bus), NULL);
>
>
> + }
>
>
> + root_hp_disabled = true;
>
>
> + return;
>
>
> +}
>
>
> +
>
>
> static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
>
>
> {
>
>
> AcpiPciHpFind *find = opaque;
>
>
> @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
>
>
> }
>
>
> }
>
>
>
> -void acpi_pcihp_reset(AcpiPciHpState *s)
>
>
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
>
>
> {
>
>
> + if (acpihp_root_off) {
>
>
> + acpi_pcihp_disable_root_bus();
>
>
> + }
>
>
> acpi_set_pci_info();
>
>
> acpi_pcihp_update(s);
>
>
> }
>
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>
>
> index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
>
>
>
> uint8_t disable_s3;
>
>
> uint8_t disable_s4;
>
>
> @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
>
>
> pci_conf[0x5B] = 0x02;
>
>
> }
>
>
> pm_io_space_update(s);
>
>
> - acpi_pcihp_reset(&s->acpi_pci_hotplug);
>
>
> + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
>
>
> }
>
>
>
> static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>
>
> @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
>
>
> + use_acpi_root_pci_hotplug, true),
>
>
> DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>
>
> acpi_memory_hotplug.is_enabled, true),
>
>
> DEFINE_PROP_END_OF_LIST(),
>
>
>
>
>
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Ani Sinha 5 years, 3 months ago

On Fri, Aug 28, 2020 at 6:40 PM Julia Suvorova <jusual@redhat.com> wrote:
>
> On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > Ani
> > On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> >
> > On Thu, 27 Aug 2020 23:29:34 +0530
> >
> > Ani Sinha <ani@anisinha.ca> wrote:
> >
> >
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> >
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >
> > From: Ani Sinha <ani@anisinha.ca>
> >
> >
> > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> >
> > we can turn on or off PCI device hotplug on the root bus. This flag can be
> >
> > used to prevent all PCI devices from getting hotplugged or unplugged from the
> >
> > root PCI bus.
> >
> > This feature is targetted mostly towards Windows VMs. It is useful in cases
> >
> > where some hypervisor admins want to deploy guest VMs in a way so that the
> >
> > users of the guest OSes are not able to hot-eject certain PCI devices from
> >
> > the Windows system tray. Laine has explained the use case here in detail:
> >
> > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> >
> >
> > Julia has resolved this issue for PCIE buses with the following commit:
> >
> > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> >
> >
> > This commit attempts to introduce similar behavior for PCI root buses used in
> >
> > i440fx machine types (although in this case, we do not have a per-slot
> >
> > capability to turn hotplug on or off).
> >
> >
> > Usage:
> >
> > -global PIIX4_PM.acpi-root-pci-hotplug=off
> >
> >
> > By default, this option is enabled which means that hotplug is turned on for
> >
> > the PCI root bus.
> >
> >
> > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> >
> > bridges remain as is and can be used along with this new flag to control PCI
> >
> > hotplug on PCI bridges.
> >
> >
> > This change has been tested using a Windows 2012R2 server guest image and also
> >
> > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> >
> > master qemu from upstream.
> >
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> >
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> >
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >
> > A glitch in scripts?
> >
> > I didn't review nor tested this (v8) version
> >
> >
> > oops! I am so eager to get this done and dusted :)
> >
> > it's merged now,
> >
> >
> > Wait it merged without your review?
>
> Yeah, not only added into the pull request, but actually merged.
>
> >
> > can you add a test case for it please?
> >
> >
> > You can use test_acpi_piix4_tcg_bridge() as model.
> >
> > See header comment at the top of bios-tables-test.c
> >
> > for how to prepare and submit testcase.
> >
> >
> > Will get on it.
>
> Also, while the whole approach seems good to me, it leaves the hotplug
> registers initialized (see build_piix4_pci_hotplug()), even if both
> root and bridges hotplug are disabled. Which you can exploit by
> writing something like this to the io registers:
>
> outl 0xae10 0
> outl 0xae08 your_slot
>
> And because of these quite interesting lines the device will be
> successfully unplugged:
>
> static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
> {
> ...
>     /* Make bsel 0 eject root bus if bsel property is not set,
>      * for compatibility with non acpi setups.
>      * TODO: really needed?
>      */
>     if (!bsel && !find.bus) {
>         find.bus = s->root;
>     }
>     return find.bus;
> }
>

I have sent a patch to address this. Please review.

> Could you please cover both issues in the follow-up patches?

Also julia, looking at the code, should we also set pm->pcihp_io_len
to 0 as well in case where bridge and pci bus 0 hotplugs are both
disabled? This is because I see:

   /* reserve PCIHP resources */
    if (pm->pcihp_io_len) {

which we do not want to do if PCI hotplug is disabled altogether?

>
> Best regards, Julia Suvorova.
>
> >
> >
> >
> > ---
> >
> > include/hw/acpi/pcihp.h | 2 +-
> >
> > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> >
> > hw/acpi/piix4.c | 5 ++++-
> >
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> >
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> >
> > index 8bc4a4c01d..02f4665767 100644
> >
> > --- a/include/hw/acpi/pcihp.h
> >
> > +++ b/include/hw/acpi/pcihp.h
> >
> > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >
> > Error **errp);
> >
> >
> > /* Called on reset */
> >
> > -void acpi_pcihp_reset(AcpiPciHpState *s);
> >
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> >
> >
> > extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> >
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> >
> > index 9e31ab2da4..39b1f74442 100644
> >
> > --- a/hw/acpi/pcihp.c
> >
> > +++ b/hw/acpi/pcihp.c
> >
> > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> >
> > }
> >
> > }
> >
> >
> > +static void acpi_pcihp_disable_root_bus(void)
> >
> > +{
> >
> > + static bool root_hp_disabled;
> >
> > + PCIBus *bus;
> >
> > +
> >
> > + if (root_hp_disabled) {
> >
> > + return;
> >
> > + }
> >
> > +
> >
> > + bus = find_i440fx();
> >
> > + if (bus) {
> >
> > + /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> >
> > + qbus_set_hotplug_handler(BUS(bus), NULL);
> >
> > + }
> >
> > + root_hp_disabled = true;
> >
> > + return;
> >
> > +}
> >
> > +
> >
> > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> >
> > {
> >
> > AcpiPciHpFind *find = opaque;
> >
> > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> >
> > }
> >
> > }
> >
> >
> > -void acpi_pcihp_reset(AcpiPciHpState *s)
> >
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> >
> > {
> >
> > + if (acpihp_root_off) {
> >
> > + acpi_pcihp_disable_root_bus();
> >
> > + }
> >
> > acpi_set_pci_info();
> >
> > acpi_pcihp_update(s);
> >
> > }
> >
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> >
> > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> >
> >
> > uint8_t disable_s3;
> >
> > uint8_t disable_s4;
> >
> > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> >
> > pci_conf[0x5B] = 0x02;
> >
> > }
> >
> > pm_io_space_update(s);
> >
> > - acpi_pcihp_reset(&s->acpi_pci_hotplug);
> >
> > + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> >
> > }
> >
> >
> > static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> >
> > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> >
> > + use_acpi_root_pci_hotplug, true),
> >
> > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> >
> > acpi_memory_hotplug.is_enabled, true),
> >
> > DEFINE_PROP_END_OF_LIST(),
> >
> >
> >
> >
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Ani Sinha 5 years, 3 months ago

On Tue, Sep 1, 2020 at 11:57 AM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Fri, Aug 28, 2020 at 6:40 PM Julia Suvorova <jusual@redhat.com> wrote:
> >
> > On Fri, Aug 28, 2020 at 11:53 AM Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > >
> > > Ani
> > > On Aug 28, 2020, 15:19 +0530, Igor Mammedov <imammedo@redhat.com>, wrote:
> > >
> > > On Thu, 27 Aug 2020 23:29:34 +0530
> > >
> > > Ani Sinha <ani@anisinha.ca> wrote:
> > >
> > >
> > > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > >
> > > On Thu, 27 Aug 2020 09:40:34 -0400
> > >
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > >
> > > From: Ani Sinha <ani@anisinha.ca>
> > >
> > >
> > > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > >
> > > we can turn on or off PCI device hotplug on the root bus. This flag can be
> > >
> > > used to prevent all PCI devices from getting hotplugged or unplugged from the
> > >
> > > root PCI bus.
> > >
> > > This feature is targetted mostly towards Windows VMs. It is useful in cases
> > >
> > > where some hypervisor admins want to deploy guest VMs in a way so that the
> > >
> > > users of the guest OSes are not able to hot-eject certain PCI devices from
> > >
> > > the Windows system tray. Laine has explained the use case here in detail:
> > >
> > > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> > >
> > >
> > > Julia has resolved this issue for PCIE buses with the following commit:
> > >
> > > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> > >
> > >
> > > This commit attempts to introduce similar behavior for PCI root buses used in
> > >
> > > i440fx machine types (although in this case, we do not have a per-slot
> > >
> > > capability to turn hotplug on or off).
> > >
> > >
> > > Usage:
> > >
> > > -global PIIX4_PM.acpi-root-pci-hotplug=off
> > >
> > >
> > > By default, this option is enabled which means that hotplug is turned on for
> > >
> > > the PCI root bus.
> > >
> > >
> > > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > >
> > > bridges remain as is and can be used along with this new flag to control PCI
> > >
> > > hotplug on PCI bridges.
> > >
> > >
> > > This change has been tested using a Windows 2012R2 server guest image and also
> > >
> > > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > >
> > > master qemu from upstream.
> > >
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > >
> > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > >
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > >
> > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > >
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > >
> > > A glitch in scripts?
> > >
> > > I didn't review nor tested this (v8) version
> > >
> > >
> > > oops! I am so eager to get this done and dusted :)
> > >
> > > it's merged now,
> > >
> > >
> > > Wait it merged without your review?
> >
> > Yeah, not only added into the pull request, but actually merged.
> >
> > >
> > > can you add a test case for it please?
> > >
> > >
> > > You can use test_acpi_piix4_tcg_bridge() as model.
> > >
> > > See header comment at the top of bios-tables-test.c
> > >
> > > for how to prepare and submit testcase.
> > >
> > >
> > > Will get on it.
> >
> > Also, while the whole approach seems good to me, it leaves the hotplug
> > registers initialized (see build_piix4_pci_hotplug()), even if both
> > root and bridges hotplug are disabled. Which you can exploit by
> > writing something like this to the io registers:
> >
> > outl 0xae10 0
> > outl 0xae08 your_slot
> >
> > And because of these quite interesting lines the device will be
> > successfully unplugged:
> >
> > static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
> > {
> > ...
> >     /* Make bsel 0 eject root bus if bsel property is not set,
> >      * for compatibility with non acpi setups.
> >      * TODO: really needed?
> >      */
> >     if (!bsel && !find.bus) {
> >         find.bus = s->root;
> >     }
> >     return find.bus;
> > }
> >
>
> I have sent a patch to address this. Please review.
>
> > Could you please cover both issues in the follow-up patches?
>
> Also julia, looking at the code, should we also set pm->pcihp_io_len
> to 0 as well in case where bridge and pci bus 0 hotplugs are both
> disabled?

I have sent a RFC patch where I check if at least one of the two
options is ON in the conditional below. Seems the right thing to do
instead of setting the io_len to 0.

This is because I see:
>
>    /* reserve PCIHP resources */
>     if (pm->pcihp_io_len) {
>
> which we do not want to do if PCI hotplug is disabled altogether?
>
> >
> > Best regards, Julia Suvorova.
> >
> > >
> > >
> > >
> > > ---
> > >
> > > include/hw/acpi/pcihp.h | 2 +-
> > >
> > > hw/acpi/pcihp.c | 23 ++++++++++++++++++++++-
> > >
> > > hw/acpi/piix4.c | 5 ++++-
> > >
> > > 3 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > >
> > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > >
> > > index 8bc4a4c01d..02f4665767 100644
> > >
> > > --- a/include/hw/acpi/pcihp.h
> > >
> > > +++ b/include/hw/acpi/pcihp.h
> > >
> > > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > >
> > > Error **errp);
> > >
> > >
> > > /* Called on reset */
> > >
> > > -void acpi_pcihp_reset(AcpiPciHpState *s);
> > >
> > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > >
> > >
> > > extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > >
> > >
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > >
> > > index 9e31ab2da4..39b1f74442 100644
> > >
> > > --- a/hw/acpi/pcihp.c
> > >
> > > +++ b/hw/acpi/pcihp.c
> > >
> > > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> > >
> > > }
> > >
> > > }
> > >
> > >
> > > +static void acpi_pcihp_disable_root_bus(void)
> > >
> > > +{
> > >
> > > + static bool root_hp_disabled;
> > >
> > > + PCIBus *bus;
> > >
> > > +
> > >
> > > + if (root_hp_disabled) {
> > >
> > > + return;
> > >
> > > + }
> > >
> > > +
> > >
> > > + bus = find_i440fx();
> > >
> > > + if (bus) {
> > >
> > > + /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> > >
> > > + qbus_set_hotplug_handler(BUS(bus), NULL);
> > >
> > > + }
> > >
> > > + root_hp_disabled = true;
> > >
> > > + return;
> > >
> > > +}
> > >
> > > +
> > >
> > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > >
> > > {
> > >
> > > AcpiPciHpFind *find = opaque;
> > >
> > > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> > >
> > > }
> > >
> > > }
> > >
> > >
> > > -void acpi_pcihp_reset(AcpiPciHpState *s)
> > >
> > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > >
> > > {
> > >
> > > + if (acpihp_root_off) {
> > >
> > > + acpi_pcihp_disable_root_bus();
> > >
> > > + }
> > >
> > > acpi_set_pci_info();
> > >
> > > acpi_pcihp_update(s);
> > >
> > > }
> > >
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > >
> > > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> > >
> > >
> > > uint8_t disable_s3;
> > >
> > > uint8_t disable_s4;
> > >
> > > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> > >
> > > pci_conf[0x5B] = 0x02;
> > >
> > > }
> > >
> > > pm_io_space_update(s);
> > >
> > > - acpi_pcihp_reset(&s->acpi_pci_hotplug);
> > >
> > > + acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> > >
> > > }
> > >
> > >
> > > static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > >
> > > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> > >
> > > + use_acpi_root_pci_hotplug, true),
> > >
> > > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > >
> > > acpi_memory_hotplug.is_enabled, true),
> > >
> > > DEFINE_PROP_END_OF_LIST(),
> > >
> > >
> > >
> > >
> >

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Ani Sinha 5 years, 3 months ago

On Fri, Aug 28, 2020 at 3:19 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 27 Aug 2020 23:29:34 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Thu, 27 Aug 2020 09:40:34 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > From: Ani Sinha <ani@anisinha.ca>
> > > >
> > > > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > > > we can turn on or off PCI device hotplug on the root bus. This flag can be
> > > > used to prevent all PCI devices from getting hotplugged or unplugged from the
> > > > root PCI bus.
> > > > This feature is targetted mostly towards Windows VMs. It is useful in cases
> > > > where some hypervisor admins want to deploy guest VMs in a way so that the
> > > > users of the guest OSes are not able to hot-eject certain PCI devices from
> > > > the Windows system tray. Laine has explained the use case here in detail:
> > > > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> > > >
> > > > Julia has resolved this issue for PCIE buses with the following commit:
> > > > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> > > >
> > > > This commit attempts to introduce similar behavior for PCI root buses used in
> > > > i440fx machine types (although in this case, we do not have a per-slot
> > > > capability to turn hotplug on or off).
> > > >
> > > > Usage:
> > > >    -global PIIX4_PM.acpi-root-pci-hotplug=off
> > > >
> > > > By default, this option is enabled which means that hotplug is turned on for
> > > > the PCI root bus.
> > > >
> > > > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > > > bridges remain as is and can be used along with this new flag to control PCI
> > > > hotplug on PCI bridges.
> > > >
> > > > This change has been tested using a Windows 2012R2 server guest image and also
> > > > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > > > master qemu from upstream.
> > > >
> > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >
> > > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > A glitch in scripts?
> > > I didn't review nor tested this (v8) version
> >
> > oops! I am so eager to get this done and dusted :)
> it's merged now,
>
> can you add a test case for it please?

Yes just sent the unit test patch set.

>
> You can use test_acpi_piix4_tcg_bridge() as model.
> See header comment at the top of bios-tables-test.c
> for how to prepare and submit testcase.
>
> >
> > >
> > > > ---
> > > >  include/hw/acpi/pcihp.h |  2 +-
> > > >  hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
> > > >  hw/acpi/piix4.c         |  5 ++++-
> > > >  3 files changed, 27 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > > index 8bc4a4c01d..02f4665767 100644
> > > > --- a/include/hw/acpi/pcihp.h
> > > > +++ b/include/hw/acpi/pcihp.h
> > > > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > >                                           Error **errp);
> > > >
> > > >  /* Called on reset */
> > > > -void acpi_pcihp_reset(AcpiPciHpState *s);
> > > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > > >
> > > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > > >
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index 9e31ab2da4..39b1f74442 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> > > >      }
> > > >  }
> > > >
> > > > +static void acpi_pcihp_disable_root_bus(void)
> > > > +{
> > > > +    static bool root_hp_disabled;
> > > > +    PCIBus *bus;
> > > > +
> > > > +    if (root_hp_disabled) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    bus = find_i440fx();
> > > > +    if (bus) {
> > > > +        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> > > > +        qbus_set_hotplug_handler(BUS(bus), NULL);
> > > > +    }
> > > > +    root_hp_disabled = true;
> > > > +    return;
> > > > +}
> > > > +
> > > >  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > > >  {
> > > >      AcpiPciHpFind *find = opaque;
> > > > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> > > >      }
> > > >  }
> > > >
> > > > -void acpi_pcihp_reset(AcpiPciHpState *s)
> > > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > > >  {
> > > > +    if (acpihp_root_off) {
> > > > +        acpi_pcihp_disable_root_bus();
> > > > +    }
> > > >      acpi_set_pci_info();
> > > >      acpi_pcihp_update(s);
> > > >  }
> > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> > > >
> > > >      uint8_t disable_s3;
> > > >      uint8_t disable_s4;
> > > > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> > > >          pci_conf[0x5B] = 0x02;
> > > >      }
> > > >      pm_io_space_update(s);
> > > > -    acpi_pcihp_reset(&s->acpi_pci_hotplug);
> > > > +    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> > > >  }
> > > >
> > > >  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > > > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> > > > +                     use_acpi_root_pci_hotplug, true),
> > > >      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > > >                       acpi_memory_hotplug.is_enabled, true),
> > > >      DEFINE_PROP_END_OF_LIST(),
> > >
> >
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Michael S. Tsirkin 5 years, 3 months ago

On Thu, Aug 27, 2020 at 11:29:34PM +0530, Ani Sinha wrote:
> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 27 Aug 2020 09:40:34 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > From: Ani Sinha <ani@anisinha.ca>
> > >
> > > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > > we can turn on or off PCI device hotplug on the root bus. This flag can be
> > > used to prevent all PCI devices from getting hotplugged or unplugged from the
> > > root PCI bus.
> > > This feature is targetted mostly towards Windows VMs. It is useful in cases
> > > where some hypervisor admins want to deploy guest VMs in a way so that the
> > > users of the guest OSes are not able to hot-eject certain PCI devices from
> > > the Windows system tray. Laine has explained the use case here in detail:
> > > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> > >
> > > Julia has resolved this issue for PCIE buses with the following commit:
> > > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> > >
> > > This commit attempts to introduce similar behavior for PCI root buses used in
> > > i440fx machine types (although in this case, we do not have a per-slot
> > > capability to turn hotplug on or off).
> > >
> > > Usage:
> > >    -global PIIX4_PM.acpi-root-pci-hotplug=off
> > >
> > > By default, this option is enabled which means that hotplug is turned on for
> > > the PCI root bus.
> > >
> > > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > > bridges remain as is and can be used along with this new flag to control PCI
> > > hotplug on PCI bridges.
> > >
> > > This change has been tested using a Windows 2012R2 server guest image and also
> > > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > > master qemu from upstream.
> > >
> > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > A glitch in scripts?
> > I didn't review nor tested this (v8) version
> 
> oops! I am so eager to get this done and dusted :)


One thing I would like to see is that all of these flags
should also disable the relevant functionality.



> >
> > > ---
> > >  include/hw/acpi/pcihp.h |  2 +-
> > >  hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
> > >  hw/acpi/piix4.c         |  5 ++++-
> > >  3 files changed, 27 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > > index 8bc4a4c01d..02f4665767 100644
> > > --- a/include/hw/acpi/pcihp.h
> > > +++ b/include/hw/acpi/pcihp.h
> > > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > >                                           Error **errp);
> > >
> > >  /* Called on reset */
> > > -void acpi_pcihp_reset(AcpiPciHpState *s);
> > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> > >
> > >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> > >
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 9e31ab2da4..39b1f74442 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> > >      }
> > >  }
> > >
> > > +static void acpi_pcihp_disable_root_bus(void)
> > > +{
> > > +    static bool root_hp_disabled;
> > > +    PCIBus *bus;
> > > +
> > > +    if (root_hp_disabled) {
> > > +        return;
> > > +    }
> > > +
> > > +    bus = find_i440fx();
> > > +    if (bus) {
> > > +        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> > > +        qbus_set_hotplug_handler(BUS(bus), NULL);
> > > +    }
> > > +    root_hp_disabled = true;
> > > +    return;
> > > +}
> > > +
> > >  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > >  {
> > >      AcpiPciHpFind *find = opaque;
> > > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> > >      }
> > >  }
> > >
> > > -void acpi_pcihp_reset(AcpiPciHpState *s)
> > > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> > >  {
> > > +    if (acpihp_root_off) {
> > > +        acpi_pcihp_disable_root_bus();
> > > +    }
> > >      acpi_set_pci_info();
> > >      acpi_pcihp_update(s);
> > >  }
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> > >
> > >      uint8_t disable_s3;
> > >      uint8_t disable_s4;
> > > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> > >          pci_conf[0x5B] = 0x02;
> > >      }
> > >      pm_io_space_update(s);
> > > -    acpi_pcihp_reset(&s->acpi_pci_hotplug);
> > > +    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> > >  }
> > >
> > >  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> > > +                     use_acpi_root_pci_hotplug, true),
> > >      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > >                       acpi_memory_hotplug.is_enabled, true),
> > >      DEFINE_PROP_END_OF_LIST(),
> >

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Ani Sinha 5 years, 3 months ago


> On Aug 31, 2020, at 2:27 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Thu, Aug 27, 2020 at 11:29:34PM +0530, Ani Sinha wrote:
>>> On Thu, Aug 27, 2020 at 11:11 PM Igor Mammedov <imammedo@redhat.com> wrote:
>>> 
>>> On Thu, 27 Aug 2020 09:40:34 -0400
>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> 
>>>> From: Ani Sinha <ani@anisinha.ca>
>>>> 
>>>> We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
>>>> we can turn on or off PCI device hotplug on the root bus. This flag can be
>>>> used to prevent all PCI devices from getting hotplugged or unplugged from the
>>>> root PCI bus.
>>>> This feature is targetted mostly towards Windows VMs. It is useful in cases
>>>> where some hypervisor admins want to deploy guest VMs in a way so that the
>>>> users of the guest OSes are not able to hot-eject certain PCI devices from
>>>> the Windows system tray. Laine has explained the use case here in detail:
>>>> https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
>>>> 
>>>> Julia has resolved this issue for PCIE buses with the following commit:
>>>> 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
>>>> 
>>>> This commit attempts to introduce similar behavior for PCI root buses used in
>>>> i440fx machine types (although in this case, we do not have a per-slot
>>>> capability to turn hotplug on or off).
>>>> 
>>>> Usage:
>>>>   -global PIIX4_PM.acpi-root-pci-hotplug=off
>>>> 
>>>> By default, this option is enabled which means that hotplug is turned on for
>>>> the PCI root bus.
>>>> 
>>>> The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
>>>> bridges remain as is and can be used along with this new flag to control PCI
>>>> hotplug on PCI bridges.
>>>> 
>>>> This change has been tested using a Windows 2012R2 server guest image and also
>>>> with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
>>>> master qemu from upstream.
>>>> 
>>>> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>>>> Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> 
>>> 
>>>> Tested-by: Igor Mammedov <imammedo@redhat.com>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>> A glitch in scripts?
>>> I didn't review nor tested this (v8) version
>> 
>> oops! I am so eager to get this done and dusted :)
> 
> 
> One thing I would like to see is that all of these flags
> should also disable the relevant functionality.

The flag that I introduced would with this patch along with the other patch I sent to address Julia's concern over ejecting root bus when bsel is absent.

> 
> 
> 
>>> 
>>>> ---
>>>> include/hw/acpi/pcihp.h |  2 +-
>>>> hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
>>>> hw/acpi/piix4.c         |  5 ++++-
>>>> 3 files changed, 27 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
>>>> index 8bc4a4c01d..02f4665767 100644
>>>> --- a/include/hw/acpi/pcihp.h
>>>> +++ b/include/hw/acpi/pcihp.h
>>>> @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>>                                          Error **errp);
>>>> 
>>>> /* Called on reset */
>>>> -void acpi_pcihp_reset(AcpiPciHpState *s);
>>>> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
>>>> 
>>>> extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
>>>> 
>>>> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>>>> index 9e31ab2da4..39b1f74442 100644
>>>> --- a/hw/acpi/pcihp.c
>>>> +++ b/hw/acpi/pcihp.c
>>>> @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
>>>>     }
>>>> }
>>>> 
>>>> +static void acpi_pcihp_disable_root_bus(void)
>>>> +{
>>>> +    static bool root_hp_disabled;
>>>> +    PCIBus *bus;
>>>> +
>>>> +    if (root_hp_disabled) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bus = find_i440fx();
>>>> +    if (bus) {
>>>> +        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
>>>> +        qbus_set_hotplug_handler(BUS(bus), NULL);
>>>> +    }
>>>> +    root_hp_disabled = true;
>>>> +    return;
>>>> +}
>>>> +
>>>> static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
>>>> {
>>>>     AcpiPciHpFind *find = opaque;
>>>> @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
>>>>     }
>>>> }
>>>> 
>>>> -void acpi_pcihp_reset(AcpiPciHpState *s)
>>>> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
>>>> {
>>>> +    if (acpihp_root_off) {
>>>> +        acpi_pcihp_disable_root_bus();
>>>> +    }
>>>>     acpi_set_pci_info();
>>>>     acpi_pcihp_update(s);
>>>> }
>>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>>>> index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
>>>> 
>>>>     uint8_t disable_s3;
>>>>     uint8_t disable_s4;
>>>> @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
>>>>         pci_conf[0x5B] = 0x02;
>>>>     }
>>>>     pm_io_space_update(s);
>>>> -    acpi_pcihp_reset(&s->acpi_pci_hotplug);
>>>> +    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
>>>> }
>>>> 
>>>> static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>>>> @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
>>>> +                     use_acpi_root_pci_hotplug, true),
>>>>     DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
>>>>                      acpi_memory_hotplug.is_enabled, true),
>>>>     DEFINE_PROP_END_OF_LIST(),
>>> 
>

Re: [PULL 06/13] Introduce a new flag for i440fx to disable PCI hotplug on the root bus

Posted by Michael S. Tsirkin 5 years, 3 months ago

On Thu, Aug 27, 2020 at 07:41:15PM +0200, Igor Mammedov wrote:
> On Thu, 27 Aug 2020 09:40:34 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > From: Ani Sinha <ani@anisinha.ca>
> > 
> > We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
> > we can turn on or off PCI device hotplug on the root bus. This flag can be
> > used to prevent all PCI devices from getting hotplugged or unplugged from the
> > root PCI bus.
> > This feature is targetted mostly towards Windows VMs. It is useful in cases
> > where some hypervisor admins want to deploy guest VMs in a way so that the
> > users of the guest OSes are not able to hot-eject certain PCI devices from
> > the Windows system tray. Laine has explained the use case here in detail:
> > https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
> > 
> > Julia has resolved this issue for PCIE buses with the following commit:
> > 530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
> > 
> > This commit attempts to introduce similar behavior for PCI root buses used in
> > i440fx machine types (although in this case, we do not have a per-slot
> > capability to turn hotplug on or off).
> > 
> > Usage:
> >    -global PIIX4_PM.acpi-root-pci-hotplug=off
> > 
> > By default, this option is enabled which means that hotplug is turned on for
> > the PCI root bus.
> > 
> > The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
> > bridges remain as is and can be used along with this new flag to control PCI
> > hotplug on PCI bridges.
> > 
> > This change has been tested using a Windows 2012R2 server guest image and also
> > with a Windows 2019 server guest image on a Ubuntu 18.04 host using the latest
> > master qemu from upstream.
> > 
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > Message-Id: <20200821165403.26589-1-ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> > Tested-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> A glitch in scripts?
> I didn't review nor tested this (v8) version

I guess I picked the tags from the previous version.
Are you ok with this one?

> > ---
> >  include/hw/acpi/pcihp.h |  2 +-
> >  hw/acpi/pcihp.c         | 23 ++++++++++++++++++++++-
> >  hw/acpi/piix4.c         |  5 ++++-
> >  3 files changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> > index 8bc4a4c01d..02f4665767 100644
> > --- a/include/hw/acpi/pcihp.h
> > +++ b/include/hw/acpi/pcihp.h
> > @@ -67,7 +67,7 @@ void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >                                           Error **errp);
> >  
> >  /* Called on reset */
> > -void acpi_pcihp_reset(AcpiPciHpState *s);
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off);
> >  
> >  extern const VMStateDescription vmstate_acpi_pcihp_pci_status;
> >  
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 9e31ab2da4..39b1f74442 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -104,6 +104,24 @@ static void acpi_set_pci_info(void)
> >      }
> >  }
> >  
> > +static void acpi_pcihp_disable_root_bus(void)
> > +{
> > +    static bool root_hp_disabled;
> > +    PCIBus *bus;
> > +
> > +    if (root_hp_disabled) {
> > +        return;
> > +    }
> > +
> > +    bus = find_i440fx();
> > +    if (bus) {
> > +        /* setting the hotplug handler to NULL makes the bus non-hotpluggable */
> > +        qbus_set_hotplug_handler(BUS(bus), NULL);
> > +    }
> > +    root_hp_disabled = true;
> > +    return;
> > +}
> > +
> >  static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> >  {
> >      AcpiPciHpFind *find = opaque;
> > @@ -209,8 +227,11 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> >      }
> >  }
> >  
> > -void acpi_pcihp_reset(AcpiPciHpState *s)
> > +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> >  {
> > +    if (acpihp_root_off) {
> > +        acpi_pcihp_disable_root_bus();
> > +    }
> >      acpi_set_pci_info();
> >      acpi_pcihp_update(s);
> >  }
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 26bac4f16c..e6163bb6ce 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_root_pci_hotplug;
> >  
> >      uint8_t disable_s3;
> >      uint8_t disable_s4;
> > @@ -324,7 +325,7 @@ static void piix4_pm_reset(DeviceState *dev)
> >          pci_conf[0x5B] = 0x02;
> >      }
> >      pm_io_space_update(s);
> > -    acpi_pcihp_reset(&s->acpi_pci_hotplug);
> > +    acpi_pcihp_reset(&s->acpi_pci_hotplug, !s->use_acpi_root_pci_hotplug);
> >  }
> >  
> >  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > @@ -635,6 +636,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-root-pci-hotplug", PIIX4PMState,
> > +                     use_acpi_root_pci_hotplug, true),
> >      DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> >                       acpi_memory_hotplug.is_enabled, true),
> >      DEFINE_PROP_END_OF_LIST(),