Re: [PATCH v2] pcie_root_port: Add enable_hotplug option

[Michael S. Tsirkin]

On Wed, Feb 19, 2020 at 03:55:40PM +0100, Julia Suvorova wrote:
> Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt
> manage it and restrict unplug for the whole machine. This is going to
> prevent user-initiated unplug in guests (Windows mostly).
> Hotplug is enabled by default.
> Usage:
>     -device pcie-root-port,enable-hotplug=false,...
> 
> If you want to disable hot-unplug on some downstream ports of one
> switch, disable hot-unplug on PCIe Root Port connected to the upstream
> port as well as on the selected downstream ports.
> 
> Discussion related:
>     https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00530.html
> 
> Signed-off-by: Julia Suvorova <address@hidden>

OK but now that I look at it, can't we put this
property on PCIE slot? We really need it for downstream
root ports too, and it seems attractive to be able to just specify
it on TYPE_PCIE_SLOT.

> ---
> v1: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04868.html
> 
> v2:
>     * change name of the option to 'enable-hotplug' [Laine]
>     * change order of enabling capability bits [Igor]
>     * enable HPS bit [Igor]
>     * add option to xio3130_downstream [Ján]
> 
>  hw/pci-bridge/pcie_root_port.c     |  3 ++-
>  hw/pci-bridge/xio3130_downstream.c |  3 ++-
>  hw/pci/pcie.c                      | 11 +++++++----
>  include/hw/pci/pcie.h              |  2 +-
>  include/hw/pci/pcie_port.h         |  1 +
>  5 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 0ba4e4dea4..6eb2bc4564 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -94,7 +94,7 @@ static void rp_realize(PCIDevice *d, Error **errp)
>  
>      pcie_cap_arifwd_init(d);
>      pcie_cap_deverr_init(d);
> -    pcie_cap_slot_init(d, s->slot);
> +    pcie_cap_slot_init(d, s);
>      pcie_cap_root_init(d);
>  
>      pcie_chassis_create(s->chassis);
> @@ -147,6 +147,7 @@ static Property rp_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
>      DEFINE_PROP_BOOL("disable-acs", PCIESlot, disable_acs, false),
> +    DEFINE_PROP_BOOL("enable-hotplug", PCIESlot, enable_hotplug, true),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/hw/pci-bridge/xio3130_downstream.c 
> b/hw/pci-bridge/xio3130_downstream.c
> index 153a4acad2..e8c388c547 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -94,7 +94,7 @@ static void xio3130_downstream_realize(PCIDevice *d, Error 
> **errp)
>      }
>      pcie_cap_flr_init(d);
>      pcie_cap_deverr_init(d);
> -    pcie_cap_slot_init(d, s->slot);
> +    pcie_cap_slot_init(d, s);
>      pcie_cap_arifwd_init(d);
>  
>      pcie_chassis_create(s->chassis);
> @@ -136,6 +136,7 @@ static void xio3130_downstream_exitfn(PCIDevice *d)
>  static Property xio3130_downstream_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> +    DEFINE_PROP_BOOL("enable-hotplug", PCIESlot, enable_hotplug, true),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 08718188bb..a963c0f82e 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -495,7 +495,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler 
> *hotplug_dev,
>  
>  /* pci express slot for pci express root/downstream port
>     PCI express capability slot registers */
> -void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
> +void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
>  {
>      uint32_t pos = dev->exp.exp_cap;
>  
> @@ -505,13 +505,16 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
>      pci_long_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCAP,
>                                   ~PCI_EXP_SLTCAP_PSN);
>      pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCAP,
> -                               (slot << PCI_EXP_SLTCAP_PSN_SHIFT) |
> +                               (s->slot << PCI_EXP_SLTCAP_PSN_SHIFT) |
>                                 PCI_EXP_SLTCAP_EIP |
> -                               PCI_EXP_SLTCAP_HPS |
> -                               PCI_EXP_SLTCAP_HPC |
>                                 PCI_EXP_SLTCAP_PIP |
>                                 PCI_EXP_SLTCAP_AIP |
>                                 PCI_EXP_SLTCAP_ABP);
> +    if (s->enable_hotplug) {
> +        pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCAP,
> +                                   PCI_EXP_SLTCAP_HPS |
> +                                   PCI_EXP_SLTCAP_HPC);
> +    }
>  
>      if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
>          pci_long_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCAP,
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 7064875835..14c58ebdb6 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -104,7 +104,7 @@ void pcie_cap_deverr_reset(PCIDevice *dev);
>  void pcie_cap_lnkctl_init(PCIDevice *dev);
>  void pcie_cap_lnkctl_reset(PCIDevice *dev);
>  
> -void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot);
> +void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s);
>  void pcie_cap_slot_reset(PCIDevice *dev);
>  void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta);
>  void pcie_cap_slot_write_config(PCIDevice *dev,
> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> index 4b3d254b08..71be598dda 100644
> --- a/include/hw/pci/pcie_port.h
> +++ b/include/hw/pci/pcie_port.h
> @@ -55,6 +55,7 @@ struct PCIESlot {
>  
>      /* Disable ACS (really for a pcie_root_port) */
>      bool        disable_acs;
> +    bool        enable_hotplug;
>      QLIST_ENTRY(PCIESlot) next;
>  };
>  
> -- 
> 2.24.1