> -----Original Message----- > From: xuyandong > Sent: Monday, January 07, 2019 10:37 PM > To: 'Michael S. Tsirkin' <m...@redhat.com> > Cc: mar...@redhat.com; Paolo Bonzini <pbonz...@redhat.com>; qemu- > de...@nongnu.org; Zhanghailiang <zhang.zhanghaili...@huawei.com>; > wangxin (U) <wangxinxin.w...@huawei.com>; Huangweidong (C) > <weidong.hu...@huawei.com> > 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 <xuyando...@huawei.com> > --- > 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