Re: [Qemu-devel] [BUG]Unassigned mem write during pci device hot-plug

[xuyandong]

> -----Original Message-----
> From: xuyandong
> Sent: Monday, January 07, 2019 10:37 PM
> To: 'Michael S. Tsirkin' <address@hidden>
> Cc: address@hidden; Paolo Bonzini <address@hidden>; qemu-
> address@hidden; Zhanghailiang <address@hidden>;
> wangxin (U) <address@hidden>; Huangweidong (C)
> <address@hidden>
> Subject: RE: [BUG]Unassigned mem write during pci device hot-plug
> 
> > > > > > > > > > > Hi all,
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > In our test, we configured VM with several
> > > > > > > > > > > pci-bridges and a virtio-net nic been attached with
> > > > > > > > > > > bus 4,
> > > > > > > > > > >
> > > > > > > > > > > After VM is startup, We ping this nic from host to
> > > > > > > > > > > judge if it is working normally. Then, we hot add
> > > > > > > > > > > pci devices to this VM with bus
> > > > > > 0.
> > > > > > > > > > >
> > > > > > > > > > > We  found the virtio-net NIC in bus 4 is not working
> > > > > > > > > > > (can not
> > > > > > > > > > > connect) occasionally, as it kick virtio backend
> > > > > > > > > > > failure with error
> 
> > > > > But I have another question, if we only fix this problem in the
> > > > > kernel, the Linux version that has been released does not work
> > > > > well on the
> > > > virtualization platform.
> > > > > Is there a way to fix this problem in the backend?
> > > >
> > > > There could we a way to work around this.
> > > > Does below help?
> > >
> > > I am sorry to tell you, I tested this patch and it doesn't work fine.
> > >
> > > >
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index
> > > > 236a20eaa8..7834cac4b0 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -551,7 +551,7 @@ static void build_append_pci_bus_devices(Aml
> > > > *parent_scope, PCIBus *bus,
> > > >
> > > >          aml_append(method, aml_store(aml_int(bsel_val),
> > aml_name("BNUM")));
> > > >          aml_append(method,
> > > > -            aml_call2("DVNT", aml_name("PCIU"), aml_int(1) /* Device
> Check
> > */)
> > > > +            aml_call2("DVNT", aml_name("PCIU"), aml_int(4) /*
> > > > + Device Check Light */)
> > > >          );
> > > >          aml_append(method,
> > > >              aml_call2("DVNT", aml_name("PCID"), aml_int(3)/*
> > > > Eject Request */)
> >
> >
> > Oh I see, another bug:
> >
> >         case ACPI_NOTIFY_DEVICE_CHECK_LIGHT:
> >                 acpi_handle_debug(handle,
> > "ACPI_NOTIFY_DEVICE_CHECK_LIGHT event\n");
> >                 /* TBD: Exactly what does 'light' mean? */
> >                 break;
> >
> > And then e.g. acpi_generic_hotplug_event(struct acpi_device *adev, u32
> > type) and friends all just ignore this event type.
> >
> >
> >
> > --
> > MST
> 
> Hi Michael,
> 
> If we want to fix this problem on the backend, it is not enough to consider 
> only
> PCI device hot plugging, because I found that if we use a command like "echo 
> 1 >
> /sys/bus/pci/rescan" in guest, this problem is very easy to reproduce.
> 
> From the perspective of device emulation, when guest writes 0xffffffff to the
> BAR, guest just want to get the size of the region but not really updating the
> address space.
> So I made the following patch to avoid  update pci mapping.
> 
> Do you think this make sense?
> 
> [PATCH] pci: avoid update pci mapping when writing 0xFFFF FFFF to BAR
> 
> When guest writes 0xffffffff to the BAR, guest just want to get the size of 
> the
> region but not really updating the address space.
> So when guest writes 0xffffffff to BAR, we need avoid pci_update_mappings or
> pci_bridge_update_mappings.
> 
> Signed-off-by: xuyandong <address@hidden>
> ---
>  hw/pci/pci.c        | 6 ++++--
>  hw/pci/pci_bridge.c | 8 +++++---
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 56b13b3..ef368e1 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1361,6 +1361,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t
> addr, uint32_t val_in, int  {
>      int i, was_irq_disabled = pci_irq_disabled(d);
>      uint32_t val = val_in;
> +    uint64_t barmask = (1 << l*8) - 1;
> 
>      for (i = 0; i < l; val >>= 8, ++i) {
>          uint8_t wmask = d->wmask[addr + i]; @@ -1369,9 +1370,10 @@ void
> pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val_in, int
>          d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
>          d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
>      }
> -    if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
> +    if ((val_in != barmask &&
> +     (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
>          ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
> -        ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) ||
> +        ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4))) ||
>          range_covers_byte(addr, l, PCI_COMMAND))
>          pci_update_mappings(d);
> 
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index ee9dff2..f2bad79
> 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -253,17 +253,19 @@ void pci_bridge_write_config(PCIDevice *d,
>      PCIBridge *s = PCI_BRIDGE(d);
>      uint16_t oldctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
>      uint16_t newctl;
> +    uint64_t barmask = (1 << len * 8) - 1;
> 
>      pci_default_write_config(d, address, val, len);
> 
>      if (ranges_overlap(address, len, PCI_COMMAND, 2) ||
> 
> -        /* io base/limit */
> -        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
> +        (val != barmask &&
> +     /* io base/limit */
> +        (ranges_overlap(address, len, PCI_IO_BASE, 2) ||
> 
>          /* memory base/limit, prefetchable base/limit and
>             io base/limit upper 16 */
> -        ranges_overlap(address, len, PCI_MEMORY_BASE, 20) ||
> +        ranges_overlap(address, len, PCI_MEMORY_BASE, 20))) ||
> 
>          /* vga enable */
>          ranges_overlap(address, len, PCI_BRIDGE_CONTROL, 2)) {
> --
> 1.8.3.1
> 
> 

Sorry, please ignore the patch above.

Here is the patch I want to post:

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 56b13b3..38a300f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1361,6 +1361,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t 
addr, uint32_t val_in, int
 {
     int i, was_irq_disabled = pci_irq_disabled(d);
     uint32_t val = val_in;
+    uint64_t barmask = ((uint64_t)1 << l*8) - 1;
 
     for (i = 0; i < l; val >>= 8, ++i) {
         uint8_t wmask = d->wmask[addr + i];
@@ -1369,9 +1370,10 @@ void pci_default_write_config(PCIDevice *d, uint32_t 
addr, uint32_t val_in, int
         d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
         d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
     }
-    if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
+    if ((val_in != barmask &&
+       (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
         ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
-        ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) ||
+        ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4))) ||
         range_covers_byte(addr, l, PCI_COMMAND))
         pci_update_mappings(d);
 
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index ee9dff2..b8f7d48 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -253,20 +253,22 @@ void pci_bridge_write_config(PCIDevice *d,
     PCIBridge *s = PCI_BRIDGE(d);
     uint16_t oldctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
     uint16_t newctl;
+    uint64_t barmask = ((uint64_t)1 << len * 8) - 1;
 
     pci_default_write_config(d, address, val, len);
 
     if (ranges_overlap(address, len, PCI_COMMAND, 2) ||
 
-        /* io base/limit */
-        ranges_overlap(address, len, PCI_IO_BASE, 2) ||
+        /* vga enable */
+        ranges_overlap(address, len, PCI_BRIDGE_CONTROL, 2) ||
 
-        /* memory base/limit, prefetchable base/limit and
-           io base/limit upper 16 */
-        ranges_overlap(address, len, PCI_MEMORY_BASE, 20) ||
+        (val != barmask &&
+        /* io base/limit */
+         (ranges_overlap(address, len, PCI_IO_BASE, 2) ||
 
-        /* vga enable */
-        ranges_overlap(address, len, PCI_BRIDGE_CONTROL, 2)) {
+         /* memory base/limit, prefetchable base/limit and
+            io base/limit upper 16 */
+         ranges_overlap(address, len, PCI_MEMORY_BASE, 20)))) {
         pci_bridge_update_mappings(s);
     }
 
-- 
1.8.3.1