[Qemu-devel] [PATCH 0/3] pcie: hotplug fixes

[Qemu-devel] [PATCH 0/3] pcie: hotplug fixes

[Michael S. Tsirkin]

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

[Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events

[Michael S. Tsirkin]

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

[Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting

[Michael S. Tsirkin]

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

[Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events

[Igor Mammedov]

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);

Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting

[Igor Mammedov]

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);

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Igor Mammedov]

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);
>      }
>  

Re: [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events

[Marcel Apfelbaum]

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

Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting

[Marcel Apfelbaum]

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

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Marcel Apfelbaum]

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

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Marcel Apfelbaum]

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
>>

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Marcel Apfelbaum]

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
>>

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Michael S. Tsirkin]

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);
> >      }
> >  

Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting

[Michael S. Tsirkin]

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);

[Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status

[Philippe Mathieu-Daudé]

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>

Re: [Qemu-devel] [PATCH 1/3] pcie: don't skip multi-mask events

[Philippe Mathieu-Daudé]

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>

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Igor Mammedov]

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);
> > >      }
> > >    
> 

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Michael S. Tsirkin]

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);
> > > >      }
> > > >    
> > 

Re: [Qemu-devel] [PATCH 3/3] pcie: work around for racy guest init

[Igor Mammedov]

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);
>      }
>  

Re: [Qemu-devel] [PATCH 2/3] pcie: check that slt ctrl changed before deleting

[Igor Mammedov]

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);

Re: [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status

[Igor Mammedov]

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);

Re: [Qemu-devel] [PATCH 4/3] pcie: minor cleanups for slot control/status

[Christophe de Dinechin]

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)