| Message ID | 20200218161717.386723-1-jusual@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pcie_root_port: Add disable_hotplug option | expand |
On Tue, 18 Feb 2020 17:17:17 +0100 Julia Suvorova <jusual@redhat.com> wrote: > Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt > to manage it and restrict unplug for the entire machine. This is going > to prevent user-initiated unplug in guests (Windows mostly). > Usage: > -device pcie-root-port,disable-hotplug=true,... > > Discussion related: > https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00530.html > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > --- > hw/core/machine.c | 1 + > hw/pci-bridge/pcie_root_port.c | 3 ++- > hw/pci-bridge/xio3130_downstream.c | 2 +- > hw/pci/pcie.c | 8 ++++++-- > include/hw/pci/pcie.h | 2 +- > include/hw/pci/pcie_port.h | 1 + > 6 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 84812a1d1c..5ff698ac3c 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -36,6 +36,7 @@ GlobalProperty hw_compat_4_2[] = { > { "usb-redir", "suppress-remote-wake", "off" }, > { "qxl", "revision", "4" }, > { "qxl-vga", "revision", "4" }, > + { "pcie-root-port-base", "disable-hotplug", "false" }, this looks unnecessary as the property is 'off' by default > }; > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > index 0ba4e4dea4..d6a080bee8 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("disable-hotplug", PCIESlot, disable_hotplug, false), > DEFINE_PROP_END_OF_LIST() > }; > > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index 153a4acad2..04aae72cd6 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); > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 08718188bb..6dd9b89e21 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,17 @@ 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 | why not to set it to begin with instead of clearing it later as afterthought? Also only _HPC but _HPS is left, wouldn't it confuse guests? > PCI_EXP_SLTCAP_PIP | > PCI_EXP_SLTCAP_AIP | > PCI_EXP_SLTCAP_ABP); > + if (s->disable_hotplug) { > + pci_long_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCAP, > + 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..796fc8f3ab 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 disable_hotplug; > QLIST_ENTRY(PCIESlot) next; > }; >
On 2/18/20 11:17 AM, Julia Suvorova wrote: > Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt > to manage it and restrict unplug for the entire machine. This is going > to prevent user-initiated unplug in guests (Windows mostly). > Usage: > -device pcie-root-port,disable-hotplug=true,... Double negatives (e.g. "disable-hotplug=false") tend to confuse simple minds like mine. Would it be any more difficult to make the name of the option positive instead (e.g. "enable-hotplug") with the default set to "true"?
On Tue, Feb 18, 2020 at 05:17:17PM +0100, Julia Suvorova wrote: >Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt >to manage it and restrict unplug for the entire machine. This is going >to prevent user-initiated unplug in guests (Windows mostly). >Usage: > -device pcie-root-port,disable-hotplug=true,... > >Discussion related: > https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00530.html > >Signed-off-by: Julia Suvorova <jusual@redhat.com> >--- > hw/core/machine.c | 1 + > hw/pci-bridge/pcie_root_port.c | 3 ++- > hw/pci-bridge/xio3130_downstream.c | 2 +- > hw/pci/pcie.c | 8 ++++++-- > include/hw/pci/pcie.h | 2 +- > include/hw/pci/pcie_port.h | 1 + > 6 files changed, 12 insertions(+), 5 deletions(-) > >diff --git a/hw/core/machine.c b/hw/core/machine.c >index 84812a1d1c..5ff698ac3c 100644 >--- a/hw/core/machine.c >+++ b/hw/core/machine.c >@@ -36,6 +36,7 @@ GlobalProperty hw_compat_4_2[] = { > { "usb-redir", "suppress-remote-wake", "off" }, > { "qxl", "revision", "4" }, > { "qxl-vga", "revision", "4" }, >+ { "pcie-root-port-base", "disable-hotplug", "false" }, > }; > const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); > >diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c >index 0ba4e4dea4..d6a080bee8 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("disable-hotplug", PCIESlot, disable_hotplug, false), > DEFINE_PROP_END_OF_LIST() > }; > >diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c >index 153a4acad2..04aae72cd6 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); > The corresponding entry in xio3130_downstream_props[] is missing. > pcie_chassis_create(s->chassis); Jano
On Tue, Feb 18, 2020 at 6:18 PM Laine Stump <laine@redhat.com> wrote: > > On 2/18/20 11:17 AM, Julia Suvorova wrote: > > Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt > > to manage it and restrict unplug for the entire machine. This is going > > to prevent user-initiated unplug in guests (Windows mostly). > > Usage: > > -device pcie-root-port,disable-hotplug=true,... > > Double negatives (e.g. "disable-hotplug=false") tend to confuse simple > minds like mine. Would it be any more difficult to make the name of the > option positive instead (e.g. "enable-hotplug") with the default set to > "true"? disable-hotplug=false will not be used, because it's default. And it follows previous naming (''disable-acs'). Best regards, Julia Suvorova.
On 2/18/20 1:40 PM, Julia Suvorova wrote: > On Tue, Feb 18, 2020 at 6:18 PM Laine Stump <laine@redhat.com> wrote: >> >> On 2/18/20 11:17 AM, Julia Suvorova wrote: >>> Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt >>> to manage it and restrict unplug for the entire machine. This is going >>> to prevent user-initiated unplug in guests (Windows mostly). >>> Usage: >>> -device pcie-root-port,disable-hotplug=true,... >> >> Double negatives (e.g. "disable-hotplug=false") tend to confuse simple >> minds like mine. Would it be any more difficult to make the name of the >> option positive instead (e.g. "enable-hotplug") with the default set to >> "true"? > > disable-hotplug=false will not be used, because it's default. And it > follows previous naming (''disable-acs'). Yeah, I don't like the name of that one either (or of "disable-modern" or "disable-legacy") but I don't follow qemu-devel closely so I didn't see them when their patches went by. But now is my chance to complain :-) I can live with it either way, but still think it's much better to not have "negative" option names. Feel free to ignore, and I'll just be happy that I didn't accept it silently. Also, is there a rhyme/reason for some options having true/false, and some being off/on? disable-acs seems to be true/false, but disable-modern is on/off. Doesn't make any difference to me in the end, but just thought I'd bring it up in case there might be a reason to use on/off instead of true/false for this one.
On Tue, Feb 18, 2020 at 10:02:19PM -0500, Laine Stump wrote: > Also, is there a rhyme/reason for some options having true/false, and some > being off/on? disable-acs seems to be true/false, but disable-modern is > on/off. Doesn't make any difference to me in the end, but just thought I'd > bring it up in case there might be a reason to use on/off instead of > true/false for this one. Some places accept on/off, some true/false, some on/off/true/false others on/off/yes/no and others on/off/true/false/yes/no. In this case both user visitor machinery. Which I *think* means on/off is the safe choice and true/false can be broken in some places. We really should clean up this mess ... Julia, what do you think? Let's make them all support all options?
On Wed, Feb 19, 2020 at 4:47 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Feb 18, 2020 at 10:02:19PM -0500, Laine Stump wrote: > > Also, is there a rhyme/reason for some options having true/false, and some > > being off/on? disable-acs seems to be true/false, but disable-modern is > > on/off. Doesn't make any difference to me in the end, but just thought I'd > > bring it up in case there might be a reason to use on/off instead of > > true/false for this one. > > Some places accept on/off, some true/false, some on/off/true/false > others on/off/yes/no and others on/off/true/false/yes/no. > > In this case both user visitor machinery. Which I *think* > means on/off is the safe choice and true/false can be > broken in some places. > > We really should clean up this mess ... Julia, what do you think? > Let's make them all support all options? Options already support all of on/off/true/false/yes/no as long as they are defined as boolean (look at parse_type_bool()). That is, you can use disable-modern with yes/no/true/false too. The only problem is with types OnOffSplit and OnOffAuto (as in disable-legacy). Best regards, Julia Suvorova.
On Tue, Feb 18, 2020 at 12:18:04PM -0500, Laine Stump wrote: > On 2/18/20 11:17 AM, Julia Suvorova wrote: > > Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt > > to manage it and restrict unplug for the entire machine. This is going > > to prevent user-initiated unplug in guests (Windows mostly). > > Usage: > > -device pcie-root-port,disable-hotplug=true,... > > Double negatives (e.g. "disable-hotplug=false") tend to confuse simple minds > like mine. Would it be any more difficult to make the name of the option > positive instead (e.g. "enable-hotplug") with the default set to "true"? Or simply "hotpluggable=on|off" Regards, Daniel
On Wed, Feb 19, 2020 at 01:21:10PM +0100, Julia Suvorova wrote: > On Wed, Feb 19, 2020 at 4:47 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Feb 18, 2020 at 10:02:19PM -0500, Laine Stump wrote: > > > Also, is there a rhyme/reason for some options having true/false, and some > > > being off/on? disable-acs seems to be true/false, but disable-modern is > > > on/off. Doesn't make any difference to me in the end, but just thought I'd > > > bring it up in case there might be a reason to use on/off instead of > > > true/false for this one. > > > > Some places accept on/off, some true/false, some on/off/true/false > > others on/off/yes/no and others on/off/true/false/yes/no. > > > > In this case both user visitor machinery. Which I *think* > > means on/off is the safe choice and true/false can be > > broken in some places. > > > > We really should clean up this mess ... Julia, what do you think? > > Let's make them all support all options? > > Options already support all of on/off/true/false/yes/no as long as > they are defined as boolean (look at parse_type_bool()). That is, you > can use disable-modern with yes/no/true/false too. > The only problem is with types OnOffSplit and OnOffAuto (as in disable-legacy). Right. Not only that though - parse_option_bool can only handle on/off. target/sparc/cpu.c can handle on/off and true/false. JSON bool is only true/false. As you said OnOffSplit and OnOffAuto are on/off. And from documentation POV, it's all over the place.
diff --git a/hw/core/machine.c b/hw/core/machine.c index 84812a1d1c..5ff698ac3c 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -36,6 +36,7 @@ GlobalProperty hw_compat_4_2[] = { { "usb-redir", "suppress-remote-wake", "off" }, { "qxl", "revision", "4" }, { "qxl-vga", "revision", "4" }, + { "pcie-root-port-base", "disable-hotplug", "false" }, }; const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2); diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c index 0ba4e4dea4..d6a080bee8 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("disable-hotplug", PCIESlot, disable_hotplug, false), DEFINE_PROP_END_OF_LIST() }; diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index 153a4acad2..04aae72cd6 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); diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 08718188bb..6dd9b89e21 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,17 @@ 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->disable_hotplug) { + pci_long_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCAP, + 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..796fc8f3ab 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 disable_hotplug; QLIST_ENTRY(PCIESlot) next; };
Make hot-plug/hot-unplug on PCIe Root Ports optional to allow libvirt to manage it and restrict unplug for the entire machine. This is going to prevent user-initiated unplug in guests (Windows mostly). Usage: -device pcie-root-port,disable-hotplug=true,... Discussion related: https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00530.html Signed-off-by: Julia Suvorova <jusual@redhat.com> --- hw/core/machine.c | 1 + hw/pci-bridge/pcie_root_port.c | 3 ++- hw/pci-bridge/xio3130_downstream.c | 2 +- hw/pci/pcie.c | 8 ++++++-- include/hw/pci/pcie.h | 2 +- include/hw/pci/pcie_port.h | 1 + 6 files changed, 12 insertions(+), 5 deletions(-)