* [Qemu-devel] [PATCH 0/3] pcie: hotplug fixes @ 2019-06-21 6:46 Michael S. Tsirkin 2019-06-21 6:46 ` [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events Michael S. Tsirkin ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2019-06-21 6:46 UTC (permalink / raw) To: qemu-devel Red Hat QE reported several bugs around PCI Express hotplug. Fixes/workarounds follow. Michael S. Tsirkin (3): pcie: don't skip multi-mask events pcie: check that slt ctrl changed before deleting pcie: work around for racy guest init include/hw/pci/pcie.h | 3 ++- hw/pci-bridge/pcie_root_port.c | 5 ++++- hw/pci-bridge/xio3130_downstream.c | 5 ++++- hw/pci/pcie.c | 35 +++++++++++++++++++++++++++--- 4 files changed, 42 insertions(+), 6 deletions(-) -- MST ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events 2019-06-21 6:46 [Qemu-devel] [PATCH 0/3] pcie: hotplug fixes Michael S. Tsirkin @ 2019-06-21 6:46 ` Michael S. Tsirkin 2019-06-25 11:14 ` Igor Mammedov ` (2 more replies) 2019-06-21 6:46 ` [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting Michael S. Tsirkin ` (2 subsequent siblings) 3 siblings, 3 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2019-06-21 6:46 UTC (permalink / raw) To: qemu-devel If we are trying to set multiple bits at once, testing that just one of them is already set gives a false positive. As a result we won't interrupt guest if e.g. presence detection change and attention button press are both set. This happens with multi-function device removal. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/pci/pcie.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 88c30ff74c..b22527000d 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -383,7 +383,7 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event) { /* Minor optimization: if nothing changed - no event is needed. */ if (pci_word_test_and_set_mask(dev->config + dev->exp.exp_cap + - PCI_EXP_SLTSTA, event)) { + PCI_EXP_SLTSTA, event) == event) { return; } hotplug_event_notify(dev); -- MST ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events 2019-06-21 6:46 ` [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events Michael S. Tsirkin @ 2019-06-25 11:14 ` Igor Mammedov 2019-07-01 7:01 ` Marcel Apfelbaum 2019-07-01 9:56 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 23+ messages in thread From: Igor Mammedov @ 2019-06-25 11:14 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Fri, 21 Jun 2019 02:46:46 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > If we are trying to set multiple bits at once, testing that just one of > them is already set gives a false positive. As a result we won't > interrupt guest if e.g. presence detection change and attention button > press are both set. This happens with multi-function device removal. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/pci/pcie.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 88c30ff74c..b22527000d 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -383,7 +383,7 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event) > { > /* Minor optimization: if nothing changed - no event is needed. */ > if (pci_word_test_and_set_mask(dev->config + dev->exp.exp_cap + > - PCI_EXP_SLTSTA, event)) { > + PCI_EXP_SLTSTA, event) == event) { > return; > } > hotplug_event_notify(dev); ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events 2019-06-21 6:46 ` [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events Michael S. Tsirkin 2019-06-25 11:14 ` Igor Mammedov @ 2019-07-01 7:01 ` Marcel Apfelbaum 2019-07-01 9:56 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 23+ messages in thread From: Marcel Apfelbaum @ 2019-07-01 7:01 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel On 6/21/19 9:46 AM, Michael S. Tsirkin wrote: > If we are trying to set multiple bits at once, testing that just one of > them is already set gives a false positive. As a result we won't > interrupt guest if e.g. presence detection change and attention button > press are both set. This happens with multi-function device removal. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci/pcie.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 88c30ff74c..b22527000d 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -383,7 +383,7 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event) > { > /* Minor optimization: if nothing changed - no event is needed. */ > if (pci_word_test_and_set_mask(dev->config + dev->exp.exp_cap + > - PCI_EXP_SLTSTA, event)) { > + PCI_EXP_SLTSTA, event) == event) { > return; > } > hotplug_event_notify(dev); Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Thanks, Marcel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events 2019-06-21 6:46 ` [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events Michael S. Tsirkin 2019-06-25 11:14 ` Igor Mammedov 2019-07-01 7:01 ` Marcel Apfelbaum @ 2019-07-01 9:56 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 23+ messages in thread From: Philippe Mathieu-Daudé @ 2019-07-01 9:56 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel On 6/21/19 8:46 AM, Michael S. Tsirkin wrote: > If we are trying to set multiple bits at once, testing that just one of > them is already set gives a false positive. As a result we won't > interrupt guest if e.g. presence detection change and attention button > press are both set. This happens with multi-function device removal. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci/pcie.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 88c30ff74c..b22527000d 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -383,7 +383,7 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event) > { > /* Minor optimization: if nothing changed - no event is needed. */ > if (pci_word_test_and_set_mask(dev->config + dev->exp.exp_cap + > - PCI_EXP_SLTSTA, event)) { > + PCI_EXP_SLTSTA, event) == event) { > return; > } > hotplug_event_notify(dev); > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting 2019-06-21 6:46 [Qemu-devel] [PATCH 0/3] pcie: hotplug fixes Michael S. Tsirkin 2019-06-21 6:46 ` [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events Michael S. Tsirkin @ 2019-06-21 6:46 ` Michael S. Tsirkin 2019-06-25 12:45 ` Igor Mammedov ` (2 more replies) 2019-06-21 6:46 ` [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init Michael S. Tsirkin 2019-07-01 9:34 ` [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status Michael S. Tsirkin 3 siblings, 3 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2019-06-21 6:46 UTC (permalink / raw) To: qemu-devel During boot, linux would sometimes overwrites control of a powered off slot before powering it on. Unfortunately QEMU interprets that as a power off request and ejects the device. For example: /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ -monitor stdio disk.qcow2 (qemu)device_add virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 (qemu)cont Balloon is deleted during guest boot. To fix, save control beforehand and check that power or led state actually change before ejecting. Note: this is more a hack than a solution, ideally we'd find a better way to detect ejects, or move away from ejects completely and instead monitor whether it's safe to delete device due to e.g. its power state. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/hw/pci/pcie.h | 3 ++- hw/pci-bridge/pcie_root_port.c | 5 ++++- hw/pci-bridge/xio3130_downstream.c | 5 ++++- hw/pci/pcie.c | 14 ++++++++++++-- 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h index e30334d74d..8d90c0e193 100644 --- a/include/hw/pci/pcie.h +++ b/include/hw/pci/pcie.h @@ -107,7 +107,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); void pcie_cap_slot_reset(PCIDevice *dev); -void pcie_cap_slot_write_config(PCIDevice *dev, +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, uint32_t addr, uint32_t val, int len); int pcie_cap_slot_post_load(void *opaque, int version_id); void pcie_cap_slot_push_attention_button(PCIDevice *dev); diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c index 92f253c924..09019ca05d 100644 --- a/hw/pci-bridge/pcie_root_port.c +++ b/hw/pci-bridge/pcie_root_port.c @@ -31,10 +31,13 @@ static void rp_write_config(PCIDevice *d, uint32_t address, { uint32_t root_cmd = pci_get_long(d->config + d->exp.aer_cap + PCI_ERR_ROOT_COMMAND); + uint16_t slt_ctl, slt_sta; + + pcie_cap_slot_get(d, &slt_ctl, &slt_sta); pci_bridge_write_config(d, address, val, len); rp_aer_vector_update(d); - pcie_cap_slot_write_config(d, address, val, len); + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); pcie_aer_write_config(d, address, val, len); pcie_aer_root_write_config(d, address, val, len, root_cmd); } diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index 264e37d6a6..899b0fd6c9 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -41,9 +41,12 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) { + uint16_t slt_ctl, slt_sta; + + pcie_cap_slot_get(d, &slt_sta, &slt_ctl); pci_bridge_write_config(d, address, val, len); pcie_cap_flr_write_config(d, address, val, len); - pcie_cap_slot_write_config(d, address, val, len); + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); pcie_aer_write_config(d, address, val, len); } diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index b22527000d..f8490a00de 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -594,7 +594,15 @@ void pcie_cap_slot_reset(PCIDevice *dev) hotplug_event_update_event_status(dev); } -void pcie_cap_slot_write_config(PCIDevice *dev, +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) +{ + uint32_t pos = dev->exp.exp_cap; + uint8_t *exp_cap = dev->config + pos; + *slt_ctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL); + *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); +} + +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, uint32_t addr, uint32_t val, int len) { uint32_t pos = dev->exp.exp_cap; @@ -623,7 +631,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev, * controller is off, it is safe to detach the devices. */ if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && - ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { + (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && + (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || + (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); pci_for_each_device(sec_bus, pci_bus_num(sec_bus), pcie_unplug_device, NULL); -- MST ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting 2019-06-21 6:46 ` [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting Michael S. Tsirkin @ 2019-06-25 12:45 ` Igor Mammedov 2019-07-01 9:23 ` Michael S. Tsirkin 2019-07-01 7:03 ` Marcel Apfelbaum 2019-07-01 13:07 ` Igor Mammedov 2 siblings, 1 reply; 23+ messages in thread From: Igor Mammedov @ 2019-06-25 12:45 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Fri, 21 Jun 2019 02:46:48 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > During boot, linux would sometimes overwrites control of a powered off > slot before powering it on. Unfortunately QEMU interprets that as a > power off request and ejects the device. > > For example: > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > -monitor stdio disk.qcow2 > (qemu)device_add virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 > (qemu)cont > > Balloon is deleted during guest boot. > > To fix, save control beforehand and check that power > or led state actually change before ejecting. > > Note: this is more a hack than a solution, ideally we'd > find a better way to detect ejects, or move away > from ejects completely and instead monitor whether > it's safe to delete device due to e.g. its power state. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/hw/pci/pcie.h | 3 ++- > hw/pci-bridge/pcie_root_port.c | 5 ++++- > hw/pci-bridge/xio3130_downstream.c | 5 ++++- > hw/pci/pcie.c | 14 ++++++++++++-- > 4 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index e30334d74d..8d90c0e193 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -107,7 +107,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); > > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); > void pcie_cap_slot_reset(PCIDevice *dev); > -void pcie_cap_slot_write_config(PCIDevice *dev, > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, > uint32_t addr, uint32_t val, int len); > int pcie_cap_slot_post_load(void *opaque, int version_id); > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > index 92f253c924..09019ca05d 100644 > --- a/hw/pci-bridge/pcie_root_port.c > +++ b/hw/pci-bridge/pcie_root_port.c > @@ -31,10 +31,13 @@ static void rp_write_config(PCIDevice *d, uint32_t address, > { > uint32_t root_cmd = > pci_get_long(d->config + d->exp.aer_cap + PCI_ERR_ROOT_COMMAND); > + uint16_t slt_ctl, slt_sta; > + > + pcie_cap_slot_get(d, &slt_ctl, &slt_sta); > > pci_bridge_write_config(d, address, val, len); > rp_aer_vector_update(d); > - pcie_cap_slot_write_config(d, address, val, len); > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > pcie_aer_write_config(d, address, val, len); > pcie_aer_root_write_config(d, address, val, len, root_cmd); > } > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index 264e37d6a6..899b0fd6c9 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -41,9 +41,12 @@ > static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address, > uint32_t val, int len) > { > + uint16_t slt_ctl, slt_sta; > + > + pcie_cap_slot_get(d, &slt_sta, &slt_ctl); > pci_bridge_write_config(d, address, val, len); > pcie_cap_flr_write_config(d, address, val, len); > - pcie_cap_slot_write_config(d, address, val, len); > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > pcie_aer_write_config(d, address, val, len); > } > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index b22527000d..f8490a00de 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -594,7 +594,15 @@ void pcie_cap_slot_reset(PCIDevice *dev) > hotplug_event_update_event_status(dev); > } > > -void pcie_cap_slot_write_config(PCIDevice *dev, > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) > +{ > + uint32_t pos = dev->exp.exp_cap; > + uint8_t *exp_cap = dev->config + pos; > + *slt_ctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL); > + *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > +} > + > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, > uint32_t addr, uint32_t val, int len) > { > uint32_t pos = dev->exp.exp_cap; > @@ -623,7 +631,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > * controller is off, it is safe to detach the devices. > */ comment above probably should be updated to account for new conditions > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > - ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > + (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > + (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || > + (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > pcie_unplug_device, NULL); ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting 2019-06-25 12:45 ` Igor Mammedov @ 2019-07-01 9:23 ` Michael S. Tsirkin 0 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2019-07-01 9:23 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel On Tue, Jun 25, 2019 at 02:45:11PM +0200, Igor Mammedov wrote: > On Fri, 21 Jun 2019 02:46:48 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > During boot, linux would sometimes overwrites control of a powered off > > slot before powering it on. Unfortunately QEMU interprets that as a > > power off request and ejects the device. > > > > For example: > > > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > > -monitor stdio disk.qcow2 > > (qemu)device_add virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 > > (qemu)cont > > > > Balloon is deleted during guest boot. > > > > To fix, save control beforehand and check that power > > or led state actually change before ejecting. > > > > Note: this is more a hack than a solution, ideally we'd > > find a better way to detect ejects, or move away > > from ejects completely and instead monitor whether > > it's safe to delete device due to e.g. its power state. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > include/hw/pci/pcie.h | 3 ++- > > hw/pci-bridge/pcie_root_port.c | 5 ++++- > > hw/pci-bridge/xio3130_downstream.c | 5 ++++- > > hw/pci/pcie.c | 14 ++++++++++++-- > > 4 files changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > > index e30334d74d..8d90c0e193 100644 > > --- a/include/hw/pci/pcie.h > > +++ b/include/hw/pci/pcie.h > > @@ -107,7 +107,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); > > > > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); > > void pcie_cap_slot_reset(PCIDevice *dev); > > -void pcie_cap_slot_write_config(PCIDevice *dev, > > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); > > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, > > uint32_t addr, uint32_t val, int len); > > int pcie_cap_slot_post_load(void *opaque, int version_id); > > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > > index 92f253c924..09019ca05d 100644 > > --- a/hw/pci-bridge/pcie_root_port.c > > +++ b/hw/pci-bridge/pcie_root_port.c > > @@ -31,10 +31,13 @@ static void rp_write_config(PCIDevice *d, uint32_t address, > > { > > uint32_t root_cmd = > > pci_get_long(d->config + d->exp.aer_cap + PCI_ERR_ROOT_COMMAND); > > + uint16_t slt_ctl, slt_sta; > > + > > + pcie_cap_slot_get(d, &slt_ctl, &slt_sta); > > > > pci_bridge_write_config(d, address, val, len); > > rp_aer_vector_update(d); > > - pcie_cap_slot_write_config(d, address, val, len); > > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > > pcie_aer_write_config(d, address, val, len); > > pcie_aer_root_write_config(d, address, val, len, root_cmd); > > } > > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > > index 264e37d6a6..899b0fd6c9 100644 > > --- a/hw/pci-bridge/xio3130_downstream.c > > +++ b/hw/pci-bridge/xio3130_downstream.c > > @@ -41,9 +41,12 @@ > > static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address, > > uint32_t val, int len) > > { > > + uint16_t slt_ctl, slt_sta; > > + > > + pcie_cap_slot_get(d, &slt_sta, &slt_ctl); > > pci_bridge_write_config(d, address, val, len); > > pcie_cap_flr_write_config(d, address, val, len); > > - pcie_cap_slot_write_config(d, address, val, len); > > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > > pcie_aer_write_config(d, address, val, len); > > } > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index b22527000d..f8490a00de 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -594,7 +594,15 @@ void pcie_cap_slot_reset(PCIDevice *dev) > > hotplug_event_update_event_status(dev); > > } > > > > -void pcie_cap_slot_write_config(PCIDevice *dev, > > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) > > +{ > > + uint32_t pos = dev->exp.exp_cap; > > + uint8_t *exp_cap = dev->config + pos; > > + *slt_ctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL); > > + *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > +} > > + > > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, > > uint32_t addr, uint32_t val, int len) > > { > > uint32_t pos = dev->exp.exp_cap; > > @@ -623,7 +631,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > * controller is off, it is safe to detach the devices. > > */ > comment above probably should be updated to account for new conditions It's actually the same condition: the change is that we do not eject if it was already true. > > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > - ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > > + (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > > + (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || > > + (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > > pcie_unplug_device, NULL); ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting 2019-06-21 6:46 ` [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting Michael S. Tsirkin 2019-06-25 12:45 ` Igor Mammedov @ 2019-07-01 7:03 ` Marcel Apfelbaum 2019-07-01 13:07 ` Igor Mammedov 2 siblings, 0 replies; 23+ messages in thread From: Marcel Apfelbaum @ 2019-07-01 7:03 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel On 6/21/19 9:46 AM, Michael S. Tsirkin wrote: > During boot, linux would sometimes overwrites control of a powered off > slot before powering it on. Unfortunately QEMU interprets that as a > power off request and ejects the device. > > For example: > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > -monitor stdio disk.qcow2 > (qemu)device_add virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 > (qemu)cont > > Balloon is deleted during guest boot. > > To fix, save control beforehand and check that power > or led state actually change before ejecting. > > Note: this is more a hack than a solution, ideally we'd > find a better way to detect ejects, or move away > from ejects completely and instead monitor whether > it's safe to delete device due to e.g. its power state. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/hw/pci/pcie.h | 3 ++- > hw/pci-bridge/pcie_root_port.c | 5 ++++- > hw/pci-bridge/xio3130_downstream.c | 5 ++++- > hw/pci/pcie.c | 14 ++++++++++++-- > 4 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index e30334d74d..8d90c0e193 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -107,7 +107,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); > > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); > void pcie_cap_slot_reset(PCIDevice *dev); > -void pcie_cap_slot_write_config(PCIDevice *dev, > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, > uint32_t addr, uint32_t val, int len); > int pcie_cap_slot_post_load(void *opaque, int version_id); > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > index 92f253c924..09019ca05d 100644 > --- a/hw/pci-bridge/pcie_root_port.c > +++ b/hw/pci-bridge/pcie_root_port.c > @@ -31,10 +31,13 @@ static void rp_write_config(PCIDevice *d, uint32_t address, > { > uint32_t root_cmd = > pci_get_long(d->config + d->exp.aer_cap + PCI_ERR_ROOT_COMMAND); > + uint16_t slt_ctl, slt_sta; > + > + pcie_cap_slot_get(d, &slt_ctl, &slt_sta); > > pci_bridge_write_config(d, address, val, len); > rp_aer_vector_update(d); > - pcie_cap_slot_write_config(d, address, val, len); > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > pcie_aer_write_config(d, address, val, len); > pcie_aer_root_write_config(d, address, val, len, root_cmd); > } > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index 264e37d6a6..899b0fd6c9 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -41,9 +41,12 @@ > static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address, > uint32_t val, int len) > { > + uint16_t slt_ctl, slt_sta; > + > + pcie_cap_slot_get(d, &slt_sta, &slt_ctl); > pci_bridge_write_config(d, address, val, len); > pcie_cap_flr_write_config(d, address, val, len); > - pcie_cap_slot_write_config(d, address, val, len); > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > pcie_aer_write_config(d, address, val, len); > } > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index b22527000d..f8490a00de 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -594,7 +594,15 @@ void pcie_cap_slot_reset(PCIDevice *dev) > hotplug_event_update_event_status(dev); > } > > -void pcie_cap_slot_write_config(PCIDevice *dev, > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) > +{ > + uint32_t pos = dev->exp.exp_cap; > + uint8_t *exp_cap = dev->config + pos; > + *slt_ctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL); > + *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > +} > + > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, > uint32_t addr, uint32_t val, int len) > { > uint32_t pos = dev->exp.exp_cap; > @@ -623,7 +631,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > * controller is off, it is safe to detach the devices. > */ > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > - ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > + (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > + (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || > + (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > pcie_unplug_device, NULL); Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Thanks, Marcel ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting 2019-06-21 6:46 ` [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting Michael S. Tsirkin 2019-06-25 12:45 ` Igor Mammedov 2019-07-01 7:03 ` Marcel Apfelbaum @ 2019-07-01 13:07 ` Igor Mammedov 2 siblings, 0 replies; 23+ messages in thread From: Igor Mammedov @ 2019-07-01 13:07 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Fri, 21 Jun 2019 02:46:48 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > During boot, linux would sometimes overwrites control of a powered off > slot before powering it on. Unfortunately QEMU interprets that as a > power off request and ejects the device. > > For example: > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > -monitor stdio disk.qcow2 > (qemu)device_add virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 > (qemu)cont > > Balloon is deleted during guest boot. > > To fix, save control beforehand and check that power > or led state actually change before ejecting. > > Note: this is more a hack than a solution, ideally we'd > find a better way to detect ejects, or move away > from ejects completely and instead monitor whether > it's safe to delete device due to e.g. its power state. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> Tested-by: Igor Mammedov <imammedo@redhat.com> > --- > include/hw/pci/pcie.h | 3 ++- > hw/pci-bridge/pcie_root_port.c | 5 ++++- > hw/pci-bridge/xio3130_downstream.c | 5 ++++- > hw/pci/pcie.c | 14 ++++++++++++-- > 4 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index e30334d74d..8d90c0e193 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -107,7 +107,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); > > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); > void pcie_cap_slot_reset(PCIDevice *dev); > -void pcie_cap_slot_write_config(PCIDevice *dev, > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, > uint32_t addr, uint32_t val, int len); > int pcie_cap_slot_post_load(void *opaque, int version_id); > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > index 92f253c924..09019ca05d 100644 > --- a/hw/pci-bridge/pcie_root_port.c > +++ b/hw/pci-bridge/pcie_root_port.c > @@ -31,10 +31,13 @@ static void rp_write_config(PCIDevice *d, uint32_t address, > { > uint32_t root_cmd = > pci_get_long(d->config + d->exp.aer_cap + PCI_ERR_ROOT_COMMAND); > + uint16_t slt_ctl, slt_sta; > + > + pcie_cap_slot_get(d, &slt_ctl, &slt_sta); > > pci_bridge_write_config(d, address, val, len); > rp_aer_vector_update(d); > - pcie_cap_slot_write_config(d, address, val, len); > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > pcie_aer_write_config(d, address, val, len); > pcie_aer_root_write_config(d, address, val, len, root_cmd); > } > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c > index 264e37d6a6..899b0fd6c9 100644 > --- a/hw/pci-bridge/xio3130_downstream.c > +++ b/hw/pci-bridge/xio3130_downstream.c > @@ -41,9 +41,12 @@ > static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address, > uint32_t val, int len) > { > + uint16_t slt_ctl, slt_sta; > + > + pcie_cap_slot_get(d, &slt_sta, &slt_ctl); > pci_bridge_write_config(d, address, val, len); > pcie_cap_flr_write_config(d, address, val, len); > - pcie_cap_slot_write_config(d, address, val, len); > + pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len); > pcie_aer_write_config(d, address, val, len); > } > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index b22527000d..f8490a00de 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -594,7 +594,15 @@ void pcie_cap_slot_reset(PCIDevice *dev) > hotplug_event_update_event_status(dev); > } > > -void pcie_cap_slot_write_config(PCIDevice *dev, > +void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) > +{ > + uint32_t pos = dev->exp.exp_cap; > + uint8_t *exp_cap = dev->config + pos; > + *slt_ctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL); > + *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > +} > + > +void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, > uint32_t addr, uint32_t val, int len) > { > uint32_t pos = dev->exp.exp_cap; > @@ -623,7 +631,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > * controller is off, it is safe to detach the devices. > */ > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > - ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > + (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > + (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || > + (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > pcie_unplug_device, NULL); ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init 2019-06-21 6:46 [Qemu-devel] [PATCH 0/3] pcie: hotplug fixes Michael S. Tsirkin 2019-06-21 6:46 ` [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events Michael S. Tsirkin 2019-06-21 6:46 ` [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting Michael S. Tsirkin @ 2019-06-21 6:46 ` Michael S. Tsirkin 2019-06-25 13:07 ` Igor Mammedov ` (2 more replies) 2019-07-01 9:34 ` [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status Michael S. Tsirkin 3 siblings, 3 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2019-06-21 6:46 UTC (permalink / raw) To: qemu-devel During boot, linux guests tend to clear all bits in pcie slot status register which is used for hotplug. If they clear bits that weren't set this is racy and will lose events: not a big problem for manual hotplug on bare-metal, but a problem for us. For example, the following is broken ATM: /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ -monitor stdio disk.qcow2 (qemu)device_del balloon (qemu)cont Balloon isn't deleted as it should. As a work-around, detect this attempt to clear slot status and revert status to what it was before the write. Note: in theory this can be detected as a duplicate button press which cancels the previous press. Does not seem to happen in practice as guests seem to only have this bug during init. Note2: the right thing to do is probably to fix Linux to read status before clearing it, and act on the bits that are set. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/pci/pcie.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index f8490a00de..c605d32dd4 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { + /* + * Guests tend to clears all bits during init. + * If they clear bits that weren't set this is racy and will lose events: + * not a big problem for manual button presses, but a problem for us. + * As a work-around, detect this and revert status to what it was + * before the write. + * + * Note: in theory this can be detected as a duplicate button press + * which cancels the previous press. Does not seem to happen in + * practice as guests seem to only have this bug during init. + */ +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ + PCI_EXP_SLTSTA_CC) + + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); + } hotplug_event_clear(dev); } -- MST ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init 2019-06-21 6:46 ` [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init Michael S. Tsirkin @ 2019-06-25 13:07 ` Igor Mammedov 2019-07-01 9:20 ` Michael S. Tsirkin 2019-07-01 7:04 ` Marcel Apfelbaum 2019-07-01 13:01 ` Igor Mammedov 2 siblings, 1 reply; 23+ messages in thread From: Igor Mammedov @ 2019-06-25 13:07 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Fri, 21 Jun 2019 02:46:50 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > During boot, linux guests tend to clear all bits in pcie slot status > register which is used for hotplug. > If they clear bits that weren't set this is racy and will lose events: > not a big problem for manual hotplug on bare-metal, but a problem for us. > > For example, the following is broken ATM: > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ > -monitor stdio disk.qcow2 > (qemu)device_del balloon > (qemu)cont > > Balloon isn't deleted as it should. > > As a work-around, detect this attempt to clear slot status and revert > status to what it was before the write. > > Note: in theory this can be detected as a duplicate button press > which cancels the previous press. Does not seem to happen in > practice as guests seem to only have this bug during init. > > Note2: the right thing to do is probably to fix Linux to > read status before clearing it, and act on the bits that are set. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci/pcie.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index f8490a00de..c605d32dd4 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { > + /* > + * Guests tend to clears all bits during init. > + * If they clear bits that weren't set this is racy and will lose events: > + * not a big problem for manual button presses, but a problem for us. > + * As a work-around, detect this and revert status to what it was > + * before the write. > + * > + * Note: in theory this can be detected as a duplicate button press > + * which cancels the previous press. Does not seem to happen in > + * practice as guests seem to only have this bug during init. > + */ > +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ > + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > + PCI_EXP_SLTSTA_CC) > + > + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); I'm reading it as: sltsta = LOWER_PART(sltsta) | UPPER_PART(sltsta) which basically sltsta = sltsta or am I missing something here? > + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > + } > hotplug_event_clear(dev); > } > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init 2019-06-25 13:07 ` Igor Mammedov @ 2019-07-01 9:20 ` Michael S. Tsirkin 2019-07-01 12:04 ` Igor Mammedov 0 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2019-07-01 9:20 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel On Tue, Jun 25, 2019 at 03:07:30PM +0200, Igor Mammedov wrote: > On Fri, 21 Jun 2019 02:46:50 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > During boot, linux guests tend to clear all bits in pcie slot status > > register which is used for hotplug. > > If they clear bits that weren't set this is racy and will lose events: > > not a big problem for manual hotplug on bare-metal, but a problem for us. > > > > For example, the following is broken ATM: > > > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > > -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ > > -monitor stdio disk.qcow2 > > (qemu)device_del balloon > > (qemu)cont > > > > Balloon isn't deleted as it should. > > > > As a work-around, detect this attempt to clear slot status and revert > > status to what it was before the write. > > > > Note: in theory this can be detected as a duplicate button press > > which cancels the previous press. Does not seem to happen in > > practice as guests seem to only have this bug during init. > > > > Note2: the right thing to do is probably to fix Linux to > > read status before clearing it, and act on the bits that are set. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > hw/pci/pcie.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index f8490a00de..c605d32dd4 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > > > if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { > > + /* > > + * Guests tend to clears all bits during init. > > + * If they clear bits that weren't set this is racy and will lose events: > > + * not a big problem for manual button presses, but a problem for us. > > + * As a work-around, detect this and revert status to what it was > > + * before the write. > > + * > > + * Note: in theory this can be detected as a duplicate button press > > + * which cancels the previous press. Does not seem to happen in > > + * practice as guests seem to only have this bug during init. > > + */ > > +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ > > + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > > + PCI_EXP_SLTSTA_CC) > > + > > + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > I'm reading it as: > sltsta = LOWER_PART(sltsta) | UPPER_PART(sltsta) > which basically > sltsta = sltsta > or am I missing something here? You are missing the underscore. slt_sta is the old value. sltsta is the new value. > > + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > > + } > > hotplug_event_clear(dev); > > } > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init 2019-07-01 9:20 ` Michael S. Tsirkin @ 2019-07-01 12:04 ` Igor Mammedov 2019-07-01 12:08 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Igor Mammedov @ 2019-07-01 12:04 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Mon, 1 Jul 2019 05:20:41 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Jun 25, 2019 at 03:07:30PM +0200, Igor Mammedov wrote: > > On Fri, 21 Jun 2019 02:46:50 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > During boot, linux guests tend to clear all bits in pcie slot status > > > register which is used for hotplug. > > > If they clear bits that weren't set this is racy and will lose events: > > > not a big problem for manual hotplug on bare-metal, but a problem for us. > > > > > > For example, the following is broken ATM: > > > > > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > > > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > > > -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ > > > -monitor stdio disk.qcow2 > > > (qemu)device_del balloon > > > (qemu)cont > > > > > > Balloon isn't deleted as it should. > > > > > > As a work-around, detect this attempt to clear slot status and revert > > > status to what it was before the write. > > > > > > Note: in theory this can be detected as a duplicate button press > > > which cancels the previous press. Does not seem to happen in > > > practice as guests seem to only have this bug during init. > > > > > > Note2: the right thing to do is probably to fix Linux to > > > read status before clearing it, and act on the bits that are set. > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > hw/pci/pcie.c | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > index f8490a00de..c605d32dd4 100644 > > > --- a/hw/pci/pcie.c > > > +++ b/hw/pci/pcie.c > > > @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > > > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > > > > > if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { > > > + /* > > > + * Guests tend to clears all bits during init. > > > + * If they clear bits that weren't set this is racy and will lose events: > > > + * not a big problem for manual button presses, but a problem for us. > > > + * As a work-around, detect this and revert status to what it was > > > + * before the write. > > > + * > > > + * Note: in theory this can be detected as a duplicate button press > > > + * which cancels the previous press. Does not seem to happen in > > > + * practice as guests seem to only have this bug during init. > > > + */ > > > +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ > > > + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > > > + PCI_EXP_SLTSTA_CC) > > > + > > > + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > > > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > > I'm reading it as: > > sltsta = LOWER_PART(sltsta) | UPPER_PART(sltsta) > > which basically > > sltsta = sltsta > > or am I missing something here? > > You are missing the underscore. > > slt_sta is the old value. > sltsta is the new value. I did notice it but still don't see where exp_cap + PCI_EXP_SLTSTA is being modified in call chain between pcie_cap_slot_get(d, &slt_ctl, &slt_sta) ... pcie_cap_slot_write_config(): sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA) > > > > + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > > > + } > > > hotplug_event_clear(dev); > > > } > > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init 2019-07-01 12:04 ` Igor Mammedov @ 2019-07-01 12:08 ` Michael S. Tsirkin 0 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2019-07-01 12:08 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel On Mon, Jul 01, 2019 at 02:04:02PM +0200, Igor Mammedov wrote: > On Mon, 1 Jul 2019 05:20:41 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Jun 25, 2019 at 03:07:30PM +0200, Igor Mammedov wrote: > > > On Fri, 21 Jun 2019 02:46:50 -0400 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > During boot, linux guests tend to clear all bits in pcie slot status > > > > register which is used for hotplug. > > > > If they clear bits that weren't set this is racy and will lose events: > > > > not a big problem for manual hotplug on bare-metal, but a problem for us. > > > > > > > > For example, the following is broken ATM: > > > > > > > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > > > > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > > > > -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ > > > > -monitor stdio disk.qcow2 > > > > (qemu)device_del balloon > > > > (qemu)cont > > > > > > > > Balloon isn't deleted as it should. > > > > > > > > As a work-around, detect this attempt to clear slot status and revert > > > > status to what it was before the write. > > > > > > > > Note: in theory this can be detected as a duplicate button press > > > > which cancels the previous press. Does not seem to happen in > > > > practice as guests seem to only have this bug during init. > > > > > > > > Note2: the right thing to do is probably to fix Linux to > > > > read status before clearing it, and act on the bits that are set. > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > hw/pci/pcie.c | 19 +++++++++++++++++++ > > > > 1 file changed, 19 insertions(+) > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > index f8490a00de..c605d32dd4 100644 > > > > --- a/hw/pci/pcie.c > > > > +++ b/hw/pci/pcie.c > > > > @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > > > > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > > > > > > > if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { > > > > + /* > > > > + * Guests tend to clears all bits during init. > > > > + * If they clear bits that weren't set this is racy and will lose events: > > > > + * not a big problem for manual button presses, but a problem for us. > > > > + * As a work-around, detect this and revert status to what it was > > > > + * before the write. > > > > + * > > > > + * Note: in theory this can be detected as a duplicate button press > > > > + * which cancels the previous press. Does not seem to happen in > > > > + * practice as guests seem to only have this bug during init. > > > > + */ > > > > +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ > > > > + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > > > > + PCI_EXP_SLTSTA_CC) > > > > + > > > > + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > > > > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > > > I'm reading it as: > > > sltsta = LOWER_PART(sltsta) | UPPER_PART(sltsta) > > > which basically > > > sltsta = sltsta > > > or am I missing something here? > > > > You are missing the underscore. > > > > slt_sta is the old value. > > sltsta is the new value. > > I did notice it but still don't see where > exp_cap + PCI_EXP_SLTSTA > is being modified in call chain between > pcie_cap_slot_get(d, &slt_ctl, &slt_sta) > ... > pcie_cap_slot_write_config(): > sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA) It's modified by pci_bridge_write_config which calls pci_default_write_config. > > > > > > + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > > > > + } > > > > hotplug_event_clear(dev); > > > > } > > > > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init 2019-06-21 6:46 ` [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init Michael S. Tsirkin 2019-06-25 13:07 ` Igor Mammedov @ 2019-07-01 7:04 ` Marcel Apfelbaum [not found] ` <20190701105708.5d28f497@redhat.com> 2019-07-01 13:01 ` Igor Mammedov 2 siblings, 1 reply; 23+ messages in thread From: Marcel Apfelbaum @ 2019-07-01 7:04 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel On 6/21/19 9:46 AM, Michael S. Tsirkin wrote: > During boot, linux guests tend to clear all bits in pcie slot status > register which is used for hotplug. > If they clear bits that weren't set this is racy and will lose events: > not a big problem for manual hotplug on bare-metal, but a problem for us. > > For example, the following is broken ATM: > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ > -monitor stdio disk.qcow2 > (qemu)device_del balloon > (qemu)cont > > Balloon isn't deleted as it should. > > As a work-around, detect this attempt to clear slot status and revert > status to what it was before the write. > > Note: in theory this can be detected as a duplicate button press > which cancels the previous press. Does not seem to happen in > practice as guests seem to only have this bug during init. > > Note2: the right thing to do is probably to fix Linux to > read status before clearing it, and act on the bits that are set. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci/pcie.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index f8490a00de..c605d32dd4 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { > + /* > + * Guests tend to clears all bits during init. > + * If they clear bits that weren't set this is racy and will lose events: > + * not a big problem for manual button presses, but a problem for us. > + * As a work-around, detect this and revert status to what it was > + * before the write. > + * > + * Note: in theory this can be detected as a duplicate button press > + * which cancels the previous press. Does not seem to happen in > + * practice as guests seem to only have this bug during init. > + */ > +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ > + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > + PCI_EXP_SLTSTA_CC) > + > + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > + } > hotplug_event_clear(dev); > } > Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Thanks, Marcel ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <20190701105708.5d28f497@redhat.com>]
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init [not found] ` <20190701105708.5d28f497@redhat.com> @ 2019-07-01 9:12 ` Marcel Apfelbaum 2019-07-01 9:13 ` Marcel Apfelbaum 1 sibling, 0 replies; 23+ messages in thread From: Marcel Apfelbaum @ 2019-07-01 9:12 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel, Michael S. Tsirkin CCing qemu-devel On 7/1/19 11:57 AM, Igor Mammedov wrote: > On Mon, 1 Jul 2019 10:04:01 +0300 > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote: > >> On 6/21/19 9:46 AM, Michael S. Tsirkin wrote: >>> During boot, linux guests tend to clear all bits in pcie slot status >>> register which is used for hotplug. >>> If they clear bits that weren't set this is racy and will lose events: >>> not a big problem for manual hotplug on bare-metal, but a problem for us. >>> >>> For example, the following is broken ATM: >>> >>> /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ >>> -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ >>> -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ >>> -monitor stdio disk.qcow2 >>> (qemu)device_del balloon >>> (qemu)cont >>> >>> Balloon isn't deleted as it should. >>> >>> As a work-around, detect this attempt to clear slot status and revert >>> status to what it was before the write. >>> >>> Note: in theory this can be detected as a duplicate button press >>> which cancels the previous press. Does not seem to happen in >>> practice as guests seem to only have this bug during init. >>> >>> Note2: the right thing to do is probably to fix Linux to >>> read status before clearing it, and act on the bits that are set. >>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> hw/pci/pcie.c | 19 +++++++++++++++++++ >>> 1 file changed, 19 insertions(+) >>> >>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >>> index f8490a00de..c605d32dd4 100644 >>> --- a/hw/pci/pcie.c >>> +++ b/hw/pci/pcie.c >>> @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s >>> uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); >>> >>> if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { >>> + /* >>> + * Guests tend to clears all bits during init. >>> + * If they clear bits that weren't set this is racy and will lose events: >>> + * not a big problem for manual button presses, but a problem for us. >>> + * As a work-around, detect this and revert status to what it was >>> + * before the write. >>> + * >>> + * Note: in theory this can be detected as a duplicate button press >>> + * which cancels the previous press. Does not seem to happen in >>> + * practice as guests seem to only have this bug during init. >>> + */ >>> +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ >>> + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ >>> + PCI_EXP_SLTSTA_CC) >>> + >>> + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { >>> + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); >>> + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); >>> + } >>> hotplug_event_clear(dev); >>> } >>> >> Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Hi Marcel, > > What about my question about > sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > being nop like > sltsta = LOWER_PART(sltsta) | UPPER_PART(sltsta) > > Can you point out what I'm missing here? Oops, I missed that, maybe it should looks like: if (...) { sltsta = (val & ~PCIE_SLOT_EVENTS) | (sltsta & PCIE_SLOT_EVENTS); .... } to keep the PCIE_SLOT_EVENTS that were set before the write. Thanks, Marcel >> Thanks, >> Marcel >> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init [not found] ` <20190701105708.5d28f497@redhat.com> 2019-07-01 9:12 ` Marcel Apfelbaum @ 2019-07-01 9:13 ` Marcel Apfelbaum 1 sibling, 0 replies; 23+ messages in thread From: Marcel Apfelbaum @ 2019-07-01 9:13 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel, Michael S. Tsirkin CCing qemu-devel On 7/1/19 11:57 AM, Igor Mammedov wrote: > On Mon, 1 Jul 2019 10:04:01 +0300 > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote: > >> On 6/21/19 9:46 AM, Michael S. Tsirkin wrote: >>> During boot, linux guests tend to clear all bits in pcie slot status >>> register which is used for hotplug. >>> If they clear bits that weren't set this is racy and will lose events: >>> not a big problem for manual hotplug on bare-metal, but a problem for us. >>> >>> For example, the following is broken ATM: >>> >>> /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ >>> -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ >>> -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ >>> -monitor stdio disk.qcow2 >>> (qemu)device_del balloon >>> (qemu)cont >>> >>> Balloon isn't deleted as it should. >>> >>> As a work-around, detect this attempt to clear slot status and revert >>> status to what it was before the write. >>> >>> Note: in theory this can be detected as a duplicate button press >>> which cancels the previous press. Does not seem to happen in >>> practice as guests seem to only have this bug during init. >>> >>> Note2: the right thing to do is probably to fix Linux to >>> read status before clearing it, and act on the bits that are set. >>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> hw/pci/pcie.c | 19 +++++++++++++++++++ >>> 1 file changed, 19 insertions(+) >>> >>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >>> index f8490a00de..c605d32dd4 100644 >>> --- a/hw/pci/pcie.c >>> +++ b/hw/pci/pcie.c >>> @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s >>> uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); >>> >>> if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { >>> + /* >>> + * Guests tend to clears all bits during init. >>> + * If they clear bits that weren't set this is racy and will lose events: >>> + * not a big problem for manual button presses, but a problem for us. >>> + * As a work-around, detect this and revert status to what it was >>> + * before the write. >>> + * >>> + * Note: in theory this can be detected as a duplicate button press >>> + * which cancels the previous press. Does not seem to happen in >>> + * practice as guests seem to only have this bug during init. >>> + */ >>> +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ >>> + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ >>> + PCI_EXP_SLTSTA_CC) >>> + >>> + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { >>> + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); >>> + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); >>> + } >>> hotplug_event_clear(dev); >>> } >>> >> Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Hi Marcel, > > What about my question about > sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > being nop like > sltsta = LOWER_PART(sltsta) | UPPER_PART(sltsta) > > Can you point out what I'm missing here? Oops, I missed that, maybe it should look like: if (...) { sltsta = (val & ~PCIE_SLOT_EVENTS) | (sltsta & PCIE_SLOT_EVENTS); .... } to keep the PCIE_SLOT_EVENTS that were set before the write. Thanks, Marcel >> Thanks, >> Marcel >> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init 2019-06-21 6:46 ` [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init Michael S. Tsirkin 2019-06-25 13:07 ` Igor Mammedov 2019-07-01 7:04 ` Marcel Apfelbaum @ 2019-07-01 13:01 ` Igor Mammedov 2 siblings, 0 replies; 23+ messages in thread From: Igor Mammedov @ 2019-07-01 13:01 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Fri, 21 Jun 2019 02:46:50 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > During boot, linux guests tend to clear all bits in pcie slot status > register which is used for hotplug. > If they clear bits that weren't set this is racy and will lose events: > not a big problem for manual hotplug on bare-metal, but a problem for us. > > For example, the following is broken ATM: > > /x86_64-softmmu/qemu-system-x86_64 -enable-kvm -S -machine q35 \ > -device pcie-root-port,id=pcie_root_port_0,slot=2,chassis=2,addr=0x2,bus=pcie.0 \ > -device virtio-balloon-pci,id=balloon,bus=pcie_root_port_0 \ > -monitor stdio disk.qcow2 > (qemu)device_del balloon > (qemu)cont > > Balloon isn't deleted as it should. > > As a work-around, detect this attempt to clear slot status and revert > status to what it was before the write. > > Note: in theory this can be detected as a duplicate button press > which cancels the previous press. Does not seem to happen in > practice as guests seem to only have this bug during init. > > Note2: the right thing to do is probably to fix Linux to > read status before clearing it, and act on the bits that are set. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> Tested-by: Igor Mammedov <imammedo@redhat.com> had to change slot addr since #2 seems to be taken by default nic > --- > hw/pci/pcie.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index f8490a00de..c605d32dd4 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -610,6 +610,25 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { > + /* > + * Guests tend to clears all bits during init. > + * If they clear bits that weren't set this is racy and will lose events: > + * not a big problem for manual button presses, but a problem for us. > + * As a work-around, detect this and revert status to what it was > + * before the write. > + * > + * Note: in theory this can be detected as a duplicate button press > + * which cancels the previous press. Does not seem to happen in > + * practice as guests seem to only have this bug during init. > + */ > +#define PCIE_SLOT_EVENTS (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | \ > + PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > + PCI_EXP_SLTSTA_CC) > + > + if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > + pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > + } > hotplug_event_clear(dev); > } > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status 2019-06-21 6:46 [Qemu-devel] [PATCH 0/3] pcie: hotplug fixes Michael S. Tsirkin ` (2 preceding siblings ...) 2019-06-21 6:46 ` [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init Michael S. Tsirkin @ 2019-07-01 9:34 ` Michael S. Tsirkin 2019-07-01 9:56 ` Philippe Mathieu-Daudé ` (2 more replies) 3 siblings, 3 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2019-07-01 9:34 UTC (permalink / raw) To: qemu-devel; +Cc: Igor Mammedov Rename function arguments to make intent clearer. Better documentation for slot control logic. Suggested-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/hw/pci/pcie.h | 3 ++- hw/pci/pcie.c | 17 +++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h index 8d90c0e193..34f277735c 100644 --- a/include/hw/pci/pcie.h +++ b/include/hw/pci/pcie.h @@ -108,7 +108,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); void pcie_cap_slot_reset(PCIDevice *dev); void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, +void pcie_cap_slot_write_config(PCIDevice *dev, + uint16_t old_slot_ctl, uint16_t old_slt_sta, uint32_t addr, uint32_t val, int len); int pcie_cap_slot_post_load(void *opaque, int version_id); void pcie_cap_slot_push_attention_button(PCIDevice *dev); diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index c605d32dd4..a6beb567bd 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -602,7 +602,8 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); } -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, +void pcie_cap_slot_write_config(PCIDevice *dev, + uint16_t old_slt_ctl, uint16_t old_slt_sta, uint32_t addr, uint32_t val, int len) { uint32_t pos = dev->exp.exp_cap; @@ -625,8 +626,8 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ PCI_EXP_SLTSTA_CC) - if (val & ~slt_sta & PCIE_SLOT_EVENTS) { - sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); + if (val & ~old_slt_sta & PCIE_SLOT_EVENTS) { + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (old_slt_sta & PCIE_SLOT_EVENTS); pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); } hotplug_event_clear(dev); @@ -646,13 +647,17 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s } /* - * If the slot is polulated, power indicator is off and power + * If the slot is populated, power indicator is off and power * controller is off, it is safe to detach the devices. + * + * Note: don't detach if condition was already true: + * this is a work around for guests that overwrite + * control of powered off slots before powering them on. */ if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && - (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || - (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { + (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || + (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); pci_for_each_device(sec_bus, pci_bus_num(sec_bus), pcie_unplug_device, NULL); -- MST ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status 2019-07-01 9:34 ` [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status Michael S. Tsirkin @ 2019-07-01 9:56 ` Philippe Mathieu-Daudé 2019-07-01 13:07 ` Igor Mammedov 2019-07-01 13:51 ` Christophe de Dinechin 2 siblings, 0 replies; 23+ messages in thread From: Philippe Mathieu-Daudé @ 2019-07-01 9:56 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel; +Cc: Igor Mammedov On 7/1/19 11:34 AM, Michael S. Tsirkin wrote: > Rename function arguments to make intent clearer. > Better documentation for slot control logic. > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > > include/hw/pci/pcie.h | 3 ++- > hw/pci/pcie.c | 17 +++++++++++------ > 2 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index 8d90c0e193..34f277735c 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -108,7 +108,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); > void pcie_cap_slot_reset(PCIDevice *dev); > void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); > -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, > +void pcie_cap_slot_write_config(PCIDevice *dev, > + uint16_t old_slot_ctl, uint16_t old_slt_sta, > uint32_t addr, uint32_t val, int len); > int pcie_cap_slot_post_load(void *opaque, int version_id); > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index c605d32dd4..a6beb567bd 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -602,7 +602,8 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) > *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > } > > -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, > +void pcie_cap_slot_write_config(PCIDevice *dev, > + uint16_t old_slt_ctl, uint16_t old_slt_sta, > uint32_t addr, uint32_t val, int len) > { > uint32_t pos = dev->exp.exp_cap; > @@ -625,8 +626,8 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > PCI_EXP_SLTSTA_CC) > > - if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > - sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > + if (val & ~old_slt_sta & PCIE_SLOT_EVENTS) { > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (old_slt_sta & PCIE_SLOT_EVENTS); > pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > } > hotplug_event_clear(dev); > @@ -646,13 +647,17 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > } > > /* > - * If the slot is polulated, power indicator is off and power > + * If the slot is populated, power indicator is off and power > * controller is off, it is safe to detach the devices. > + * > + * Note: don't detach if condition was already true: > + * this is a work around for guests that overwrite > + * control of powered off slots before powering them on. > */ > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > - (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || > - (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > + (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || > + (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > pcie_unplug_device, NULL); > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status 2019-07-01 9:34 ` [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status Michael S. Tsirkin 2019-07-01 9:56 ` Philippe Mathieu-Daudé @ 2019-07-01 13:07 ` Igor Mammedov 2019-07-01 13:51 ` Christophe de Dinechin 2 siblings, 0 replies; 23+ messages in thread From: Igor Mammedov @ 2019-07-01 13:07 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Mon, 1 Jul 2019 05:34:54 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Rename function arguments to make intent clearer. > Better documentation for slot control logic. > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > > > include/hw/pci/pcie.h | 3 ++- > hw/pci/pcie.c | 17 +++++++++++------ > 2 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index 8d90c0e193..34f277735c 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -108,7 +108,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); > void pcie_cap_slot_reset(PCIDevice *dev); > void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); > -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, > +void pcie_cap_slot_write_config(PCIDevice *dev, > + uint16_t old_slot_ctl, uint16_t old_slt_sta, > uint32_t addr, uint32_t val, int len); > int pcie_cap_slot_post_load(void *opaque, int version_id); > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index c605d32dd4..a6beb567bd 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -602,7 +602,8 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) > *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > } > > -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, > +void pcie_cap_slot_write_config(PCIDevice *dev, > + uint16_t old_slt_ctl, uint16_t old_slt_sta, > uint32_t addr, uint32_t val, int len) > { > uint32_t pos = dev->exp.exp_cap; > @@ -625,8 +626,8 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > PCI_EXP_SLTSTA_CC) > > - if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > - sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > + if (val & ~old_slt_sta & PCIE_SLOT_EVENTS) { > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (old_slt_sta & PCIE_SLOT_EVENTS); > pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > } > hotplug_event_clear(dev); > @@ -646,13 +647,17 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > } > > /* > - * If the slot is polulated, power indicator is off and power > + * If the slot is populated, power indicator is off and power > * controller is off, it is safe to detach the devices. > + * > + * Note: don't detach if condition was already true: > + * this is a work around for guests that overwrite > + * control of powered off slots before powering them on. > */ > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > - (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || > - (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > + (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || > + (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > pcie_unplug_device, NULL); ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status 2019-07-01 9:34 ` [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status Michael S. Tsirkin 2019-07-01 9:56 ` Philippe Mathieu-Daudé 2019-07-01 13:07 ` Igor Mammedov @ 2019-07-01 13:51 ` Christophe de Dinechin 2 siblings, 0 replies; 23+ messages in thread From: Christophe de Dinechin @ 2019-07-01 13:51 UTC (permalink / raw) To: qemu-devel; +Cc: Igor Mammedov Michael S. Tsirkin writes: > Rename function arguments to make intent clearer. > Better documentation for slot control logic. > > Suggested-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > > include/hw/pci/pcie.h | 3 ++- > hw/pci/pcie.c | 17 +++++++++++------ > 2 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index 8d90c0e193..34f277735c 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -108,7 +108,8 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev); > void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot); > void pcie_cap_slot_reset(PCIDevice *dev); > void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta); > -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slot_ctl, uint16_t slt_sta, > +void pcie_cap_slot_write_config(PCIDevice *dev, > + uint16_t old_slot_ctl, uint16_t old_slt_sta, > uint32_t addr, uint32_t val, int len); > int pcie_cap_slot_post_load(void *opaque, int version_id); > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index c605d32dd4..a6beb567bd 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -602,7 +602,8 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta) > *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > } > > -void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_sta, > +void pcie_cap_slot_write_config(PCIDevice *dev, > + uint16_t old_slt_ctl, uint16_t old_slt_sta, > uint32_t addr, uint32_t val, int len) > { > uint32_t pos = dev->exp.exp_cap; > @@ -625,8 +626,8 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | \ > PCI_EXP_SLTSTA_CC) > > - if (val & ~slt_sta & PCIE_SLOT_EVENTS) { > - sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (slt_sta & PCIE_SLOT_EVENTS); > + if (val & ~old_slt_sta & PCIE_SLOT_EVENTS) { > + sltsta = (sltsta & ~PCIE_SLOT_EVENTS) | (old_slt_sta & PCIE_SLOT_EVENTS); > pci_set_word(exp_cap + PCI_EXP_SLTSTA, sltsta); > } > hotplug_event_clear(dev); > @@ -646,13 +647,17 @@ void pcie_cap_slot_write_config(PCIDevice *dev, uint16_t slt_ctl, uint16_t slt_s > } > > /* > - * If the slot is polulated, power indicator is off and power > + * If the slot is populated, power indicator is off and power > * controller is off, it is safe to detach the devices. > + * > + * Note: don't detach if condition was already true: > + * this is a work around for guests that overwrite > + * control of powered off slots before powering them on. > */ > if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > - (!(slt_ctl & PCI_EXP_SLTCTL_PCC) || > - (slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > + (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || > + (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > pcie_unplug_device, NULL); Good idea. Reviewed-by: Christophe de Dinechin <dinechin@redhat.com> -- Cheers, Christophe de Dinechin (IRC c3d) ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-07-01 14:43 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-21 6:46 [Qemu-devel] [PATCH 0/3] pcie: hotplug fixes Michael S. Tsirkin 2019-06-21 6:46 ` [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events Michael S. Tsirkin 2019-06-25 11:14 ` Igor Mammedov 2019-07-01 7:01 ` Marcel Apfelbaum 2019-07-01 9:56 ` Philippe Mathieu-Daudé 2019-06-21 6:46 ` [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting Michael S. Tsirkin 2019-06-25 12:45 ` Igor Mammedov 2019-07-01 9:23 ` Michael S. Tsirkin 2019-07-01 7:03 ` Marcel Apfelbaum 2019-07-01 13:07 ` Igor Mammedov 2019-06-21 6:46 ` [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init Michael S. Tsirkin 2019-06-25 13:07 ` Igor Mammedov 2019-07-01 9:20 ` Michael S. Tsirkin 2019-07-01 12:04 ` Igor Mammedov 2019-07-01 12:08 ` Michael S. Tsirkin 2019-07-01 7:04 ` Marcel Apfelbaum [not found] ` <20190701105708.5d28f497@redhat.com> 2019-07-01 9:12 ` Marcel Apfelbaum 2019-07-01 9:13 ` Marcel Apfelbaum 2019-07-01 13:01 ` Igor Mammedov 2019-07-01 9:34 ` [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status Michael S. Tsirkin 2019-07-01 9:56 ` Philippe Mathieu-Daudé 2019-07-01 13:07 ` Igor Mammedov 2019-07-01 13:51 ` Christophe de Dinechin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).