| Message ID | 20200219145540.648365-1-jusual@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pcie_root_port: Add enable_hotplug option | expand |
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 <jusual@redhat.com> >--- >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(-) > Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, 19 Feb 2020 15:55:40 +0100 Julia Suvorova <jusual@redhat.com> 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 <jusual@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > 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; > }; >
On 2/19/20 9:55 AM, 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 <jusual@redhat.com> > --- > v1: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg04868.html > > v2: > * change name of the option to 'enable-hotplug' [Laine] Heh... I didn't actually expect you to do that just for me :-) (especially since I guess nobody else was bothered by "disable"). But now that you did, I look at it and realize that the "enable-" part is redundant, ie. just "hotplug=on|off|true|false" is plenty descriptive (since it's implied that it's being enabled). But I've already created too much of a tempest over such a tiny detail, and kind of wish I'd just kept quiet instead... I'll try to test this with libvirt in the next day or two. > * change order of enabling capability bits [Igor] > * enable HPS bit [Igor] > * add option to xio3130_downstream [Ján] >
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 <jusual@redhat.com> 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
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; };
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 <jusual@redhat.com> --- 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(-)