* [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges
@ 2015-06-08 21:18 Don Slutz
2015-06-08 21:18 ` [Qemu-devel] [BUGFIX][PATCH v2 1/4] exec: Do not use MemoryRegion after free Don Slutz
` (4 more replies)
0 siblings, 5 replies; 23+ messages in thread
From: Don Slutz @ 2015-06-08 21:18 UTC (permalink / raw)
To: qemu-devel, xen-devel
Cc: Paul Durrant, Don Slutz, Stefano Stabellini, Michael S. Tsirkin
changes v1 to v2:
Split v1 patch into 3.
Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion after
free").
Technically this bug fix should be a separate patch, however this
issue only seems to reproduce when this patch set is applied.
Michael S. Tsirkin:
"You need some other API that makes sense, probably PCI specific."
This is basically patch #2: "Extend device listener interface..."
"This is relying on undocumented assumptions and how specific
firmware works. There's nothing special about bus number 255,
and 0 is not very special either (except it happens to be the
reset value)."
Dropped all checking of 0 and 255.
Open question by Michael S. Tsirkin:
>>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote:
...
>>>> It is not clear to me that the complexity of tracking bus
>>>> visibility make sense. Clearly you do.
>>> Well what was the point of the change?
>
> To get config cycles that are valid that you do not get today.
>
>>> If you don't care that we get irrelevant config cycles why not
>>> just send them all to QEMU?
>>>
>
> That would need to be answered by Paul Durrant and/or other people (see
> below)
>
We could broadcast config space ioreqs to all emulators, the problem
is how do we know (in the case of a read) which emulator is actually
the one supplying the data? At some level Xen needs to know who is
implementing what.
Paul Durrant
So this patch set just adjusts Xen to correctly know the current set
of PCI devices and their bus, device, and function.
It does not attempt to calculate the visibility of the PCI devices.
v1:
Note: Right now hvmloader in Xen does not handle PCI to PCI bridges
and so SeaBIOS does not have access to PCI device(s) on secondary
buses. How ever domU can setup the secondary bus(es) and this patch
will restore access to these PCI devices.
Don Slutz (4):
exec: Do not use MemoryRegion after free
Extend device listener interface for PCI to PCI bridges
xen: Add usage of device listener interface for PCI to PCI bridges
xen: Fix map/unmap of pcidev to ioreq server
exec.c | 8 ++++--
hw/core/qdev.c | 7 ++++++
hw/pci/pci_bridge.c | 18 +++++++++++++
include/hw/qdev-core.h | 3 +++
include/hw/xen/xen_common.h | 61 ++++++++++++++++++++++++++++++++++-----------
trace-events | 6 +++--
xen-hvm.c | 20 +++++++++++++--
7 files changed, 102 insertions(+), 21 deletions(-)
--
1.8.4
^ permalink raw reply [flat|nested] 23+ messages in thread* [Qemu-devel] [BUGFIX][PATCH v2 1/4] exec: Do not use MemoryRegion after free 2015-06-08 21:18 [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Don Slutz @ 2015-06-08 21:18 ` Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges Don Slutz ` (3 subsequent siblings) 4 siblings, 0 replies; 23+ messages in thread From: Don Slutz @ 2015-06-08 21:18 UTC (permalink / raw) To: qemu-devel, xen-devel Cc: Paul Durrant, Don Slutz, Don Slutz, Stefano Stabellini, Michael S. Tsirkin Here is gdb output that shows this happening: Breakpoint 3, object_finalize (data=0x7fdf32a14010) at qom/object.c:417 417 obj->free(obj); (gdb) bt #0 object_finalize (data=0x7fdf32a14010) at qom/object.c:417 #1 0x00000000007329d4 in object_unref (obj=0x7fdf32a14010) at qom/object.c:720 #2 0x0000000000468a65 in memory_region_unref (mr=0x7fdf32a168b0) at xen/tools/qemu-xen-dir/memory.c:1359 #3 0x000000000040eb52 in phys_section_destroy (mr=0x7fdf32a168b0) at xen/tools/qemu-xen-dir/exec.c:960 #4 0x000000000040ec0a in phys_sections_free (map=0x3e51fc8) at xen/tools/qemu-xen-dir/exec.c:973 #5 0x0000000000411cc9 in address_space_dispatch_free (d=0x3e51fb0) at xen/tools/qemu-xen-dir/exec.c:2133 #6 0x0000000000840ae2 in call_rcu_thread (opaque=0x0) at util/rcu.c:256 #7 0x00000032fdc07d14 in start_thread (arg=0x7fdf34866700) at pthread_create.c:309 #8 0x00000032fd4f168d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115 (gdb) p obj $5 = (Object *) 0x7fdf32a14010 (gdb) p *obj $6 = {class = 0x302f380, free = 0x40a1e0 <g_free@plt>, properties = {tqh_first = 0x0, tqh_last = 0x7fdf32a14020}, ref = 0, parent = 0x0} (gdb) up #1 0x00000000007329d4 in object_unref (obj=0x7fdf32a14010) at qom/object.c:720 720 object_finalize(obj); (gdb) up #2 0x0000000000468a65 in memory_region_unref (mr=0x7fdf32a168b0) at xen/tools/qemu-xen-dir/memory.c:1359 1359 object_unref(obj->parent); (gdb) up #3 0x000000000040eb52 in phys_section_destroy (mr=0x7fdf32a168b0) at xen/tools/qemu-xen-dir/exec.c:960 960 memory_region_unref(mr); (gdb) l 955 return map->sections_nb++; 956 } 957 958 static void phys_section_destroy(MemoryRegion *mr) 959 { 960 memory_region_unref(mr); 961 962 if (mr->subpage) { 963 subpage_t *subpage = container_of(mr, subpage_t, iomem); 964 object_unref(OBJECT(&subpage->iomem)); (gdb) p mr $7 = (MemoryRegion *) 0x7fdf32a168b0 (gdb) p mr->subpage $9 = false (gdb) n 419 } (gdb) n object_unref (obj=0x7fdf32a14010) at qom/object.c:722 722 } (gdb) n memory_region_unref (mr=0x7fdf32a168b0) at xen/tools/qemu-xen-dir/memory.c:1363 1363 } (gdb) n phys_section_destroy (mr=0x7fdf32a168b0) at xen/tools/qemu-xen-dir/exec.c:962 962 if (mr->subpage) { (gdb) p mr $10 = (MemoryRegion *) 0x7fdf32a168b0 (gdb) p *mr Cannot access memory at address 0x7fdf32a168b0 Signed-off-by: Don Slutz <dslutz@verizon.com> CC: Don Slutz <don.slutz@gmail.com> --- exec.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/exec.c b/exec.c index 487583b..2f44a80 100644 --- a/exec.c +++ b/exec.c @@ -957,10 +957,14 @@ static uint16_t phys_section_add(PhysPageMap *map, static void phys_section_destroy(MemoryRegion *mr) { - memory_region_unref(mr); + subpage_t *subpage = NULL; if (mr->subpage) { - subpage_t *subpage = container_of(mr, subpage_t, iomem); + subpage = container_of(mr, subpage_t, iomem); + } + memory_region_unref(mr); + + if (subpage) { object_unref(OBJECT(&subpage->iomem)); g_free(subpage); } -- 1.8.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges 2015-06-08 21:18 [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [BUGFIX][PATCH v2 1/4] exec: Do not use MemoryRegion after free Don Slutz @ 2015-06-08 21:18 ` Don Slutz 2015-06-09 9:08 ` Paul Durrant 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 3/4] xen: Add usage of " Don Slutz ` (2 subsequent siblings) 4 siblings, 1 reply; 23+ messages in thread From: Don Slutz @ 2015-06-08 21:18 UTC (permalink / raw) To: qemu-devel, xen-devel Cc: Paul Durrant, Don Slutz, Don Slutz, Stefano Stabellini, Michael S. Tsirkin The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a PCI device has a static address. This is not true for PCI devices that are on the secondary bus of a PCI to PCI bridge. BIOS and/or guest OS can change the secondary bus number to a new value at any time. Extend the device listener interface to be called when ever the secondary bus number is set to a new value. This new interface is called for all PCI devices that are on the secondary bridge. Signed-off-by: Don Slutz <dslutz@verizon.com> CC: Don Slutz <don.slutz@gmail.com> --- hw/core/qdev.c | 7 +++++++ hw/pci/pci_bridge.c | 18 ++++++++++++++++++ include/hw/qdev-core.h | 3 +++ 3 files changed, 28 insertions(+) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index b0f0f84..7728ee7 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -239,6 +239,13 @@ void device_listener_unregister(DeviceListener *listener) QTAILQ_REMOVE(&device_listeners, listener, link); } +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void *opaque) +{ + uint8_t oldbus = GPOINTER_TO_INT(opaque); + + DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus); +} + static void device_realize(DeviceState *dev, Error **errp) { DeviceClass *dc = DEVICE_GET_CLASS(dev); diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index 40c97b1..742c4d8 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -248,6 +248,7 @@ 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; + uint8_t oldbus = pci_get_byte(d->config + PCI_SECONDARY_BUS); pci_default_write_config(d, address, val, len); @@ -265,6 +266,14 @@ void pci_bridge_write_config(PCIDevice *d, pci_bridge_update_mappings(s); } + if (oldbus != pci_get_byte(d->config + PCI_SECONDARY_BUS)) { + PCIBus *sec_bus = pci_bridge_get_sec_bus(s); + + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), + device_listener_change_pci_bus_num, + GINT_TO_POINTER(oldbus)); + } + newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) { /* Trigger hot reset on 0->1 transition. */ @@ -297,6 +306,7 @@ void pci_bridge_reset(DeviceState *qdev) { PCIDevice *dev = PCI_DEVICE(qdev); uint8_t *conf = dev->config; + uint8_t oldbus = conf[PCI_SECONDARY_BUS]; conf[PCI_PRIMARY_BUS] = 0; conf[PCI_SECONDARY_BUS] = 0; @@ -329,6 +339,14 @@ void pci_bridge_reset(DeviceState *qdev) pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0); pci_set_word(conf + PCI_BRIDGE_CONTROL, 0); + + if (oldbus) { + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); + + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), + device_listener_change_pci_bus_num, + GINT_TO_POINTER(oldbus)); + } } /* default qdev initialization function for PCI-to-PCI bridge */ diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index d4be92f..154b4c1 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -168,6 +168,8 @@ struct DeviceState { struct DeviceListener { void (*realize)(DeviceListener *listener, DeviceState *dev); void (*unrealize)(DeviceListener *listener, DeviceState *dev); + void (*change_pci_bus_num)(DeviceListener *listener, PCIDevice *d, + uint8_t oldbus); QTAILQ_ENTRY(DeviceListener) link; }; @@ -387,5 +389,6 @@ static inline bool qbus_is_hotpluggable(BusState *bus) void device_listener_register(DeviceListener *listener); void device_listener_unregister(DeviceListener *listener); +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void *opaque); #endif -- 1.8.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges Don Slutz @ 2015-06-09 9:08 ` Paul Durrant 2015-06-09 13:53 ` Don Slutz 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 9:08 UTC (permalink / raw) To: Don Slutz, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 08 June 2015 22:19 > To: qemu-devel@nongnu.org; xen-devel@lists.xen.org > Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz > Subject: [PATCH v2 2/4] Extend device listener interface for PCI to PCI > bridges > > The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a > PCI device has a static address. This is not true for PCI devices > that are on the secondary bus of a PCI to PCI bridge. > > BIOS and/or guest OS can change the secondary bus number to a new > value at any time. > > Extend the device listener interface to be called when ever the > secondary bus number is set to a new value. This new interface > is called for all PCI devices that are on the secondary bridge. > Can't you just make unrealize and realize calls back-to-back when the bus number changes, rather than having a new callback? Paul > Signed-off-by: Don Slutz <dslutz@verizon.com> > CC: Don Slutz <don.slutz@gmail.com> > --- > hw/core/qdev.c | 7 +++++++ > hw/pci/pci_bridge.c | 18 ++++++++++++++++++ > include/hw/qdev-core.h | 3 +++ > 3 files changed, 28 insertions(+) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index b0f0f84..7728ee7 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -239,6 +239,13 @@ void device_listener_unregister(DeviceListener > *listener) > QTAILQ_REMOVE(&device_listeners, listener, link); > } > > +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void > *opaque) > +{ > + uint8_t oldbus = GPOINTER_TO_INT(opaque); > + > + DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus); > +} > + > static void device_realize(DeviceState *dev, Error **errp) > { > DeviceClass *dc = DEVICE_GET_CLASS(dev); > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c > index 40c97b1..742c4d8 100644 > --- a/hw/pci/pci_bridge.c > +++ b/hw/pci/pci_bridge.c > @@ -248,6 +248,7 @@ 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; > + uint8_t oldbus = pci_get_byte(d->config + PCI_SECONDARY_BUS); > > pci_default_write_config(d, address, val, len); > > @@ -265,6 +266,14 @@ void pci_bridge_write_config(PCIDevice *d, > pci_bridge_update_mappings(s); > } > > + if (oldbus != pci_get_byte(d->config + PCI_SECONDARY_BUS)) { > + PCIBus *sec_bus = pci_bridge_get_sec_bus(s); > + > + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > + device_listener_change_pci_bus_num, > + GINT_TO_POINTER(oldbus)); > + } > + > newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); > if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) { > /* Trigger hot reset on 0->1 transition. */ > @@ -297,6 +306,7 @@ void pci_bridge_reset(DeviceState *qdev) > { > PCIDevice *dev = PCI_DEVICE(qdev); > uint8_t *conf = dev->config; > + uint8_t oldbus = conf[PCI_SECONDARY_BUS]; > > conf[PCI_PRIMARY_BUS] = 0; > conf[PCI_SECONDARY_BUS] = 0; > @@ -329,6 +339,14 @@ void pci_bridge_reset(DeviceState *qdev) > pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0); > > pci_set_word(conf + PCI_BRIDGE_CONTROL, 0); > + > + if (oldbus) { > + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > + > + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > + device_listener_change_pci_bus_num, > + GINT_TO_POINTER(oldbus)); > + } > } > > /* default qdev initialization function for PCI-to-PCI bridge */ > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index d4be92f..154b4c1 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -168,6 +168,8 @@ struct DeviceState { > struct DeviceListener { > void (*realize)(DeviceListener *listener, DeviceState *dev); > void (*unrealize)(DeviceListener *listener, DeviceState *dev); > + void (*change_pci_bus_num)(DeviceListener *listener, PCIDevice *d, > + uint8_t oldbus); > QTAILQ_ENTRY(DeviceListener) link; > }; > > @@ -387,5 +389,6 @@ static inline bool qbus_is_hotpluggable(BusState > *bus) > > void device_listener_register(DeviceListener *listener); > void device_listener_unregister(DeviceListener *listener); > +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void > *opaque); > > #endif > -- > 1.8.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges 2015-06-09 9:08 ` Paul Durrant @ 2015-06-09 13:53 ` Don Slutz 2015-06-09 13:55 ` Paul Durrant 0 siblings, 1 reply; 23+ messages in thread From: Don Slutz @ 2015-06-09 13:53 UTC (permalink / raw) To: Paul Durrant, Slutz, Donald Christopher, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin On 06/09/15 05:08, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 08 June 2015 22:19 >> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org >> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz >> Subject: [PATCH v2 2/4] Extend device listener interface for PCI to PCI >> bridges >> >> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a >> PCI device has a static address. This is not true for PCI devices >> that are on the secondary bus of a PCI to PCI bridge. >> >> BIOS and/or guest OS can change the secondary bus number to a new >> value at any time. >> >> Extend the device listener interface to be called when ever the >> secondary bus number is set to a new value. This new interface >> is called for all PCI devices that are on the secondary bridge. >> > > Can't you just make unrealize and realize calls back-to-back when the bus number changes, rather than having a new callback? > That is what V1 had. Michael S. Tsirkin did not like this. -Don Slutz > Paul > >> Signed-off-by: Don Slutz <dslutz@verizon.com> >> CC: Don Slutz <don.slutz@gmail.com> >> --- >> hw/core/qdev.c | 7 +++++++ >> hw/pci/pci_bridge.c | 18 ++++++++++++++++++ >> include/hw/qdev-core.h | 3 +++ >> 3 files changed, 28 insertions(+) >> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index b0f0f84..7728ee7 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -239,6 +239,13 @@ void device_listener_unregister(DeviceListener >> *listener) >> QTAILQ_REMOVE(&device_listeners, listener, link); >> } >> >> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void >> *opaque) >> +{ >> + uint8_t oldbus = GPOINTER_TO_INT(opaque); >> + >> + DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus); >> +} >> + >> static void device_realize(DeviceState *dev, Error **errp) >> { >> DeviceClass *dc = DEVICE_GET_CLASS(dev); >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c >> index 40c97b1..742c4d8 100644 >> --- a/hw/pci/pci_bridge.c >> +++ b/hw/pci/pci_bridge.c >> @@ -248,6 +248,7 @@ 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; >> + uint8_t oldbus = pci_get_byte(d->config + PCI_SECONDARY_BUS); >> >> pci_default_write_config(d, address, val, len); >> >> @@ -265,6 +266,14 @@ void pci_bridge_write_config(PCIDevice *d, >> pci_bridge_update_mappings(s); >> } >> >> + if (oldbus != pci_get_byte(d->config + PCI_SECONDARY_BUS)) { >> + PCIBus *sec_bus = pci_bridge_get_sec_bus(s); >> + >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >> + device_listener_change_pci_bus_num, >> + GINT_TO_POINTER(oldbus)); >> + } >> + >> newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); >> if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) { >> /* Trigger hot reset on 0->1 transition. */ >> @@ -297,6 +306,7 @@ void pci_bridge_reset(DeviceState *qdev) >> { >> PCIDevice *dev = PCI_DEVICE(qdev); >> uint8_t *conf = dev->config; >> + uint8_t oldbus = conf[PCI_SECONDARY_BUS]; >> >> conf[PCI_PRIMARY_BUS] = 0; >> conf[PCI_SECONDARY_BUS] = 0; >> @@ -329,6 +339,14 @@ void pci_bridge_reset(DeviceState *qdev) >> pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0); >> >> pci_set_word(conf + PCI_BRIDGE_CONTROL, 0); >> + >> + if (oldbus) { >> + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); >> + >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >> + device_listener_change_pci_bus_num, >> + GINT_TO_POINTER(oldbus)); >> + } >> } >> >> /* default qdev initialization function for PCI-to-PCI bridge */ >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index d4be92f..154b4c1 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -168,6 +168,8 @@ struct DeviceState { >> struct DeviceListener { >> void (*realize)(DeviceListener *listener, DeviceState *dev); >> void (*unrealize)(DeviceListener *listener, DeviceState *dev); >> + void (*change_pci_bus_num)(DeviceListener *listener, PCIDevice *d, >> + uint8_t oldbus); >> QTAILQ_ENTRY(DeviceListener) link; >> }; >> >> @@ -387,5 +389,6 @@ static inline bool qbus_is_hotpluggable(BusState >> *bus) >> >> void device_listener_register(DeviceListener *listener); >> void device_listener_unregister(DeviceListener *listener); >> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, void >> *opaque); >> >> #endif >> -- >> 1.8.4 > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges 2015-06-09 13:53 ` Don Slutz @ 2015-06-09 13:55 ` Paul Durrant 2015-06-09 14:00 ` Don Slutz 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 13:55 UTC (permalink / raw) To: Don Slutz, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 09 June 2015 14:53 > To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; xen- > devel@lists.xen.org > Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz > Subject: Re: [PATCH v2 2/4] Extend device listener interface for PCI to PCI > bridges > > On 06/09/15 05:08, Paul Durrant wrote: > >> -----Original Message----- > >> From: Don Slutz [mailto:dslutz@verizon.com] > >> Sent: 08 June 2015 22:19 > >> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org > >> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz > >> Subject: [PATCH v2 2/4] Extend device listener interface for PCI to PCI > >> bridges > >> > >> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a > >> PCI device has a static address. This is not true for PCI devices > >> that are on the secondary bus of a PCI to PCI bridge. > >> > >> BIOS and/or guest OS can change the secondary bus number to a new > >> value at any time. > >> > >> Extend the device listener interface to be called when ever the > >> secondary bus number is set to a new value. This new interface > >> is called for all PCI devices that are on the secondary bridge. > >> > > > > Can't you just make unrealize and realize calls back-to-back when the bus > number changes, rather than having a new callback? > > > > That is what V1 had. Michael S. Tsirkin did not like this. > I thought he objected to you actually calling the actual bus unrealize/realize functions. All I'm suggesting is calling the device listener unrealize/realize callbacks, rather than adding a new one. Paul > -Don Slutz > > > Paul > > > >> Signed-off-by: Don Slutz <dslutz@verizon.com> > >> CC: Don Slutz <don.slutz@gmail.com> > >> --- > >> hw/core/qdev.c | 7 +++++++ > >> hw/pci/pci_bridge.c | 18 ++++++++++++++++++ > >> include/hw/qdev-core.h | 3 +++ > >> 3 files changed, 28 insertions(+) > >> > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c > >> index b0f0f84..7728ee7 100644 > >> --- a/hw/core/qdev.c > >> +++ b/hw/core/qdev.c > >> @@ -239,6 +239,13 @@ void device_listener_unregister(DeviceListener > >> *listener) > >> QTAILQ_REMOVE(&device_listeners, listener, link); > >> } > >> > >> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, > void > >> *opaque) > >> +{ > >> + uint8_t oldbus = GPOINTER_TO_INT(opaque); > >> + > >> + DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus); > >> +} > >> + > >> static void device_realize(DeviceState *dev, Error **errp) > >> { > >> DeviceClass *dc = DEVICE_GET_CLASS(dev); > >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c > >> index 40c97b1..742c4d8 100644 > >> --- a/hw/pci/pci_bridge.c > >> +++ b/hw/pci/pci_bridge.c > >> @@ -248,6 +248,7 @@ 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; > >> + uint8_t oldbus = pci_get_byte(d->config + PCI_SECONDARY_BUS); > >> > >> pci_default_write_config(d, address, val, len); > >> > >> @@ -265,6 +266,14 @@ void pci_bridge_write_config(PCIDevice *d, > >> pci_bridge_update_mappings(s); > >> } > >> > >> + if (oldbus != pci_get_byte(d->config + PCI_SECONDARY_BUS)) { > >> + PCIBus *sec_bus = pci_bridge_get_sec_bus(s); > >> + > >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > >> + device_listener_change_pci_bus_num, > >> + GINT_TO_POINTER(oldbus)); > >> + } > >> + > >> newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); > >> if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) { > >> /* Trigger hot reset on 0->1 transition. */ > >> @@ -297,6 +306,7 @@ void pci_bridge_reset(DeviceState *qdev) > >> { > >> PCIDevice *dev = PCI_DEVICE(qdev); > >> uint8_t *conf = dev->config; > >> + uint8_t oldbus = conf[PCI_SECONDARY_BUS]; > >> > >> conf[PCI_PRIMARY_BUS] = 0; > >> conf[PCI_SECONDARY_BUS] = 0; > >> @@ -329,6 +339,14 @@ void pci_bridge_reset(DeviceState *qdev) > >> pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0); > >> > >> pci_set_word(conf + PCI_BRIDGE_CONTROL, 0); > >> + > >> + if (oldbus) { > >> + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > >> + > >> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > >> + device_listener_change_pci_bus_num, > >> + GINT_TO_POINTER(oldbus)); > >> + } > >> } > >> > >> /* default qdev initialization function for PCI-to-PCI bridge */ > >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > >> index d4be92f..154b4c1 100644 > >> --- a/include/hw/qdev-core.h > >> +++ b/include/hw/qdev-core.h > >> @@ -168,6 +168,8 @@ struct DeviceState { > >> struct DeviceListener { > >> void (*realize)(DeviceListener *listener, DeviceState *dev); > >> void (*unrealize)(DeviceListener *listener, DeviceState *dev); > >> + void (*change_pci_bus_num)(DeviceListener *listener, PCIDevice *d, > >> + uint8_t oldbus); > >> QTAILQ_ENTRY(DeviceListener) link; > >> }; > >> > >> @@ -387,5 +389,6 @@ static inline bool qbus_is_hotpluggable(BusState > >> *bus) > >> > >> void device_listener_register(DeviceListener *listener); > >> void device_listener_unregister(DeviceListener *listener); > >> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, > void > >> *opaque); > >> > >> #endif > >> -- > >> 1.8.4 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges 2015-06-09 13:55 ` Paul Durrant @ 2015-06-09 14:00 ` Don Slutz 0 siblings, 0 replies; 23+ messages in thread From: Don Slutz @ 2015-06-09 14:00 UTC (permalink / raw) To: Paul Durrant, Slutz, Donald Christopher, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin On 06/09/15 09:55, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 09 June 2015 14:53 >> To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; xen- >> devel@lists.xen.org >> Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz >> Subject: Re: [PATCH v2 2/4] Extend device listener interface for PCI to PCI >> bridges >> >> On 06/09/15 05:08, Paul Durrant wrote: >>>> -----Original Message----- >>>> From: Don Slutz [mailto:dslutz@verizon.com] >>>> Sent: 08 June 2015 22:19 >>>> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org >>>> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz >>>> Subject: [PATCH v2 2/4] Extend device listener interface for PCI to PCI >>>> bridges >>>> >>>> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a >>>> PCI device has a static address. This is not true for PCI devices >>>> that are on the secondary bus of a PCI to PCI bridge. >>>> >>>> BIOS and/or guest OS can change the secondary bus number to a new >>>> value at any time. >>>> >>>> Extend the device listener interface to be called when ever the >>>> secondary bus number is set to a new value. This new interface >>>> is called for all PCI devices that are on the secondary bridge. >>>> >>> >>> Can't you just make unrealize and realize calls back-to-back when the bus >> number changes, rather than having a new callback? >>> >> >> That is what V1 had. Michael S. Tsirkin did not like this. >> > > I thought he objected to you actually calling the actual bus unrealize/realize functions. All I'm suggesting is calling the device listener unrealize/realize callbacks, rather than adding a new one. > Nope. I added routines that would call on the device listener callbacks. Then added the calls to the new routines when the secondary bus number changed. What you are saying looks real close to what I did on V1. -Don Slutz > Paul > >> -Don Slutz >> >>> Paul >>> >>>> Signed-off-by: Don Slutz <dslutz@verizon.com> >>>> CC: Don Slutz <don.slutz@gmail.com> >>>> --- >>>> hw/core/qdev.c | 7 +++++++ >>>> hw/pci/pci_bridge.c | 18 ++++++++++++++++++ >>>> include/hw/qdev-core.h | 3 +++ >>>> 3 files changed, 28 insertions(+) >>>> >>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >>>> index b0f0f84..7728ee7 100644 >>>> --- a/hw/core/qdev.c >>>> +++ b/hw/core/qdev.c >>>> @@ -239,6 +239,13 @@ void device_listener_unregister(DeviceListener >>>> *listener) >>>> QTAILQ_REMOVE(&device_listeners, listener, link); >>>> } >>>> >>>> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, >> void >>>> *opaque) >>>> +{ >>>> + uint8_t oldbus = GPOINTER_TO_INT(opaque); >>>> + >>>> + DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus); >>>> +} >>>> + >>>> static void device_realize(DeviceState *dev, Error **errp) >>>> { >>>> DeviceClass *dc = DEVICE_GET_CLASS(dev); >>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c >>>> index 40c97b1..742c4d8 100644 >>>> --- a/hw/pci/pci_bridge.c >>>> +++ b/hw/pci/pci_bridge.c >>>> @@ -248,6 +248,7 @@ 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; >>>> + uint8_t oldbus = pci_get_byte(d->config + PCI_SECONDARY_BUS); >>>> >>>> pci_default_write_config(d, address, val, len); >>>> >>>> @@ -265,6 +266,14 @@ void pci_bridge_write_config(PCIDevice *d, >>>> pci_bridge_update_mappings(s); >>>> } >>>> >>>> + if (oldbus != pci_get_byte(d->config + PCI_SECONDARY_BUS)) { >>>> + PCIBus *sec_bus = pci_bridge_get_sec_bus(s); >>>> + >>>> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >>>> + device_listener_change_pci_bus_num, >>>> + GINT_TO_POINTER(oldbus)); >>>> + } >>>> + >>>> newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); >>>> if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) { >>>> /* Trigger hot reset on 0->1 transition. */ >>>> @@ -297,6 +306,7 @@ void pci_bridge_reset(DeviceState *qdev) >>>> { >>>> PCIDevice *dev = PCI_DEVICE(qdev); >>>> uint8_t *conf = dev->config; >>>> + uint8_t oldbus = conf[PCI_SECONDARY_BUS]; >>>> >>>> conf[PCI_PRIMARY_BUS] = 0; >>>> conf[PCI_SECONDARY_BUS] = 0; >>>> @@ -329,6 +339,14 @@ void pci_bridge_reset(DeviceState *qdev) >>>> pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0); >>>> >>>> pci_set_word(conf + PCI_BRIDGE_CONTROL, 0); >>>> + >>>> + if (oldbus) { >>>> + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); >>>> + >>>> + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), >>>> + device_listener_change_pci_bus_num, >>>> + GINT_TO_POINTER(oldbus)); >>>> + } >>>> } >>>> >>>> /* default qdev initialization function for PCI-to-PCI bridge */ >>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>>> index d4be92f..154b4c1 100644 >>>> --- a/include/hw/qdev-core.h >>>> +++ b/include/hw/qdev-core.h >>>> @@ -168,6 +168,8 @@ struct DeviceState { >>>> struct DeviceListener { >>>> void (*realize)(DeviceListener *listener, DeviceState *dev); >>>> void (*unrealize)(DeviceListener *listener, DeviceState *dev); >>>> + void (*change_pci_bus_num)(DeviceListener *listener, PCIDevice *d, >>>> + uint8_t oldbus); >>>> QTAILQ_ENTRY(DeviceListener) link; >>>> }; >>>> >>>> @@ -387,5 +389,6 @@ static inline bool qbus_is_hotpluggable(BusState >>>> *bus) >>>> >>>> void device_listener_register(DeviceListener *listener); >>>> void device_listener_unregister(DeviceListener *listener); >>>> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d, >> void >>>> *opaque); >>>> >>>> #endif >>>> -- >>>> 1.8.4 >>> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] xen: Add usage of device listener interface for PCI to PCI bridges 2015-06-08 21:18 [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [BUGFIX][PATCH v2 1/4] exec: Do not use MemoryRegion after free Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges Don Slutz @ 2015-06-08 21:18 ` Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server Don Slutz 2015-06-09 9:13 ` [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Michael S. Tsirkin 4 siblings, 0 replies; 23+ messages in thread From: Don Slutz @ 2015-06-08 21:18 UTC (permalink / raw) To: qemu-devel, xen-devel Cc: Paul Durrant, Don Slutz, Don Slutz, Stefano Stabellini, Michael S. Tsirkin Signed-off-by: Don Slutz <dslutz@verizon.com> CC: Don Slutz <don.slutz@gmail.com> --- include/hw/xen/xen_common.h | 10 ++++++---- xen-hvm.c | 13 ++++++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h index 38f29fb..6579b78 100644 --- a/include/hw/xen/xen_common.h +++ b/include/hw/xen/xen_common.h @@ -229,7 +229,8 @@ static inline void xen_map_pcidev(XenXC xc, domid_t dom, static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, ioservid_t ioservid, - PCIDevice *pci_dev) + PCIDevice *pci_dev, + uint8_t oldbus) { } @@ -357,12 +358,13 @@ static inline void xen_map_pcidev(XenXC xc, domid_t dom, static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, ioservid_t ioservid, - PCIDevice *pci_dev) + PCIDevice *pci_dev, + uint8_t oldbus) { - trace_xen_unmap_pcidev(ioservid, pci_bus_num(pci_dev->bus), + trace_xen_unmap_pcidev(ioservid, oldbus, PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, - 0, pci_bus_num(pci_dev->bus), + 0, oldbus, PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); } diff --git a/xen-hvm.c b/xen-hvm.c index 42356b8..7b6d8f1 100644 --- a/xen-hvm.c +++ b/xen-hvm.c @@ -590,10 +590,20 @@ static void xen_device_unrealize(DeviceListener *listener, if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { PCIDevice *pci_dev = PCI_DEVICE(dev); - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, + pci_bus_num(pci_dev->bus)); } } +static void xen_device_change_pci_bus_num(DeviceListener *listener, + PCIDevice *pci_dev, uint8_t oldbus) +{ + XenIOState *state = container_of(listener, XenIOState, device_listener); + + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, oldbus); + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); +} + static void xen_sync_dirty_bitmap(XenIOState *state, hwaddr start_addr, ram_addr_t size) @@ -709,6 +719,7 @@ static MemoryListener xen_io_listener = { static DeviceListener xen_device_listener = { .realize = xen_device_realize, .unrealize = xen_device_unrealize, + .change_pci_bus_num = xen_device_change_pci_bus_num, }; /* get the ioreq packets from share mem */ -- 1.8.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server 2015-06-08 21:18 [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Don Slutz ` (2 preceding siblings ...) 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 3/4] xen: Add usage of " Don Slutz @ 2015-06-08 21:18 ` Don Slutz 2015-06-09 9:05 ` Paul Durrant 2015-06-09 9:13 ` [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Michael S. Tsirkin 4 siblings, 1 reply; 23+ messages in thread From: Don Slutz @ 2015-06-08 21:18 UTC (permalink / raw) To: qemu-devel, xen-devel Cc: Paul Durrant, Don Slutz, Don Slutz, Stefano Stabellini, Michael S. Tsirkin The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 added usage of xc_hvm_map_pcidev_from_ioreq_server() and xc_hvm_unmap_pcidev_from_ioreq_server(). These routines only correctly work if the PCI BDF of a PCI device is unique. The 3 parts (bus, device, and function) of a PCI BDF are not required to be unique. The PCI BDF is accessed in QEMU: bus pci_bus_num() device PCI_SLOT() function PCI_FUNC() Add a hash table to track the current set of PCI BDFs and only call on xc_hvm_map_pcidev_from_ioreq_server() and xc_hvm_unmap_pcidev_from_ioreq_server() when needed. Also fix the PCI BDF default stderr trace output: bus is seperated by ':', and function is only 1 digit. Here is an example of stderr trace output: xen_map_pcidev id: 1 bdf: 00:00.0 xen_map_pcidev id: 1 bdf: 00:01.0 xen_map_pcidev id: 1 bdf: 00:01.1 xen_map_pcidev id: 1 bdf: 00:01.3 xen_map_pcidev id: 1 bdf: 00:02.0 xen_map_pcidev id: 1 bdf: 00:03.0 xen_map_pcidev id: 1 bdf: 00:04.0 xen_map_pcidev id: 1 bdf: 00:05.0 xen_map_pcidev id: 1 bdf: 00:11.0 xen_map_pcidev id: 1 bdf: 00:15.0 xen_map_pcidev id: 1 bdf: 00:16.0 xen_map_pcidev id: 1 bdf: 00:17.0 xen_map_pcidev id: 1 bdf: 00:18.0 xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 xen_map_pcidev_dup id: 1 bdf: 00:04.0 dup: 2 xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 xen_map_pcidev id: 1 bdf: ff:03.0 xen_unmap_pcidev id: 1 bdf: 00:17.0 xen_map_pcidev id: 1 bdf: ff:17.0 xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 xen_map_pcidev id: 1 bdf: ff:01.0 xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 xen_map_pcidev_dup id: 1 bdf: ff:03.0 dup: 2 xen_unmap_pcidev_dup id: 1 bdf: 00:04.0 dup: 1 xen_map_pcidev id: 1 bdf: ff:04.0 xen_unmap_pcidev_dup id: 1 bdf: ff:03.0 dup: 1 xen_map_pcidev id: 1 bdf: 01:03.0 xen_unmap_pcidev id: 1 bdf: ff:17.0 xen_map_pcidev id: 1 bdf: 01:17.0 xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 xen_map_pcidev_dup id: 1 bdf: ff:01.0 dup: 2 xen_unmap_pcidev_dup id: 1 bdf: ff:01.0 dup: 1 xen_map_pcidev id: 1 bdf: 02:01.0 xen_unmap_pcidev id: 1 bdf: ff:01.0 xen_map_pcidev id: 1 bdf: 03:01.0 xen_unmap_pcidev id: 1 bdf: ff:03.0 xen_map_pcidev id: 1 bdf: 03:03.0 xen_unmap_pcidev id: 1 bdf: ff:04.0 xen_map_pcidev id: 1 bdf: 03:04.0 xen_unmap_pcidev id: 1 bdf: 00:04.0 xen_unmap_pcidev id: 1 bdf: 00:05.0 xen_unmap_pcidev id: 1 bdf: 01:03.0 xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 xen_unmap_pcidev id: 1 bdf: 01:17.0 xen_map_pcidev id: 1 bdf: 00:17.0 xen_unmap_pcidev id: 1 bdf: 03:01.0 xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 xen_unmap_pcidev id: 1 bdf: 03:03.0 xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 xen_unmap_pcidev id: 1 bdf: 03:04.0 xen_map_pcidev id: 1 bdf: 00:04.0 xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 xen_map_pcidev id: 1 bdf: 01:03.0 xen_unmap_pcidev id: 1 bdf: 00:17.0 xen_map_pcidev id: 1 bdf: 01:17.0 xen_unmap_pcidev id: 1 bdf: 02:01.0 xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 xen_map_pcidev id: 1 bdf: 02:01.0 xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 xen_map_pcidev id: 1 bdf: 03:01.0 xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 xen_map_pcidev id: 1 bdf: 03:03.0 xen_unmap_pcidev id: 1 bdf: 00:04.0 xen_map_pcidev id: 1 bdf: 03:04.0 Signed-off-by: Don Slutz <dslutz@verizon.com> CC: Don Slutz <don.slutz@gmail.com> --- include/hw/xen/xen_common.h | 53 +++++++++++++++++++++++++++++++++++---------- trace-events | 6 +++-- xen-hvm.c | 15 ++++++++----- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h index 6579b78..260ee58 100644 --- a/include/hw/xen/xen_common.h +++ b/include/hw/xen/xen_common.h @@ -223,12 +223,14 @@ static inline void xen_unmap_io_section(XenXC xc, domid_t dom, static inline void xen_map_pcidev(XenXC xc, domid_t dom, ioservid_t ioservid, + GHashTable *pci_bdf, PCIDevice *pci_dev) { } static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, ioservid_t ioservid, + GHashTable *pci_bdf, PCIDevice *pci_dev, uint8_t oldbus) { @@ -346,27 +348,54 @@ static inline void xen_unmap_io_section(XenXC xc, domid_t dom, static inline void xen_map_pcidev(XenXC xc, domid_t dom, ioservid_t ioservid, + GHashTable *pci_bdf, PCIDevice *pci_dev) { - trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); - xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, - 0, pci_bus_num(pci_dev->bus), - PCI_SLOT(pci_dev->devfn), - PCI_FUNC(pci_dev->devfn)); + gpointer bdf_key = GINT_TO_POINTER((pci_bus_num(pci_dev->bus) << 8) | + pci_dev->devfn); + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, bdf_key)); + + if (!bdf_cnt) { + trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), + PCI_SLOT(pci_dev->devfn), + PCI_FUNC(pci_dev->devfn)); + g_hash_table_insert(pci_bdf, bdf_key, GINT_TO_POINTER(1)); + xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, + 0, pci_bus_num(pci_dev->bus), + PCI_SLOT(pci_dev->devfn), + PCI_FUNC(pci_dev->devfn)); + } else { + trace_xen_map_pcidev_dup(ioservid, pci_bus_num(pci_dev->bus), + PCI_SLOT(pci_dev->devfn), + PCI_FUNC(pci_dev->devfn), + bdf_cnt + 1); + g_hash_table_replace(pci_bdf, bdf_key, GINT_TO_POINTER(bdf_cnt + 1)); + } } static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, ioservid_t ioservid, + GHashTable *pci_bdf, PCIDevice *pci_dev, uint8_t oldbus) { - trace_xen_unmap_pcidev(ioservid, oldbus, - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); - xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, - 0, oldbus, - PCI_SLOT(pci_dev->devfn), - PCI_FUNC(pci_dev->devfn)); + gpointer bdf_key = GINT_TO_POINTER((oldbus << 8) | pci_dev->devfn); + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, bdf_key)); + + if (bdf_cnt == 1) { + trace_xen_unmap_pcidev(ioservid, oldbus, + PCI_SLOT(pci_dev->devfn), + PCI_FUNC(pci_dev->devfn)); + g_hash_table_remove(pci_bdf, bdf_key); + xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, + 0, oldbus, + PCI_SLOT(pci_dev->devfn), + PCI_FUNC(pci_dev->devfn)); + } else { + trace_xen_unmap_pcidev_dup(ioservid, oldbus, PCI_SLOT(pci_dev->devfn), + PCI_FUNC(pci_dev->devfn), bdf_cnt - 1); + g_hash_table_replace(pci_bdf, bdf_key, GINT_TO_POINTER(bdf_cnt - 1)); + } } static inline int xen_create_ioreq_server(XenXC xc, domid_t dom, diff --git a/trace-events b/trace-events index a589650..58deeaf 100644 --- a/trace-events +++ b/trace-events @@ -930,8 +930,10 @@ xen_map_mmio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 -xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u bdf: %02x.%02x.%02x" -xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u bdf: %02x.%02x.%02x" +xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u bdf: %02x:%02x.%x" +xen_map_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func, int dup) "id: %u bdf: %02x:%02x.%x dup: %d" +xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u bdf: %02x:%02x.%x" +xen_unmap_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func, int dup) "id: %u bdf: %02x:%02x.%x dup: %d" # xen-mapcache.c xen_map_cache(uint64_t phys_addr) "want %#"PRIx64 diff --git a/xen-hvm.c b/xen-hvm.c index 7b6d8f1..f041909 100644 --- a/xen-hvm.c +++ b/xen-hvm.c @@ -123,6 +123,7 @@ typedef struct XenIOState { MemoryListener memory_listener; MemoryListener io_listener; DeviceListener device_listener; + GHashTable *pci_bdf; QLIST_HEAD(, XenPhysmap) physmap; hwaddr free_phys_offset; const XenPhysmap *log_for_dirtybit; @@ -578,7 +579,8 @@ static void xen_device_realize(DeviceListener *listener, if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { PCIDevice *pci_dev = PCI_DEVICE(dev); - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, + pci_dev); } } @@ -590,8 +592,8 @@ static void xen_device_unrealize(DeviceListener *listener, if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { PCIDevice *pci_dev = PCI_DEVICE(dev); - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, - pci_bus_num(pci_dev->bus)); + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, + pci_dev, pci_bus_num(pci_dev->bus)); } } @@ -600,8 +602,10 @@ static void xen_device_change_pci_bus_num(DeviceListener *listener, { XenIOState *state = container_of(listener, XenIOState, device_listener); - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, oldbus); - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, + pci_dev, oldbus); + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, + pci_dev); } static void xen_sync_dirty_bitmap(XenIOState *state, @@ -1318,6 +1322,7 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size, memory_listener_register(&state->io_listener, &address_space_io); state->device_listener = xen_device_listener; + state->pci_bdf = g_hash_table_new(NULL, NULL); device_listener_register(&state->device_listener); /* Initialize backend core & drivers */ -- 1.8.4 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server Don Slutz @ 2015-06-09 9:05 ` Paul Durrant 2015-06-09 13:51 ` Don Slutz 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 9:05 UTC (permalink / raw) To: Don Slutz, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 08 June 2015 22:19 > To: qemu-devel@nongnu.org; xen-devel@lists.xen.org > Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz > Subject: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server > > The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 added usage of > xc_hvm_map_pcidev_from_ioreq_server() and > xc_hvm_unmap_pcidev_from_ioreq_server(). These routines only > correctly work if the PCI BDF of a PCI device is unique. The 3 > parts (bus, device, and function) of a PCI BDF are not required to > be unique. > > The PCI BDF is accessed in QEMU: > bus pci_bus_num() > device PCI_SLOT() > function PCI_FUNC() > > Add a hash table to track the current set of PCI BDFs and only call > on xc_hvm_map_pcidev_from_ioreq_server() and > xc_hvm_unmap_pcidev_from_ioreq_server() when needed. > This seems to imply that the devices are being realized multiple times without being unrealized? Is that really the case? Paul > Also fix the PCI BDF default stderr trace output: bus is seperated > by ':', and function is only 1 digit. > > Here is an example of stderr trace output: > > xen_map_pcidev id: 1 bdf: 00:00.0 > xen_map_pcidev id: 1 bdf: 00:01.0 > xen_map_pcidev id: 1 bdf: 00:01.1 > xen_map_pcidev id: 1 bdf: 00:01.3 > xen_map_pcidev id: 1 bdf: 00:02.0 > xen_map_pcidev id: 1 bdf: 00:03.0 > xen_map_pcidev id: 1 bdf: 00:04.0 > xen_map_pcidev id: 1 bdf: 00:05.0 > xen_map_pcidev id: 1 bdf: 00:11.0 > xen_map_pcidev id: 1 bdf: 00:15.0 > xen_map_pcidev id: 1 bdf: 00:16.0 > xen_map_pcidev id: 1 bdf: 00:17.0 > xen_map_pcidev id: 1 bdf: 00:18.0 > xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 > xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 > xen_map_pcidev_dup id: 1 bdf: 00:04.0 dup: 2 > xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > xen_map_pcidev id: 1 bdf: ff:03.0 > xen_unmap_pcidev id: 1 bdf: 00:17.0 > xen_map_pcidev id: 1 bdf: ff:17.0 > xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > xen_map_pcidev id: 1 bdf: ff:01.0 > xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 > xen_map_pcidev_dup id: 1 bdf: ff:03.0 dup: 2 > xen_unmap_pcidev_dup id: 1 bdf: 00:04.0 dup: 1 > xen_map_pcidev id: 1 bdf: ff:04.0 > xen_unmap_pcidev_dup id: 1 bdf: ff:03.0 dup: 1 > xen_map_pcidev id: 1 bdf: 01:03.0 > xen_unmap_pcidev id: 1 bdf: ff:17.0 > xen_map_pcidev id: 1 bdf: 01:17.0 > xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 > xen_map_pcidev_dup id: 1 bdf: ff:01.0 dup: 2 > xen_unmap_pcidev_dup id: 1 bdf: ff:01.0 dup: 1 > xen_map_pcidev id: 1 bdf: 02:01.0 > xen_unmap_pcidev id: 1 bdf: ff:01.0 > xen_map_pcidev id: 1 bdf: 03:01.0 > xen_unmap_pcidev id: 1 bdf: ff:03.0 > xen_map_pcidev id: 1 bdf: 03:03.0 > xen_unmap_pcidev id: 1 bdf: ff:04.0 > xen_map_pcidev id: 1 bdf: 03:04.0 > xen_unmap_pcidev id: 1 bdf: 00:04.0 > xen_unmap_pcidev id: 1 bdf: 00:05.0 > xen_unmap_pcidev id: 1 bdf: 01:03.0 > xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > xen_unmap_pcidev id: 1 bdf: 01:17.0 > xen_map_pcidev id: 1 bdf: 00:17.0 > xen_unmap_pcidev id: 1 bdf: 03:01.0 > xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > xen_unmap_pcidev id: 1 bdf: 03:03.0 > xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 > xen_unmap_pcidev id: 1 bdf: 03:04.0 > xen_map_pcidev id: 1 bdf: 00:04.0 > xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > xen_map_pcidev id: 1 bdf: 01:03.0 > xen_unmap_pcidev id: 1 bdf: 00:17.0 > xen_map_pcidev id: 1 bdf: 01:17.0 > xen_unmap_pcidev id: 1 bdf: 02:01.0 > xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 > xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > xen_map_pcidev id: 1 bdf: 02:01.0 > xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 > xen_map_pcidev id: 1 bdf: 03:01.0 > xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 > xen_map_pcidev id: 1 bdf: 03:03.0 > xen_unmap_pcidev id: 1 bdf: 00:04.0 > xen_map_pcidev id: 1 bdf: 03:04.0 > > Signed-off-by: Don Slutz <dslutz@verizon.com> > CC: Don Slutz <don.slutz@gmail.com> > --- > include/hw/xen/xen_common.h | 53 > +++++++++++++++++++++++++++++++++++---------- > trace-events | 6 +++-- > xen-hvm.c | 15 ++++++++----- > 3 files changed, 55 insertions(+), 19 deletions(-) > > diff --git a/include/hw/xen/xen_common.h > b/include/hw/xen/xen_common.h > index 6579b78..260ee58 100644 > --- a/include/hw/xen/xen_common.h > +++ b/include/hw/xen/xen_common.h > @@ -223,12 +223,14 @@ static inline void xen_unmap_io_section(XenXC xc, > domid_t dom, > > static inline void xen_map_pcidev(XenXC xc, domid_t dom, > ioservid_t ioservid, > + GHashTable *pci_bdf, > PCIDevice *pci_dev) > { > } > > static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, > ioservid_t ioservid, > + GHashTable *pci_bdf, > PCIDevice *pci_dev, > uint8_t oldbus) > { > @@ -346,27 +348,54 @@ static inline void xen_unmap_io_section(XenXC xc, > domid_t dom, > > static inline void xen_map_pcidev(XenXC xc, domid_t dom, > ioservid_t ioservid, > + GHashTable *pci_bdf, > PCIDevice *pci_dev) > { > - trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), > - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); > - xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, > - 0, pci_bus_num(pci_dev->bus), > - PCI_SLOT(pci_dev->devfn), > - PCI_FUNC(pci_dev->devfn)); > + gpointer bdf_key = GINT_TO_POINTER((pci_bus_num(pci_dev->bus) << > 8) | > + pci_dev->devfn); > + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, > bdf_key)); > + > + if (!bdf_cnt) { > + trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), > + PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn)); > + g_hash_table_insert(pci_bdf, bdf_key, GINT_TO_POINTER(1)); > + xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, > + 0, pci_bus_num(pci_dev->bus), > + PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn)); > + } else { > + trace_xen_map_pcidev_dup(ioservid, pci_bus_num(pci_dev->bus), > + PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn), > + bdf_cnt + 1); > + g_hash_table_replace(pci_bdf, bdf_key, GINT_TO_POINTER(bdf_cnt + > 1)); > + } > } > > static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, > ioservid_t ioservid, > + GHashTable *pci_bdf, > PCIDevice *pci_dev, > uint8_t oldbus) > { > - trace_xen_unmap_pcidev(ioservid, oldbus, > - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); > - xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, > - 0, oldbus, > - PCI_SLOT(pci_dev->devfn), > - PCI_FUNC(pci_dev->devfn)); > + gpointer bdf_key = GINT_TO_POINTER((oldbus << 8) | pci_dev->devfn); > + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, > bdf_key)); > + > + if (bdf_cnt == 1) { > + trace_xen_unmap_pcidev(ioservid, oldbus, > + PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn)); > + g_hash_table_remove(pci_bdf, bdf_key); > + xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, > + 0, oldbus, > + PCI_SLOT(pci_dev->devfn), > + PCI_FUNC(pci_dev->devfn)); > + } else { > + trace_xen_unmap_pcidev_dup(ioservid, oldbus, PCI_SLOT(pci_dev- > >devfn), > + PCI_FUNC(pci_dev->devfn), bdf_cnt - 1); > + g_hash_table_replace(pci_bdf, bdf_key, GINT_TO_POINTER(bdf_cnt - > 1)); > + } > } > > static inline int xen_create_ioreq_server(XenXC xc, domid_t dom, > diff --git a/trace-events b/trace-events > index a589650..58deeaf 100644 > --- a/trace-events > +++ b/trace-events > @@ -930,8 +930,10 @@ xen_map_mmio_range(uint32_t id, uint64_t > start_addr, uint64_t end_addr) "id: %u > xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t > end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 > xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) > "id: %u start: %#"PRIx64" end: %#"PRIx64 > xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t > end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 > -xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u > bdf: %02x.%02x.%02x" > -xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: > %u bdf: %02x.%02x.%02x" > +xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u > bdf: %02x:%02x.%x" > +xen_map_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func, > int dup) "id: %u bdf: %02x:%02x.%x dup: %d" > +xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: > %u bdf: %02x:%02x.%x" > +xen_unmap_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func, > int dup) "id: %u bdf: %02x:%02x.%x dup: %d" > > # xen-mapcache.c > xen_map_cache(uint64_t phys_addr) "want %#"PRIx64 > diff --git a/xen-hvm.c b/xen-hvm.c > index 7b6d8f1..f041909 100644 > --- a/xen-hvm.c > +++ b/xen-hvm.c > @@ -123,6 +123,7 @@ typedef struct XenIOState { > MemoryListener memory_listener; > MemoryListener io_listener; > DeviceListener device_listener; > + GHashTable *pci_bdf; > QLIST_HEAD(, XenPhysmap) physmap; > hwaddr free_phys_offset; > const XenPhysmap *log_for_dirtybit; > @@ -578,7 +579,8 @@ static void xen_device_realize(DeviceListener > *listener, > if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > PCIDevice *pci_dev = PCI_DEVICE(dev); > > - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); > + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, > + pci_dev); > } > } > > @@ -590,8 +592,8 @@ static void xen_device_unrealize(DeviceListener > *listener, > if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > PCIDevice *pci_dev = PCI_DEVICE(dev); > > - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, > - pci_bus_num(pci_dev->bus)); > + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- > >pci_bdf, > + pci_dev, pci_bus_num(pci_dev->bus)); > } > } > > @@ -600,8 +602,10 @@ static void > xen_device_change_pci_bus_num(DeviceListener *listener, > { > XenIOState *state = container_of(listener, XenIOState, device_listener); > > - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, > oldbus); > - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); > + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, > + pci_dev, oldbus); > + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, > + pci_dev); > } > > static void xen_sync_dirty_bitmap(XenIOState *state, > @@ -1318,6 +1322,7 @@ int xen_hvm_init(ram_addr_t > *below_4g_mem_size, ram_addr_t *above_4g_mem_size, > memory_listener_register(&state->io_listener, &address_space_io); > > state->device_listener = xen_device_listener; > + state->pci_bdf = g_hash_table_new(NULL, NULL); > device_listener_register(&state->device_listener); > > /* Initialize backend core & drivers */ > -- > 1.8.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server 2015-06-09 9:05 ` Paul Durrant @ 2015-06-09 13:51 ` Don Slutz 2015-06-09 14:03 ` Paul Durrant 0 siblings, 1 reply; 23+ messages in thread From: Don Slutz @ 2015-06-09 13:51 UTC (permalink / raw) To: Paul Durrant, Slutz, Donald Christopher, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin On 06/09/15 05:05, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 08 June 2015 22:19 >> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org >> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz >> Subject: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server >> >> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 added usage of >> xc_hvm_map_pcidev_from_ioreq_server() and >> xc_hvm_unmap_pcidev_from_ioreq_server(). These routines only >> correctly work if the PCI BDF of a PCI device is unique. The 3 >> parts (bus, device, and function) of a PCI BDF are not required to >> be unique. >> >> The PCI BDF is accessed in QEMU: >> bus pci_bus_num() >> device PCI_SLOT() >> function PCI_FUNC() >> >> Add a hash table to track the current set of PCI BDFs and only call >> on xc_hvm_map_pcidev_from_ioreq_server() and >> xc_hvm_unmap_pcidev_from_ioreq_server() when needed. >> > > This seems to imply that the devices are being realized multiple times without being unrealized? Not directly. The devices are not being "realized" in QEMU terms. The PCI to PCI brige is changing state, and the BDF for all PCI devices on the secondary bus changes when the PCI config named PCI_SECONDARY_BUS on the PCI to PCI bridge is changed. This is what patch #2 is all about. > Is that really the case? > That is what it looks like to Xen. But the device listener callbacks are not called. -Don Slutz > Paul > >> Also fix the PCI BDF default stderr trace output: bus is seperated >> by ':', and function is only 1 digit. >> >> Here is an example of stderr trace output: >> >> xen_map_pcidev id: 1 bdf: 00:00.0 >> xen_map_pcidev id: 1 bdf: 00:01.0 >> xen_map_pcidev id: 1 bdf: 00:01.1 >> xen_map_pcidev id: 1 bdf: 00:01.3 >> xen_map_pcidev id: 1 bdf: 00:02.0 >> xen_map_pcidev id: 1 bdf: 00:03.0 >> xen_map_pcidev id: 1 bdf: 00:04.0 >> xen_map_pcidev id: 1 bdf: 00:05.0 >> xen_map_pcidev id: 1 bdf: 00:11.0 >> xen_map_pcidev id: 1 bdf: 00:15.0 >> xen_map_pcidev id: 1 bdf: 00:16.0 >> xen_map_pcidev id: 1 bdf: 00:17.0 >> xen_map_pcidev id: 1 bdf: 00:18.0 >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 >> xen_map_pcidev_dup id: 1 bdf: 00:04.0 dup: 2 >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >> xen_map_pcidev id: 1 bdf: ff:03.0 >> xen_unmap_pcidev id: 1 bdf: 00:17.0 >> xen_map_pcidev id: 1 bdf: ff:17.0 >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >> xen_map_pcidev id: 1 bdf: ff:01.0 >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 >> xen_map_pcidev_dup id: 1 bdf: ff:03.0 dup: 2 >> xen_unmap_pcidev_dup id: 1 bdf: 00:04.0 dup: 1 >> xen_map_pcidev id: 1 bdf: ff:04.0 >> xen_unmap_pcidev_dup id: 1 bdf: ff:03.0 dup: 1 >> xen_map_pcidev id: 1 bdf: 01:03.0 >> xen_unmap_pcidev id: 1 bdf: ff:17.0 >> xen_map_pcidev id: 1 bdf: 01:17.0 >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 >> xen_map_pcidev_dup id: 1 bdf: ff:01.0 dup: 2 >> xen_unmap_pcidev_dup id: 1 bdf: ff:01.0 dup: 1 >> xen_map_pcidev id: 1 bdf: 02:01.0 >> xen_unmap_pcidev id: 1 bdf: ff:01.0 >> xen_map_pcidev id: 1 bdf: 03:01.0 >> xen_unmap_pcidev id: 1 bdf: ff:03.0 >> xen_map_pcidev id: 1 bdf: 03:03.0 >> xen_unmap_pcidev id: 1 bdf: ff:04.0 >> xen_map_pcidev id: 1 bdf: 03:04.0 >> xen_unmap_pcidev id: 1 bdf: 00:04.0 >> xen_unmap_pcidev id: 1 bdf: 00:05.0 >> xen_unmap_pcidev id: 1 bdf: 01:03.0 >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >> xen_unmap_pcidev id: 1 bdf: 01:17.0 >> xen_map_pcidev id: 1 bdf: 00:17.0 >> xen_unmap_pcidev id: 1 bdf: 03:01.0 >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >> xen_unmap_pcidev id: 1 bdf: 03:03.0 >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 >> xen_unmap_pcidev id: 1 bdf: 03:04.0 >> xen_map_pcidev id: 1 bdf: 00:04.0 >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >> xen_map_pcidev id: 1 bdf: 01:03.0 >> xen_unmap_pcidev id: 1 bdf: 00:17.0 >> xen_map_pcidev id: 1 bdf: 01:17.0 >> xen_unmap_pcidev id: 1 bdf: 02:01.0 >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >> xen_map_pcidev id: 1 bdf: 02:01.0 >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 >> xen_map_pcidev id: 1 bdf: 03:01.0 >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 >> xen_map_pcidev id: 1 bdf: 03:03.0 >> xen_unmap_pcidev id: 1 bdf: 00:04.0 >> xen_map_pcidev id: 1 bdf: 03:04.0 >> >> Signed-off-by: Don Slutz <dslutz@verizon.com> >> CC: Don Slutz <don.slutz@gmail.com> >> --- >> include/hw/xen/xen_common.h | 53 >> +++++++++++++++++++++++++++++++++++---------- >> trace-events | 6 +++-- >> xen-hvm.c | 15 ++++++++----- >> 3 files changed, 55 insertions(+), 19 deletions(-) >> >> diff --git a/include/hw/xen/xen_common.h >> b/include/hw/xen/xen_common.h >> index 6579b78..260ee58 100644 >> --- a/include/hw/xen/xen_common.h >> +++ b/include/hw/xen/xen_common.h >> @@ -223,12 +223,14 @@ static inline void xen_unmap_io_section(XenXC xc, >> domid_t dom, >> >> static inline void xen_map_pcidev(XenXC xc, domid_t dom, >> ioservid_t ioservid, >> + GHashTable *pci_bdf, >> PCIDevice *pci_dev) >> { >> } >> >> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, >> ioservid_t ioservid, >> + GHashTable *pci_bdf, >> PCIDevice *pci_dev, >> uint8_t oldbus) >> { >> @@ -346,27 +348,54 @@ static inline void xen_unmap_io_section(XenXC xc, >> domid_t dom, >> >> static inline void xen_map_pcidev(XenXC xc, domid_t dom, >> ioservid_t ioservid, >> + GHashTable *pci_bdf, >> PCIDevice *pci_dev) >> { >> - trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), >> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); >> - xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, >> - 0, pci_bus_num(pci_dev->bus), >> - PCI_SLOT(pci_dev->devfn), >> - PCI_FUNC(pci_dev->devfn)); >> + gpointer bdf_key = GINT_TO_POINTER((pci_bus_num(pci_dev->bus) << >> 8) | >> + pci_dev->devfn); >> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, >> bdf_key)); >> + >> + if (!bdf_cnt) { >> + trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), >> + PCI_SLOT(pci_dev->devfn), >> + PCI_FUNC(pci_dev->devfn)); >> + g_hash_table_insert(pci_bdf, bdf_key, GINT_TO_POINTER(1)); >> + xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, >> + 0, pci_bus_num(pci_dev->bus), >> + PCI_SLOT(pci_dev->devfn), >> + PCI_FUNC(pci_dev->devfn)); >> + } else { >> + trace_xen_map_pcidev_dup(ioservid, pci_bus_num(pci_dev->bus), >> + PCI_SLOT(pci_dev->devfn), >> + PCI_FUNC(pci_dev->devfn), >> + bdf_cnt + 1); >> + g_hash_table_replace(pci_bdf, bdf_key, GINT_TO_POINTER(bdf_cnt + >> 1)); >> + } >> } >> >> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, >> ioservid_t ioservid, >> + GHashTable *pci_bdf, >> PCIDevice *pci_dev, >> uint8_t oldbus) >> { >> - trace_xen_unmap_pcidev(ioservid, oldbus, >> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); >> - xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, >> - 0, oldbus, >> - PCI_SLOT(pci_dev->devfn), >> - PCI_FUNC(pci_dev->devfn)); >> + gpointer bdf_key = GINT_TO_POINTER((oldbus << 8) | pci_dev->devfn); >> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, >> bdf_key)); >> + >> + if (bdf_cnt == 1) { >> + trace_xen_unmap_pcidev(ioservid, oldbus, >> + PCI_SLOT(pci_dev->devfn), >> + PCI_FUNC(pci_dev->devfn)); >> + g_hash_table_remove(pci_bdf, bdf_key); >> + xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, >> + 0, oldbus, >> + PCI_SLOT(pci_dev->devfn), >> + PCI_FUNC(pci_dev->devfn)); >> + } else { >> + trace_xen_unmap_pcidev_dup(ioservid, oldbus, PCI_SLOT(pci_dev- >>> devfn), >> + PCI_FUNC(pci_dev->devfn), bdf_cnt - 1); >> + g_hash_table_replace(pci_bdf, bdf_key, GINT_TO_POINTER(bdf_cnt - >> 1)); >> + } >> } >> >> static inline int xen_create_ioreq_server(XenXC xc, domid_t dom, >> diff --git a/trace-events b/trace-events >> index a589650..58deeaf 100644 >> --- a/trace-events >> +++ b/trace-events >> @@ -930,8 +930,10 @@ xen_map_mmio_range(uint32_t id, uint64_t >> start_addr, uint64_t end_addr) "id: %u >> xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t >> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 >> xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) >> "id: %u start: %#"PRIx64" end: %#"PRIx64 >> xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t >> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 >> -xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u >> bdf: %02x.%02x.%02x" >> -xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: >> %u bdf: %02x.%02x.%02x" >> +xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u >> bdf: %02x:%02x.%x" >> +xen_map_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func, >> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" >> +xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: >> %u bdf: %02x:%02x.%x" >> +xen_unmap_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func, >> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" >> >> # xen-mapcache.c >> xen_map_cache(uint64_t phys_addr) "want %#"PRIx64 >> diff --git a/xen-hvm.c b/xen-hvm.c >> index 7b6d8f1..f041909 100644 >> --- a/xen-hvm.c >> +++ b/xen-hvm.c >> @@ -123,6 +123,7 @@ typedef struct XenIOState { >> MemoryListener memory_listener; >> MemoryListener io_listener; >> DeviceListener device_listener; >> + GHashTable *pci_bdf; >> QLIST_HEAD(, XenPhysmap) physmap; >> hwaddr free_phys_offset; >> const XenPhysmap *log_for_dirtybit; >> @@ -578,7 +579,8 @@ static void xen_device_realize(DeviceListener >> *listener, >> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >> PCIDevice *pci_dev = PCI_DEVICE(dev); >> >> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); >> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, >> + pci_dev); >> } >> } >> >> @@ -590,8 +592,8 @@ static void xen_device_unrealize(DeviceListener >> *listener, >> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >> PCIDevice *pci_dev = PCI_DEVICE(dev); >> >> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, >> - pci_bus_num(pci_dev->bus)); >> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- >>> pci_bdf, >> + pci_dev, pci_bus_num(pci_dev->bus)); >> } >> } >> >> @@ -600,8 +602,10 @@ static void >> xen_device_change_pci_bus_num(DeviceListener *listener, >> { >> XenIOState *state = container_of(listener, XenIOState, device_listener); >> >> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, >> oldbus); >> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); >> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, >> + pci_dev, oldbus); >> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state->pci_bdf, >> + pci_dev); >> } >> >> static void xen_sync_dirty_bitmap(XenIOState *state, >> @@ -1318,6 +1322,7 @@ int xen_hvm_init(ram_addr_t >> *below_4g_mem_size, ram_addr_t *above_4g_mem_size, >> memory_listener_register(&state->io_listener, &address_space_io); >> >> state->device_listener = xen_device_listener; >> + state->pci_bdf = g_hash_table_new(NULL, NULL); >> device_listener_register(&state->device_listener); >> >> /* Initialize backend core & drivers */ >> -- >> 1.8.4 > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server 2015-06-09 13:51 ` Don Slutz @ 2015-06-09 14:03 ` Paul Durrant 2015-06-09 14:28 ` Don Slutz 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 14:03 UTC (permalink / raw) To: Don Slutz, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 09 June 2015 14:51 > To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; xen- > devel@lists.xen.org > Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz > Subject: Re: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server > > On 06/09/15 05:05, Paul Durrant wrote: > >> -----Original Message----- > >> From: Don Slutz [mailto:dslutz@verizon.com] > >> Sent: 08 June 2015 22:19 > >> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org > >> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz > >> Subject: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server > >> > >> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 added usage of > >> xc_hvm_map_pcidev_from_ioreq_server() and > >> xc_hvm_unmap_pcidev_from_ioreq_server(). These routines only > >> correctly work if the PCI BDF of a PCI device is unique. The 3 > >> parts (bus, device, and function) of a PCI BDF are not required to > >> be unique. > >> > >> The PCI BDF is accessed in QEMU: > >> bus pci_bus_num() > >> device PCI_SLOT() > >> function PCI_FUNC() > >> > >> Add a hash table to track the current set of PCI BDFs and only call > >> on xc_hvm_map_pcidev_from_ioreq_server() and > >> xc_hvm_unmap_pcidev_from_ioreq_server() when needed. > >> > > > > This seems to imply that the devices are being realized multiple times > without being unrealized? > > Not directly. The devices are not being "realized" in QEMU terms. The > PCI to PCI brige is changing state, and the BDF for all PCI devices on > the secondary bus changes when the PCI config named > PCI_SECONDARY_BUS on > the PCI to PCI bridge is changed. This is what patch #2 is all about. > > > Is that really the case? > > > > That is what it looks like to Xen. But the device listener callbacks > are not called. Then what's calling xc_hvm_map_pcidev_from_ioreq_server()? I don't understand why you are having to introduce this to avoid duplicate calls? Paul > > -Don Slutz > > > Paul > > > >> Also fix the PCI BDF default stderr trace output: bus is seperated > >> by ':', and function is only 1 digit. > >> > >> Here is an example of stderr trace output: > >> > >> xen_map_pcidev id: 1 bdf: 00:00.0 > >> xen_map_pcidev id: 1 bdf: 00:01.0 > >> xen_map_pcidev id: 1 bdf: 00:01.1 > >> xen_map_pcidev id: 1 bdf: 00:01.3 > >> xen_map_pcidev id: 1 bdf: 00:02.0 > >> xen_map_pcidev id: 1 bdf: 00:03.0 > >> xen_map_pcidev id: 1 bdf: 00:04.0 > >> xen_map_pcidev id: 1 bdf: 00:05.0 > >> xen_map_pcidev id: 1 bdf: 00:11.0 > >> xen_map_pcidev id: 1 bdf: 00:15.0 > >> xen_map_pcidev id: 1 bdf: 00:16.0 > >> xen_map_pcidev id: 1 bdf: 00:17.0 > >> xen_map_pcidev id: 1 bdf: 00:18.0 > >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 > >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 > >> xen_map_pcidev_dup id: 1 bdf: 00:04.0 dup: 2 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >> xen_map_pcidev id: 1 bdf: ff:03.0 > >> xen_unmap_pcidev id: 1 bdf: 00:17.0 > >> xen_map_pcidev id: 1 bdf: ff:17.0 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >> xen_map_pcidev id: 1 bdf: ff:01.0 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 > >> xen_map_pcidev_dup id: 1 bdf: ff:03.0 dup: 2 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:04.0 dup: 1 > >> xen_map_pcidev id: 1 bdf: ff:04.0 > >> xen_unmap_pcidev_dup id: 1 bdf: ff:03.0 dup: 1 > >> xen_map_pcidev id: 1 bdf: 01:03.0 > >> xen_unmap_pcidev id: 1 bdf: ff:17.0 > >> xen_map_pcidev id: 1 bdf: 01:17.0 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 > >> xen_map_pcidev_dup id: 1 bdf: ff:01.0 dup: 2 > >> xen_unmap_pcidev_dup id: 1 bdf: ff:01.0 dup: 1 > >> xen_map_pcidev id: 1 bdf: 02:01.0 > >> xen_unmap_pcidev id: 1 bdf: ff:01.0 > >> xen_map_pcidev id: 1 bdf: 03:01.0 > >> xen_unmap_pcidev id: 1 bdf: ff:03.0 > >> xen_map_pcidev id: 1 bdf: 03:03.0 > >> xen_unmap_pcidev id: 1 bdf: ff:04.0 > >> xen_map_pcidev id: 1 bdf: 03:04.0 > >> xen_unmap_pcidev id: 1 bdf: 00:04.0 > >> xen_unmap_pcidev id: 1 bdf: 00:05.0 > >> xen_unmap_pcidev id: 1 bdf: 01:03.0 > >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >> xen_unmap_pcidev id: 1 bdf: 01:17.0 > >> xen_map_pcidev id: 1 bdf: 00:17.0 > >> xen_unmap_pcidev id: 1 bdf: 03:01.0 > >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >> xen_unmap_pcidev id: 1 bdf: 03:03.0 > >> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 > >> xen_unmap_pcidev id: 1 bdf: 03:04.0 > >> xen_map_pcidev id: 1 bdf: 00:04.0 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >> xen_map_pcidev id: 1 bdf: 01:03.0 > >> xen_unmap_pcidev id: 1 bdf: 00:17.0 > >> xen_map_pcidev id: 1 bdf: 01:17.0 > >> xen_unmap_pcidev id: 1 bdf: 02:01.0 > >> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >> xen_map_pcidev id: 1 bdf: 02:01.0 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 > >> xen_map_pcidev id: 1 bdf: 03:01.0 > >> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 > >> xen_map_pcidev id: 1 bdf: 03:03.0 > >> xen_unmap_pcidev id: 1 bdf: 00:04.0 > >> xen_map_pcidev id: 1 bdf: 03:04.0 > >> > >> Signed-off-by: Don Slutz <dslutz@verizon.com> > >> CC: Don Slutz <don.slutz@gmail.com> > >> --- > >> include/hw/xen/xen_common.h | 53 > >> +++++++++++++++++++++++++++++++++++---------- > >> trace-events | 6 +++-- > >> xen-hvm.c | 15 ++++++++----- > >> 3 files changed, 55 insertions(+), 19 deletions(-) > >> > >> diff --git a/include/hw/xen/xen_common.h > >> b/include/hw/xen/xen_common.h > >> index 6579b78..260ee58 100644 > >> --- a/include/hw/xen/xen_common.h > >> +++ b/include/hw/xen/xen_common.h > >> @@ -223,12 +223,14 @@ static inline void xen_unmap_io_section(XenXC > xc, > >> domid_t dom, > >> > >> static inline void xen_map_pcidev(XenXC xc, domid_t dom, > >> ioservid_t ioservid, > >> + GHashTable *pci_bdf, > >> PCIDevice *pci_dev) > >> { > >> } > >> > >> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, > >> ioservid_t ioservid, > >> + GHashTable *pci_bdf, > >> PCIDevice *pci_dev, > >> uint8_t oldbus) > >> { > >> @@ -346,27 +348,54 @@ static inline void xen_unmap_io_section(XenXC > xc, > >> domid_t dom, > >> > >> static inline void xen_map_pcidev(XenXC xc, domid_t dom, > >> ioservid_t ioservid, > >> + GHashTable *pci_bdf, > >> PCIDevice *pci_dev) > >> { > >> - trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), > >> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); > >> - xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, > >> - 0, pci_bus_num(pci_dev->bus), > >> - PCI_SLOT(pci_dev->devfn), > >> - PCI_FUNC(pci_dev->devfn)); > >> + gpointer bdf_key = GINT_TO_POINTER((pci_bus_num(pci_dev->bus) > << > >> 8) | > >> + pci_dev->devfn); > >> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, > >> bdf_key)); > >> + > >> + if (!bdf_cnt) { > >> + trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), > >> + PCI_SLOT(pci_dev->devfn), > >> + PCI_FUNC(pci_dev->devfn)); > >> + g_hash_table_insert(pci_bdf, bdf_key, GINT_TO_POINTER(1)); > >> + xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, > >> + 0, pci_bus_num(pci_dev->bus), > >> + PCI_SLOT(pci_dev->devfn), > >> + PCI_FUNC(pci_dev->devfn)); > >> + } else { > >> + trace_xen_map_pcidev_dup(ioservid, pci_bus_num(pci_dev->bus), > >> + PCI_SLOT(pci_dev->devfn), > >> + PCI_FUNC(pci_dev->devfn), > >> + bdf_cnt + 1); > >> + g_hash_table_replace(pci_bdf, bdf_key, > GINT_TO_POINTER(bdf_cnt + > >> 1)); > >> + } > >> } > >> > >> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, > >> ioservid_t ioservid, > >> + GHashTable *pci_bdf, > >> PCIDevice *pci_dev, > >> uint8_t oldbus) > >> { > >> - trace_xen_unmap_pcidev(ioservid, oldbus, > >> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); > >> - xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, > >> - 0, oldbus, > >> - PCI_SLOT(pci_dev->devfn), > >> - PCI_FUNC(pci_dev->devfn)); > >> + gpointer bdf_key = GINT_TO_POINTER((oldbus << 8) | pci_dev- > >devfn); > >> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, > >> bdf_key)); > >> + > >> + if (bdf_cnt == 1) { > >> + trace_xen_unmap_pcidev(ioservid, oldbus, > >> + PCI_SLOT(pci_dev->devfn), > >> + PCI_FUNC(pci_dev->devfn)); > >> + g_hash_table_remove(pci_bdf, bdf_key); > >> + xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, > >> + 0, oldbus, > >> + PCI_SLOT(pci_dev->devfn), > >> + PCI_FUNC(pci_dev->devfn)); > >> + } else { > >> + trace_xen_unmap_pcidev_dup(ioservid, oldbus, PCI_SLOT(pci_dev- > >>> devfn), > >> + PCI_FUNC(pci_dev->devfn), bdf_cnt - 1); > >> + g_hash_table_replace(pci_bdf, bdf_key, > GINT_TO_POINTER(bdf_cnt - > >> 1)); > >> + } > >> } > >> > >> static inline int xen_create_ioreq_server(XenXC xc, domid_t dom, > >> diff --git a/trace-events b/trace-events > >> index a589650..58deeaf 100644 > >> --- a/trace-events > >> +++ b/trace-events > >> @@ -930,8 +930,10 @@ xen_map_mmio_range(uint32_t id, uint64_t > >> start_addr, uint64_t end_addr) "id: %u > >> xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t > >> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 > >> xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t > end_addr) > >> "id: %u start: %#"PRIx64" end: %#"PRIx64 > >> xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t > >> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 > >> -xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: > %u > >> bdf: %02x.%02x.%02x" > >> -xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) > "id: > >> %u bdf: %02x.%02x.%02x" > >> +xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: > %u > >> bdf: %02x:%02x.%x" > >> +xen_map_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t > func, > >> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" > >> +xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) > "id: > >> %u bdf: %02x:%02x.%x" > >> +xen_unmap_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t > func, > >> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" > >> > >> # xen-mapcache.c > >> xen_map_cache(uint64_t phys_addr) "want %#"PRIx64 > >> diff --git a/xen-hvm.c b/xen-hvm.c > >> index 7b6d8f1..f041909 100644 > >> --- a/xen-hvm.c > >> +++ b/xen-hvm.c > >> @@ -123,6 +123,7 @@ typedef struct XenIOState { > >> MemoryListener memory_listener; > >> MemoryListener io_listener; > >> DeviceListener device_listener; > >> + GHashTable *pci_bdf; > >> QLIST_HEAD(, XenPhysmap) physmap; > >> hwaddr free_phys_offset; > >> const XenPhysmap *log_for_dirtybit; > >> @@ -578,7 +579,8 @@ static void xen_device_realize(DeviceListener > >> *listener, > >> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > >> PCIDevice *pci_dev = PCI_DEVICE(dev); > >> > >> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); > >> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- > >pci_bdf, > >> + pci_dev); > >> } > >> } > >> > >> @@ -590,8 +592,8 @@ static void xen_device_unrealize(DeviceListener > >> *listener, > >> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > >> PCIDevice *pci_dev = PCI_DEVICE(dev); > >> > >> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, > >> - pci_bus_num(pci_dev->bus)); > >> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- > >>> pci_bdf, > >> + pci_dev, pci_bus_num(pci_dev->bus)); > >> } > >> } > >> > >> @@ -600,8 +602,10 @@ static void > >> xen_device_change_pci_bus_num(DeviceListener *listener, > >> { > >> XenIOState *state = container_of(listener, XenIOState, > device_listener); > >> > >> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, > >> oldbus); > >> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); > >> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- > >pci_bdf, > >> + pci_dev, oldbus); > >> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- > >pci_bdf, > >> + pci_dev); > >> } > >> > >> static void xen_sync_dirty_bitmap(XenIOState *state, > >> @@ -1318,6 +1322,7 @@ int xen_hvm_init(ram_addr_t > >> *below_4g_mem_size, ram_addr_t *above_4g_mem_size, > >> memory_listener_register(&state->io_listener, &address_space_io); > >> > >> state->device_listener = xen_device_listener; > >> + state->pci_bdf = g_hash_table_new(NULL, NULL); > >> device_listener_register(&state->device_listener); > >> > >> /* Initialize backend core & drivers */ > >> -- > >> 1.8.4 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server 2015-06-09 14:03 ` Paul Durrant @ 2015-06-09 14:28 ` Don Slutz 2015-06-09 15:11 ` Paul Durrant 0 siblings, 1 reply; 23+ messages in thread From: Don Slutz @ 2015-06-09 14:28 UTC (permalink / raw) To: Paul Durrant, Slutz, Donald Christopher, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin On 06/09/15 10:03, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 09 June 2015 14:51 >> To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; xen- >> devel@lists.xen.org >> Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz >> Subject: Re: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server >> >> On 06/09/15 05:05, Paul Durrant wrote: >>>> -----Original Message----- >>>> From: Don Slutz [mailto:dslutz@verizon.com] >>>> Sent: 08 June 2015 22:19 >>>> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org >>>> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don Slutz >>>> Subject: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server >>>> >>>> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 added usage of >>>> xc_hvm_map_pcidev_from_ioreq_server() and >>>> xc_hvm_unmap_pcidev_from_ioreq_server(). These routines only >>>> correctly work if the PCI BDF of a PCI device is unique. The 3 >>>> parts (bus, device, and function) of a PCI BDF are not required to >>>> be unique. >>>> >>>> The PCI BDF is accessed in QEMU: >>>> bus pci_bus_num() >>>> device PCI_SLOT() >>>> function PCI_FUNC() >>>> >>>> Add a hash table to track the current set of PCI BDFs and only call >>>> on xc_hvm_map_pcidev_from_ioreq_server() and >>>> xc_hvm_unmap_pcidev_from_ioreq_server() when needed. >>>> >>> >>> This seems to imply that the devices are being realized multiple times >> without being unrealized? >> >> Not directly. The devices are not being "realized" in QEMU terms. The >> PCI to PCI brige is changing state, and the BDF for all PCI devices on >> the secondary bus changes when the PCI config named >> PCI_SECONDARY_BUS on >> the PCI to PCI bridge is changed. This is what patch #2 is all about. >> >>> Is that really the case? >>> >> >> That is what it looks like to Xen. But the device listener callbacks >> are not called. > > Then what's calling xc_hvm_map_pcidev_from_ioreq_server()? Currently (without these patches) the calls on xc_hvm_map_pcidev_from_ioreq_server() are the ones you added. These are from QEMU's device realize. However since on PCI reset (aka start up) the seconary bus number is zero. So the call on pci_bus_num(pci_dev->bus) will return zero in that case. So using the subset trace output (copied from below) and adjusted to be the old way: xen_map_pcidev id: 1 bdf: 00.01.00 xen_map_pcidev id: 1 bdf: 00.01.00 xen_map_pcidev id: 1 bdf: 00.01.00 Is part of what you see. It may help to have the Xen config that matters: device_model_args_hvm = [ "-monitor", "pty", "-boot", "menu=on", "-device", "pci-bridge,chassis_nr=1,msi=on,id=pciBridge1.0,addr=0x11.0", "-device", "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0", "-device", "pci-bridge,chassis_nr=3,msi=on,id=pciBridge6.0,multifunction=on,addr=0x16.0", "-device", "pci-bridge,chassis_nr=4,msi=on,id=pciBridge7.0,multifunction=on,bus=pciBridge1.0,addr=0x17.0", "-device", "pci-bridge,chassis_nr=5,msi=on,id=pciBridge8.0,multifunction=on,addr=0x18.0", "-device", "pvscsi,id=scsi0,bus=pciBridge5.0,addr=0x1.0x0", "-drive", "if=none,id=disk0-0,format=raw,file=/dev/etherd/e500.1", "-device", "scsi-disk,vendor=VMware,ver=1.0,product=Virtual disk,bus=scsi0.0,scsi-id=0,drive=disk0-0", "-device", "pvscsi,id=sas1,bus=pciBridge7.0,addr=0x1.0x0", "-drive", "if=none,id=disk1-1,format=raw,file=/dev/etherd/e500.2", "-device", "scsi-disk,vendor=VMware,ver=1.0,product=Virtual disk,bus=sas1.0,scsi-id=1,drive=disk1-1", "-device", "pvscsi,id=scsi2,bus=pciBridge1.0,addr=0x3.0x0", "-drive", "if=none,id=disk2-2,format=raw,file=/dev/etherd/e500.3", "-device", "scsi-disk,vendor=VMware,ver=1.0,product=Virtual disk,bus=scsi2.0,scsi-id=2,drive=disk2-2", "-device", "e1000,id=nic2,netdev=net2,mac=00:0c:29:86:44:b4,bus=pciBridge5.0,addr=0x3.0x0", "-netdev", "type=tap,id=net2,ifname=vif.2-emu,script=/etc/qemu-ifup,downscript=no", "-device", "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,addr=0x4.0x0", "-netdev", "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu-ifup,downscript=no", ] If you want to try this, you would also need a bootable disk listed in disk= > I don't understand why you are having to introduce this to avoid > duplicate calls? > That is because in general a PCI device's BDF is not fixed. The only fixed ones are the ones on the root (host bus). It might help to look at the code for pci_bus_num(): int pci_bus_num(PCIBus *s) { if (pci_bus_is_root(s)) return 0; /* pci host bridge */ return s->parent_dev->config[PCI_SECONDARY_BUS]; } So the bus number this routine returns depends on what the guest has set in the PCI config space of the PCI to PCI bridge. The starting (and reset) value is 0. So for the config above at QEMU device realized time there are 3 PCI devices (PIIX4, 2 pvscsi disk controllers). -Don Slutz > Paul > >> >> -Don Slutz >> >>> Paul >>> >>>> Also fix the PCI BDF default stderr trace output: bus is seperated >>>> by ':', and function is only 1 digit. >>>> >>>> Here is an example of stderr trace output: >>>> >>>> xen_map_pcidev id: 1 bdf: 00:00.0 >>>> xen_map_pcidev id: 1 bdf: 00:01.0 >>>> xen_map_pcidev id: 1 bdf: 00:01.1 >>>> xen_map_pcidev id: 1 bdf: 00:01.3 >>>> xen_map_pcidev id: 1 bdf: 00:02.0 >>>> xen_map_pcidev id: 1 bdf: 00:03.0 >>>> xen_map_pcidev id: 1 bdf: 00:04.0 >>>> xen_map_pcidev id: 1 bdf: 00:05.0 >>>> xen_map_pcidev id: 1 bdf: 00:11.0 >>>> xen_map_pcidev id: 1 bdf: 00:15.0 >>>> xen_map_pcidev id: 1 bdf: 00:16.0 >>>> xen_map_pcidev id: 1 bdf: 00:17.0 >>>> xen_map_pcidev id: 1 bdf: 00:18.0 >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 >>>> xen_map_pcidev_dup id: 1 bdf: 00:04.0 dup: 2 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>> xen_map_pcidev id: 1 bdf: ff:03.0 >>>> xen_unmap_pcidev id: 1 bdf: 00:17.0 >>>> xen_map_pcidev id: 1 bdf: ff:17.0 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>> xen_map_pcidev id: 1 bdf: ff:01.0 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 >>>> xen_map_pcidev_dup id: 1 bdf: ff:03.0 dup: 2 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:04.0 dup: 1 >>>> xen_map_pcidev id: 1 bdf: ff:04.0 >>>> xen_unmap_pcidev_dup id: 1 bdf: ff:03.0 dup: 1 >>>> xen_map_pcidev id: 1 bdf: 01:03.0 >>>> xen_unmap_pcidev id: 1 bdf: ff:17.0 >>>> xen_map_pcidev id: 1 bdf: 01:17.0 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 >>>> xen_map_pcidev_dup id: 1 bdf: ff:01.0 dup: 2 >>>> xen_unmap_pcidev_dup id: 1 bdf: ff:01.0 dup: 1 >>>> xen_map_pcidev id: 1 bdf: 02:01.0 >>>> xen_unmap_pcidev id: 1 bdf: ff:01.0 >>>> xen_map_pcidev id: 1 bdf: 03:01.0 >>>> xen_unmap_pcidev id: 1 bdf: ff:03.0 >>>> xen_map_pcidev id: 1 bdf: 03:03.0 >>>> xen_unmap_pcidev id: 1 bdf: ff:04.0 >>>> xen_map_pcidev id: 1 bdf: 03:04.0 >>>> xen_unmap_pcidev id: 1 bdf: 00:04.0 >>>> xen_unmap_pcidev id: 1 bdf: 00:05.0 >>>> xen_unmap_pcidev id: 1 bdf: 01:03.0 >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>> xen_unmap_pcidev id: 1 bdf: 01:17.0 >>>> xen_map_pcidev id: 1 bdf: 00:17.0 >>>> xen_unmap_pcidev id: 1 bdf: 03:01.0 >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>> xen_unmap_pcidev id: 1 bdf: 03:03.0 >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 >>>> xen_unmap_pcidev id: 1 bdf: 03:04.0 >>>> xen_map_pcidev id: 1 bdf: 00:04.0 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>> xen_map_pcidev id: 1 bdf: 01:03.0 >>>> xen_unmap_pcidev id: 1 bdf: 00:17.0 >>>> xen_map_pcidev id: 1 bdf: 01:17.0 >>>> xen_unmap_pcidev id: 1 bdf: 02:01.0 >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>> xen_map_pcidev id: 1 bdf: 02:01.0 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 >>>> xen_map_pcidev id: 1 bdf: 03:01.0 >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 >>>> xen_map_pcidev id: 1 bdf: 03:03.0 >>>> xen_unmap_pcidev id: 1 bdf: 00:04.0 >>>> xen_map_pcidev id: 1 bdf: 03:04.0 >>>> >>>> Signed-off-by: Don Slutz <dslutz@verizon.com> >>>> CC: Don Slutz <don.slutz@gmail.com> >>>> --- >>>> include/hw/xen/xen_common.h | 53 >>>> +++++++++++++++++++++++++++++++++++---------- >>>> trace-events | 6 +++-- >>>> xen-hvm.c | 15 ++++++++----- >>>> 3 files changed, 55 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/include/hw/xen/xen_common.h >>>> b/include/hw/xen/xen_common.h >>>> index 6579b78..260ee58 100644 >>>> --- a/include/hw/xen/xen_common.h >>>> +++ b/include/hw/xen/xen_common.h >>>> @@ -223,12 +223,14 @@ static inline void xen_unmap_io_section(XenXC >> xc, >>>> domid_t dom, >>>> >>>> static inline void xen_map_pcidev(XenXC xc, domid_t dom, >>>> ioservid_t ioservid, >>>> + GHashTable *pci_bdf, >>>> PCIDevice *pci_dev) >>>> { >>>> } >>>> >>>> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, >>>> ioservid_t ioservid, >>>> + GHashTable *pci_bdf, >>>> PCIDevice *pci_dev, >>>> uint8_t oldbus) >>>> { >>>> @@ -346,27 +348,54 @@ static inline void xen_unmap_io_section(XenXC >> xc, >>>> domid_t dom, >>>> >>>> static inline void xen_map_pcidev(XenXC xc, domid_t dom, >>>> ioservid_t ioservid, >>>> + GHashTable *realizedpci_bdf, >>>> PCIDevice *pci_dev) >>>> { >>>> - trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), >>>> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); >>>> - xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, >>>> - 0, pci_bus_num(pci_dev->bus), >>>> - PCI_SLOT(pci_dev->devfn), >>>> - PCI_FUNC(pci_dev->devfn)); >>>> + gpointer bdf_key = GINT_TO_POINTER((pci_bus_num(pci_dev->bus) >> << >>>> 8) | >>>> + pci_dev->devfn); >>>> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, >>>> bdf_key)); >>>> + >>>> + if (!bdf_cnt) { >>>> + trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), >>>> + PCI_SLOT(pci_dev->devfn), >>>> + PCI_FUNC(pci_dev->devfn)); >>>> + g_hash_table_insert(pci_bdf, bdf_key, GINT_TO_POINTER(1)); >>>> + xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, >>>> + 0, pci_bus_num(pci_dev->bus), >>>> + PCI_SLOT(pci_dev->devfn), >>>> + PCI_FUNC(pci_dev->devfn)); >>>> + } else { >>>> + trace_xen_map_pcidev_dup(ioservid, pci_bus_num(pci_dev->bus), >>>> + PCI_SLOT(pci_dev->devfn), >>>> + PCI_FUNC(pci_dev->devfn), >>>> + bdf_cnt + 1); >>>> + g_hash_table_replace(pci_bdf, bdf_key, >> GINT_TO_POINTER(bdf_cnt + >>>> 1)); >>>> + } >>>> } >>>> >>>> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, >>>> ioservid_t ioservid, >>>> + GHashTable *pci_bdf, >>>> PCIDevice *pci_dev, >>>> uint8_t oldbus) >>>> { >>>> - trace_xen_unmap_pcidev(ioservid, oldbus, >>>> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); >>>> - xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, >>>> - 0, oldbus, >>>> - PCI_SLOT(pci_dev->devfn), >>>> - PCI_FUNC(pci_dev->devfn)); >>>> + gpointer bdf_key = GINT_TO_POINTER((oldbus << 8) | pci_dev- >>> devfn); >>>> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, >>>> bdf_key)); >>>> + >>>> + if (bdf_cnt == 1) { >>>> + trace_xen_unmap_pcidev(ioservid, oldbus, >>>> + PCI_SLOT(pci_dev->devfn), >>>> + PCI_FUNC(pci_dev->devfn)); >>>> + g_hash_table_remove(pci_bdf, bdf_key); >>>> + xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, >>>> + 0, oldbus, >>>> + PCI_SLOT(pci_dev->devfn), >>>> + PCI_FUNC(pci_dev->devfn)); >>>> + } else { >>>> + trace_xen_unmap_pcidev_dup(ioservid, oldbus, PCI_SLOT(pci_dev- >>>>> devfn), >>>> + PCI_FUNC(pci_dev->devfn), bdf_cnt - 1); >>>> + g_hash_table_replace(pci_bdf, bdf_key, >> GINT_TO_POINTER(bdf_cnt - >>>> 1)); >>>> + } >>>> } >>>> >>>> static inline int xen_create_ioreq_server(XenXC xc, domid_t dom, >>>> diff --git a/trace-events b/trace-events >>>> index a589650..58deeaf 100644 >>>> --- a/trace-events >>>> +++ b/trace-events >>>> @@ -930,8 +930,10 @@ xen_map_mmio_range(uint32_t id, uint64_t >>>> start_addr, uint64_t end_addr) "id: %u >>>> xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t >>>> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 >>>> xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t >> end_addr) >>>> "id: %u start: %#"PRIx64" end: %#"PRIx64 >>>> xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t >>>> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 >>>> -xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: >> %u >>>> bdf: %02x.%02x.%02x" >>>> -xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) >> "id: >>>> %u bdf: %02x.%02x.%02x" >>>> +xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: >> %u >>>> bdf: %02x:%02x.%x" >>>> +xen_map_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t >> func, >>>> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" >>>> +xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) >> "id: >>>> %u bdf: %02x:%02x.%x" >>>> +xen_unmap_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t >> func, >>>> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" >>>> >>>> # xen-mapcache.c >>>> xen_map_cache(uint64_t phys_addr) "want %#"PRIx64 >>>> diff --git a/xen-hvm.c b/xen-hvm.c >>>> index 7b6d8f1..f041909 100644 >>>> --- a/xen-hvm.c >>>> +++ b/xen-hvm.c >>>> @@ -123,6 +123,7 @@ typedef struct XenIOState { >>>> MemoryListener memory_listener; >>>> MemoryListener io_listener; >>>> DeviceListener device_listener; >>>> + GHashTable *pci_bdf; >>>> QLIST_HEAD(, XenPhysmap) physmap; >>>> hwaddr free_phys_offset; >>>> const XenPhysmap *log_for_dirtybit; >>>> @@ -578,7 +579,8 @@ static void xen_device_realize(DeviceListener >>>> *listener, >>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >>>> PCIDevice *pci_dev = PCI_DEVICE(dev); >>>> >>>> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); >>>> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- >>> pci_bdf, >>>> + pci_dev); >>>> } >>>> } >>>> >>>> @@ -590,8 +592,8 @@ static void xen_device_unrealize(DeviceListener >>>> *listener, >>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >>>> PCIDevice *pci_dev = PCI_DEVICE(dev); >>>> >>>> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, >>>> - pci_bus_num(pci_dev->bus)); >>>> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- >>>>> pci_bdf, >>>> + pci_dev, pci_bus_num(pci_dev->bus)); >>>> } >>>> } >>>> >>>> @@ -600,8 +602,10 @@ static void >>>> xen_device_change_pci_bus_num(DeviceListener *listener, >>>> { >>>> XenIOState *state = container_of(listener, XenIOState, >> device_listener); >>>> >>>> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, >>>> oldbus); >>>> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); >>>> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- >>> pci_bdf, >>>> + pci_dev, oldbus); >>>> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- >>> pci_bdf, >>>> + pci_dev); >>>> } >>>> >>>> static void xen_sync_dirty_bitmap(XenIOState *state, >>>> @@ -1318,6 +1322,7 @@ int xen_hvm_init(ram_addr_t >>>> *below_4g_mem_size, ram_addr_t *above_4g_mem_size, >>>> memory_listener_register(&state->io_listener, &address_space_io); >>>> >>>> state->device_listener = xen_device_listener; >>>> + state->pci_bdf = g_hash_table_new(NULL, NULL); >>>> device_listener_register(&state->device_listener); >>>> >>>> /* Initialize backend core & drivers */ >>>> -- >>>> 1.8.4 >>> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server 2015-06-09 14:28 ` Don Slutz @ 2015-06-09 15:11 ` Paul Durrant 2015-06-09 16:40 ` Don Slutz 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 15:11 UTC (permalink / raw) To: Don Slutz, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 09 June 2015 15:28 > To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; xen- > devel@lists.xen.org > Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz > Subject: Re: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server > > On 06/09/15 10:03, Paul Durrant wrote: > >> -----Original Message----- > >> From: Don Slutz [mailto:dslutz@verizon.com] > >> Sent: 09 June 2015 14:51 > >> To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; > xen- > >> devel@lists.xen.org > >> Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz > >> Subject: Re: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server > >> > >> On 06/09/15 05:05, Paul Durrant wrote: > >>>> -----Original Message----- > >>>> From: Don Slutz [mailto:dslutz@verizon.com] > >>>> Sent: 08 June 2015 22:19 > >>>> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org > >>>> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don > Slutz > >>>> Subject: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server > >>>> > >>>> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 added usage > of > >>>> xc_hvm_map_pcidev_from_ioreq_server() and > >>>> xc_hvm_unmap_pcidev_from_ioreq_server(). These routines only > >>>> correctly work if the PCI BDF of a PCI device is unique. The 3 > >>>> parts (bus, device, and function) of a PCI BDF are not required to > >>>> be unique. > >>>> > >>>> The PCI BDF is accessed in QEMU: > >>>> bus pci_bus_num() > >>>> device PCI_SLOT() > >>>> function PCI_FUNC() > >>>> > >>>> Add a hash table to track the current set of PCI BDFs and only call > >>>> on xc_hvm_map_pcidev_from_ioreq_server() and > >>>> xc_hvm_unmap_pcidev_from_ioreq_server() when needed. > >>>> > >>> > >>> This seems to imply that the devices are being realized multiple times > >> without being unrealized? > >> > >> Not directly. The devices are not being "realized" in QEMU terms. The > >> PCI to PCI brige is changing state, and the BDF for all PCI devices on > >> the secondary bus changes when the PCI config named > >> PCI_SECONDARY_BUS on > >> the PCI to PCI bridge is changed. This is what patch #2 is all about. > >> > >>> Is that really the case? > >>> > >> > >> That is what it looks like to Xen. But the device listener callbacks > >> are not called. > > > > Then what's calling xc_hvm_map_pcidev_from_ioreq_server()? > > > > Currently (without these patches) the calls on > xc_hvm_map_pcidev_from_ioreq_server() are the ones you added. These > are > from QEMU's device realize. However since on PCI reset (aka start up) > the seconary bus number is zero. So the call on > pci_bus_num(pci_dev->bus) will return zero in that case. So using the > subset trace output (copied from below) and adjusted to be the old way: > > xen_map_pcidev id: 1 bdf: 00.01.00 > xen_map_pcidev id: 1 bdf: 00.01.00 > xen_map_pcidev id: 1 bdf: 00.01.00 > > Is part of what you see. It may help to have the Xen config that matters: > > device_model_args_hvm = [ > "-monitor", > "pty", > "-boot", > "menu=on", > "-device", > "pci-bridge,chassis_nr=1,msi=on,id=pciBridge1.0,addr=0x11.0", > "-device", > "pci- > bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0", > "-device", > "pci- > bridge,chassis_nr=3,msi=on,id=pciBridge6.0,multifunction=on,addr=0x16.0", > "-device", > "pci- > bridge,chassis_nr=4,msi=on,id=pciBridge7.0,multifunction=on,bus=pciBridge > 1.0,addr=0x17.0", > "-device", > "pci- > bridge,chassis_nr=5,msi=on,id=pciBridge8.0,multifunction=on,addr=0x18.0", > "-device", > "pvscsi,id=scsi0,bus=pciBridge5.0,addr=0x1.0x0", > "-drive", > "if=none,id=disk0-0,format=raw,file=/dev/etherd/e500.1", > "-device", > "scsi-disk,vendor=VMware,ver=1.0,product=Virtual > disk,bus=scsi0.0,scsi-id=0,drive=disk0-0", > "-device", > "pvscsi,id=sas1,bus=pciBridge7.0,addr=0x1.0x0", > "-drive", > "if=none,id=disk1-1,format=raw,file=/dev/etherd/e500.2", > "-device", > "scsi-disk,vendor=VMware,ver=1.0,product=Virtual > disk,bus=sas1.0,scsi-id=1,drive=disk1-1", > "-device", > "pvscsi,id=scsi2,bus=pciBridge1.0,addr=0x3.0x0", > "-drive", > "if=none,id=disk2-2,format=raw,file=/dev/etherd/e500.3", > "-device", > "scsi-disk,vendor=VMware,ver=1.0,product=Virtual > disk,bus=scsi2.0,scsi-id=2,drive=disk2-2", > "-device", > > "e1000,id=nic2,netdev=net2,mac=00:0c:29:86:44:b4,bus=pciBridge5.0,addr=0 > x3.0x0", > "-netdev", > "type=tap,id=net2,ifname=vif.2-emu,script=/etc/qemu- > ifup,downscript=no", > "-device", > > "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,add > r=0x4.0x0", > "-netdev", > "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu- > ifup,downscript=no", > ] > > > If you want to try this, you would also need a bootable disk listed in > disk= > > > I don't understand why you are having to introduce this to avoid > > duplicate calls? > > > > That is because in general a PCI device's BDF is not fixed. The only > fixed ones are the ones on the root (host bus). Yes, the code was written with that assumption, which is correct if you use the libxl toolstack. You are bypassing the toolstack and hence running into problems. > It might help to look > at the code for pci_bus_num(): > > int pci_bus_num(PCIBus *s) > { > if (pci_bus_is_root(s)) > return 0; /* pci host bridge */ > return s->parent_dev->config[PCI_SECONDARY_BUS]; > } > > So the bus number this routine returns depends on what the guest has set > in the PCI config space of the PCI to PCI bridge. > So, if the ioreq server registration interface is to remain based on SBDF (and I'm not sure what alternative there is), I think you are going to have to make Xen aware of PCI<->PCI bridges so it can track the bus number changes to already-registered devices. The alternative of broadcasting config. space accesses is somewhat unpalatable and also means you'd need a fundamental change in the way ioreqs are handled. Paul > The starting (and reset) value is 0. So for the config above at QEMU > device realized time there are 3 PCI devices (PIIX4, 2 pvscsi disk > controllers). > > -Don Slutz > > > Paul > > > > >> > >> -Don Slutz > >> > >>> Paul > >>> > >>>> Also fix the PCI BDF default stderr trace output: bus is seperated > >>>> by ':', and function is only 1 digit. > >>>> > >>>> Here is an example of stderr trace output: > >>>> > >>>> xen_map_pcidev id: 1 bdf: 00:00.0 > >>>> xen_map_pcidev id: 1 bdf: 00:01.0 > >>>> xen_map_pcidev id: 1 bdf: 00:01.1 > >>>> xen_map_pcidev id: 1 bdf: 00:01.3 > >>>> xen_map_pcidev id: 1 bdf: 00:02.0 > >>>> xen_map_pcidev id: 1 bdf: 00:03.0 > >>>> xen_map_pcidev id: 1 bdf: 00:04.0 > >>>> xen_map_pcidev id: 1 bdf: 00:05.0 > >>>> xen_map_pcidev id: 1 bdf: 00:11.0 > >>>> xen_map_pcidev id: 1 bdf: 00:15.0 > >>>> xen_map_pcidev id: 1 bdf: 00:16.0 > >>>> xen_map_pcidev id: 1 bdf: 00:17.0 > >>>> xen_map_pcidev id: 1 bdf: 00:18.0 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:04.0 dup: 2 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >>>> xen_map_pcidev id: 1 bdf: ff:03.0 > >>>> xen_unmap_pcidev id: 1 bdf: 00:17.0 > >>>> xen_map_pcidev id: 1 bdf: ff:17.0 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >>>> xen_map_pcidev id: 1 bdf: ff:01.0 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 > >>>> xen_map_pcidev_dup id: 1 bdf: ff:03.0 dup: 2 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:04.0 dup: 1 > >>>> xen_map_pcidev id: 1 bdf: ff:04.0 > >>>> xen_unmap_pcidev_dup id: 1 bdf: ff:03.0 dup: 1 > >>>> xen_map_pcidev id: 1 bdf: 01:03.0 > >>>> xen_unmap_pcidev id: 1 bdf: ff:17.0 > >>>> xen_map_pcidev id: 1 bdf: 01:17.0 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 > >>>> xen_map_pcidev_dup id: 1 bdf: ff:01.0 dup: 2 > >>>> xen_unmap_pcidev_dup id: 1 bdf: ff:01.0 dup: 1 > >>>> xen_map_pcidev id: 1 bdf: 02:01.0 > >>>> xen_unmap_pcidev id: 1 bdf: ff:01.0 > >>>> xen_map_pcidev id: 1 bdf: 03:01.0 > >>>> xen_unmap_pcidev id: 1 bdf: ff:03.0 > >>>> xen_map_pcidev id: 1 bdf: 03:03.0 > >>>> xen_unmap_pcidev id: 1 bdf: ff:04.0 > >>>> xen_map_pcidev id: 1 bdf: 03:04.0 > >>>> xen_unmap_pcidev id: 1 bdf: 00:04.0 > >>>> xen_unmap_pcidev id: 1 bdf: 00:05.0 > >>>> xen_unmap_pcidev id: 1 bdf: 01:03.0 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >>>> xen_unmap_pcidev id: 1 bdf: 01:17.0 > >>>> xen_map_pcidev id: 1 bdf: 00:17.0 > >>>> xen_unmap_pcidev id: 1 bdf: 03:01.0 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >>>> xen_unmap_pcidev id: 1 bdf: 03:03.0 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 > >>>> xen_unmap_pcidev id: 1 bdf: 03:04.0 > >>>> xen_map_pcidev id: 1 bdf: 00:04.0 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 > >>>> xen_map_pcidev id: 1 bdf: 01:03.0 > >>>> xen_unmap_pcidev id: 1 bdf: 00:17.0 > >>>> xen_map_pcidev id: 1 bdf: 01:17.0 > >>>> xen_unmap_pcidev id: 1 bdf: 02:01.0 > >>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 > >>>> xen_map_pcidev id: 1 bdf: 02:01.0 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 > >>>> xen_map_pcidev id: 1 bdf: 03:01.0 > >>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 > >>>> xen_map_pcidev id: 1 bdf: 03:03.0 > >>>> xen_unmap_pcidev id: 1 bdf: 00:04.0 > >>>> xen_map_pcidev id: 1 bdf: 03:04.0 > >>>> > >>>> Signed-off-by: Don Slutz <dslutz@verizon.com> > >>>> CC: Don Slutz <don.slutz@gmail.com> > >>>> --- > >>>> include/hw/xen/xen_common.h | 53 > >>>> +++++++++++++++++++++++++++++++++++---------- > >>>> trace-events | 6 +++-- > >>>> xen-hvm.c | 15 ++++++++----- > >>>> 3 files changed, 55 insertions(+), 19 deletions(-) > >>>> > >>>> diff --git a/include/hw/xen/xen_common.h > >>>> b/include/hw/xen/xen_common.h > >>>> index 6579b78..260ee58 100644 > >>>> --- a/include/hw/xen/xen_common.h > >>>> +++ b/include/hw/xen/xen_common.h > >>>> @@ -223,12 +223,14 @@ static inline void > xen_unmap_io_section(XenXC > >> xc, > >>>> domid_t dom, > >>>> > >>>> static inline void xen_map_pcidev(XenXC xc, domid_t dom, > >>>> ioservid_t ioservid, > >>>> + GHashTable *pci_bdf, > >>>> PCIDevice *pci_dev) > >>>> { > >>>> } > >>>> > >>>> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, > >>>> ioservid_t ioservid, > >>>> + GHashTable *pci_bdf, > >>>> PCIDevice *pci_dev, > >>>> uint8_t oldbus) > >>>> { > >>>> @@ -346,27 +348,54 @@ static inline void > xen_unmap_io_section(XenXC > >> xc, > >>>> domid_t dom, > >>>> > >>>> static inline void xen_map_pcidev(XenXC xc, domid_t dom, > >>>> ioservid_t ioservid, > >>>> + GHashTable *realizedpci_bdf, > >>>> PCIDevice *pci_dev) > >>>> { > >>>> - trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), > >>>> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); > >>>> - xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, > >>>> - 0, pci_bus_num(pci_dev->bus), > >>>> - PCI_SLOT(pci_dev->devfn), > >>>> - PCI_FUNC(pci_dev->devfn)); > >>>> + gpointer bdf_key = GINT_TO_POINTER((pci_bus_num(pci_dev- > >bus) > >> << > >>>> 8) | > >>>> + pci_dev->devfn); > >>>> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, > >>>> bdf_key)); > >>>> + > >>>> + if (!bdf_cnt) { > >>>> + trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), > >>>> + PCI_SLOT(pci_dev->devfn), > >>>> + PCI_FUNC(pci_dev->devfn)); > >>>> + g_hash_table_insert(pci_bdf, bdf_key, GINT_TO_POINTER(1)); > >>>> + xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, > >>>> + 0, pci_bus_num(pci_dev->bus), > >>>> + PCI_SLOT(pci_dev->devfn), > >>>> + PCI_FUNC(pci_dev->devfn)); > >>>> + } else { > >>>> + trace_xen_map_pcidev_dup(ioservid, pci_bus_num(pci_dev- > >bus), > >>>> + PCI_SLOT(pci_dev->devfn), > >>>> + PCI_FUNC(pci_dev->devfn), > >>>> + bdf_cnt + 1); > >>>> + g_hash_table_replace(pci_bdf, bdf_key, > >> GINT_TO_POINTER(bdf_cnt + > >>>> 1)); > >>>> + } > >>>> } > >>>> > >>>> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, > >>>> ioservid_t ioservid, > >>>> + GHashTable *pci_bdf, > >>>> PCIDevice *pci_dev, > >>>> uint8_t oldbus) > >>>> { > >>>> - trace_xen_unmap_pcidev(ioservid, oldbus, > >>>> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev- > >devfn)); > >>>> - xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, > >>>> - 0, oldbus, > >>>> - PCI_SLOT(pci_dev->devfn), > >>>> - PCI_FUNC(pci_dev->devfn)); > >>>> + gpointer bdf_key = GINT_TO_POINTER((oldbus << 8) | pci_dev- > >>> devfn); > >>>> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, > >>>> bdf_key)); > >>>> + > >>>> + if (bdf_cnt == 1) { > >>>> + trace_xen_unmap_pcidev(ioservid, oldbus, > >>>> + PCI_SLOT(pci_dev->devfn), > >>>> + PCI_FUNC(pci_dev->devfn)); > >>>> + g_hash_table_remove(pci_bdf, bdf_key); > >>>> + xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, > >>>> + 0, oldbus, > >>>> + PCI_SLOT(pci_dev->devfn), > >>>> + PCI_FUNC(pci_dev->devfn)); > >>>> + } else { > >>>> + trace_xen_unmap_pcidev_dup(ioservid, oldbus, > PCI_SLOT(pci_dev- > >>>>> devfn), > >>>> + PCI_FUNC(pci_dev->devfn), bdf_cnt - 1); > >>>> + g_hash_table_replace(pci_bdf, bdf_key, > >> GINT_TO_POINTER(bdf_cnt - > >>>> 1)); > >>>> + } > >>>> } > >>>> > >>>> static inline int xen_create_ioreq_server(XenXC xc, domid_t dom, > >>>> diff --git a/trace-events b/trace-events > >>>> index a589650..58deeaf 100644 > >>>> --- a/trace-events > >>>> +++ b/trace-events > >>>> @@ -930,8 +930,10 @@ xen_map_mmio_range(uint32_t id, uint64_t > >>>> start_addr, uint64_t end_addr) "id: %u > >>>> xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t > >>>> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 > >>>> xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t > >> end_addr) > >>>> "id: %u start: %#"PRIx64" end: %#"PRIx64 > >>>> xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t > >>>> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 > >>>> -xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) > "id: > >> %u > >>>> bdf: %02x.%02x.%02x" > >>>> -xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t > func) > >> "id: > >>>> %u bdf: %02x.%02x.%02x" > >>>> +xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) > "id: > >> %u > >>>> bdf: %02x:%02x.%x" > >>>> +xen_map_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t > >> func, > >>>> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" > >>>> +xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t > func) > >> "id: > >>>> %u bdf: %02x:%02x.%x" > >>>> +xen_unmap_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, > uint8_t > >> func, > >>>> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" > >>>> > >>>> # xen-mapcache.c > >>>> xen_map_cache(uint64_t phys_addr) "want %#"PRIx64 > >>>> diff --git a/xen-hvm.c b/xen-hvm.c > >>>> index 7b6d8f1..f041909 100644 > >>>> --- a/xen-hvm.c > >>>> +++ b/xen-hvm.c > >>>> @@ -123,6 +123,7 @@ typedef struct XenIOState { > >>>> MemoryListener memory_listener; > >>>> MemoryListener io_listener; > >>>> DeviceListener device_listener; > >>>> + GHashTable *pci_bdf; > >>>> QLIST_HEAD(, XenPhysmap) physmap; > >>>> hwaddr free_phys_offset; > >>>> const XenPhysmap *log_for_dirtybit; > >>>> @@ -578,7 +579,8 @@ static void xen_device_realize(DeviceListener > >>>> *listener, > >>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > >>>> PCIDevice *pci_dev = PCI_DEVICE(dev); > >>>> > >>>> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); > >>>> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- > >>> pci_bdf, > >>>> + pci_dev); > >>>> } > >>>> } > >>>> > >>>> @@ -590,8 +592,8 @@ static void xen_device_unrealize(DeviceListener > >>>> *listener, > >>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > >>>> PCIDevice *pci_dev = PCI_DEVICE(dev); > >>>> > >>>> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, > >>>> - pci_bus_num(pci_dev->bus)); > >>>> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- > >>>>> pci_bdf, > >>>> + pci_dev, pci_bus_num(pci_dev->bus)); > >>>> } > >>>> } > >>>> > >>>> @@ -600,8 +602,10 @@ static void > >>>> xen_device_change_pci_bus_num(DeviceListener *listener, > >>>> { > >>>> XenIOState *state = container_of(listener, XenIOState, > >> device_listener); > >>>> > >>>> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, > >>>> oldbus); > >>>> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); > >>>> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- > >>> pci_bdf, > >>>> + pci_dev, oldbus); > >>>> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- > >>> pci_bdf, > >>>> + pci_dev); > >>>> } > >>>> > >>>> static void xen_sync_dirty_bitmap(XenIOState *state, > >>>> @@ -1318,6 +1322,7 @@ int xen_hvm_init(ram_addr_t > >>>> *below_4g_mem_size, ram_addr_t *above_4g_mem_size, > >>>> memory_listener_register(&state->io_listener, > &address_space_io); > >>>> > >>>> state->device_listener = xen_device_listener; > >>>> + state->pci_bdf = g_hash_table_new(NULL, NULL); > >>>> device_listener_register(&state->device_listener); > >>>> > >>>> /* Initialize backend core & drivers */ > >>>> -- > >>>> 1.8.4 > >>> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server 2015-06-09 15:11 ` Paul Durrant @ 2015-06-09 16:40 ` Don Slutz 0 siblings, 0 replies; 23+ messages in thread From: Don Slutz @ 2015-06-09 16:40 UTC (permalink / raw) To: Paul Durrant, Slutz, Donald Christopher, qemu-devel@nongnu.org, xen-devel@lists.xen.org Cc: Stefano Stabellini, Don Slutz, Michael S. Tsirkin On 06/09/15 11:11, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 09 June 2015 15:28 >> To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; xen- >> devel@lists.xen.org >> Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz >> Subject: Re: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server >> >> On 06/09/15 10:03, Paul Durrant wrote: >>>> -----Original Message----- >>>> From: Don Slutz [mailto:dslutz@verizon.com] >>>> Sent: 09 June 2015 14:51 >>>> To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@nongnu.org; >> xen- >>>> devel@lists.xen.org >>>> Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz >>>> Subject: Re: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server >>>> >>>> On 06/09/15 05:05, Paul Durrant wrote: >>>>>> -----Original Message----- >>>>>> From: Don Slutz [mailto:dslutz@verizon.com] >>>>>> Sent: 08 June 2015 22:19 >>>>>> To: qemu-devel@nongnu.org; xen-devel@lists.xen.org >>>>>> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don >> Slutz >>>>>> Subject: [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server >>>>>> >>>>>> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 added usage >> of >>>>>> xc_hvm_map_pcidev_from_ioreq_server() and >>>>>> xc_hvm_unmap_pcidev_from_ioreq_server(). These routines only >>>>>> correctly work if the PCI BDF of a PCI device is unique. The 3 >>>>>> parts (bus, device, and function) of a PCI BDF are not required to >>>>>> be unique. >>>>>> >>>>>> The PCI BDF is accessed in QEMU: >>>>>> bus pci_bus_num() >>>>>> device PCI_SLOT() >>>>>> function PCI_FUNC() >>>>>> >>>>>> Add a hash table to track the current set of PCI BDFs and only call >>>>>> on xc_hvm_map_pcidev_from_ioreq_server() and >>>>>> xc_hvm_unmap_pcidev_from_ioreq_server() when needed. >>>>>> >>>>> >>>>> This seems to imply that the devices are being realized multiple times >>>> without being unrealized? >>>> >>>> Not directly. The devices are not being "realized" in QEMU terms. The >>>> PCI to PCI brige is changing state, and the BDF for all PCI devices on >>>> the secondary bus changes when the PCI config named >>>> PCI_SECONDARY_BUS on >>>> the PCI to PCI bridge is changed. This is what patch #2 is all about. >>>> >>>>> Is that really the case? >>>>> >>>> >>>> That is what it looks like to Xen. But the device listener callbacks >>>> are not called. >>> >>> Then what's calling xc_hvm_map_pcidev_from_ioreq_server()? >> >> >> >> Currently (without these patches) the calls on >> xc_hvm_map_pcidev_from_ioreq_server() are the ones you added. These >> are >> from QEMU's device realize. However since on PCI reset (aka start up) >> the seconary bus number is zero. So the call on >> pci_bus_num(pci_dev->bus) will return zero in that case. So using the >> subset trace output (copied from below) and adjusted to be the old way: >> >> xen_map_pcidev id: 1 bdf: 00.01.00 >> xen_map_pcidev id: 1 bdf: 00.01.00 >> xen_map_pcidev id: 1 bdf: 00.01.00 >> >> Is part of what you see. It may help to have the Xen config that matters: >> >> device_model_args_hvm = [ >> "-monitor", >> "pty", >> "-boot", >> "menu=on", >> "-device", >> "pci-bridge,chassis_nr=1,msi=on,id=pciBridge1.0,addr=0x11.0", >> "-device", >> "pci- >> bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0", >> "-device", >> "pci- >> bridge,chassis_nr=3,msi=on,id=pciBridge6.0,multifunction=on,addr=0x16.0", >> "-device", >> "pci- >> bridge,chassis_nr=4,msi=on,id=pciBridge7.0,multifunction=on,bus=pciBridge >> 1.0,addr=0x17.0", >> "-device", >> "pci- >> bridge,chassis_nr=5,msi=on,id=pciBridge8.0,multifunction=on,addr=0x18.0", >> "-device", >> "pvscsi,id=scsi0,bus=pciBridge5.0,addr=0x1.0x0", >> "-drive", >> "if=none,id=disk0-0,format=raw,file=/dev/etherd/e500.1", >> "-device", >> "scsi-disk,vendor=VMware,ver=1.0,product=Virtual >> disk,bus=scsi0.0,scsi-id=0,drive=disk0-0", >> "-device", >> "pvscsi,id=sas1,bus=pciBridge7.0,addr=0x1.0x0", >> "-drive", >> "if=none,id=disk1-1,format=raw,file=/dev/etherd/e500.2", >> "-device", >> "scsi-disk,vendor=VMware,ver=1.0,product=Virtual >> disk,bus=sas1.0,scsi-id=1,drive=disk1-1", >> "-device", >> "pvscsi,id=scsi2,bus=pciBridge1.0,addr=0x3.0x0", >> "-drive", >> "if=none,id=disk2-2,format=raw,file=/dev/etherd/e500.3", >> "-device", >> "scsi-disk,vendor=VMware,ver=1.0,product=Virtual >> disk,bus=scsi2.0,scsi-id=2,drive=disk2-2", >> "-device", >> >> "e1000,id=nic2,netdev=net2,mac=00:0c:29:86:44:b4,bus=pciBridge5.0,addr=0 >> x3.0x0", >> "-netdev", >> "type=tap,id=net2,ifname=vif.2-emu,script=/etc/qemu- >> ifup,downscript=no", >> "-device", >> >> "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,add >> r=0x4.0x0", >> "-netdev", >> "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu- >> ifup,downscript=no", >> ] >> >> >> If you want to try this, you would also need a bootable disk listed in >> disk= >> >>> I don't understand why you are having to introduce this to avoid >>> duplicate calls? >>> >> >> That is because in general a PCI device's BDF is not fixed. The only >> fixed ones are the ones on the root (host bus). > > Yes, the code was written with that assumption, which is correct if you use the libxl toolstack. You are bypassing the toolstack and hence running into problems. > I am bypassing the libxl toolstack. But this works in QEMU 2.2.0, breaks in QEMU 2.3.0. To me that is a bug. So I do not see "have to only use libxl toolstack features" to make this issue not a bug. I have opened QEMU Bug #1463463, for the issue that I have and this patch set fixes so as not to lose that part of the discussion. >> It might help to look >> at the code for pci_bus_num(): >> >> int pci_bus_num(PCIBus *s) >> { >> if (pci_bus_is_root(s)) >> return 0; /* pci host bridge */ >> return s->parent_dev->config[PCI_SECONDARY_BUS]; >> } >> >> So the bus number this routine returns depends on what the guest has set >> in the PCI config space of the PCI to PCI bridge. >> > > So, if the ioreq server registration interface is to remain based on SBDF (and I'm not sure what alternative there is), I think you are going to have to make Xen aware of PCI<->PCI bridges so it can track the bus number changes to already-registered devices. In some sense this patch set does this. It lets Xen know when the SBDF changes. Moving the code into Xen to do this monitoring appears to me to be the inverse of the current design (where QEMU tells Xen about a set of SBDFs). The monitoring code is the one that need to know about PCI to PCI bridges and their config space writes which is what QEMU already has. Adding PCI-to_PCI bridges to libxl to me is an enhancement and is not required to fix the bug. > The alternative of broadcasting config. space accesses is somewhat unpalatable and also means you'd need a fundamental change in the way ioreqs are handled. > I am not interested in changing to the "broadcasting config. space accesses". I am mostly focused on getting the bug fixed. But I do not want to fix it and causes issues in the future. Having 2 or more ioreq servers (aka QEMU) handle PCI devices that are not on a root bus looks to be really complex if they share a root (host) bus and/or PCI to PCI bridge. Currently one of the PCI device that Xen knows about (VIFs) does not have a way to specify the PCI SBDF in libxl and so it's SBDF is not fixed. -Don Slutz > Paul > >> The starting (and reset) value is 0. So for the config above at QEMU >> device realized time there are 3 PCI devices (PIIX4, 2 pvscsi disk >> controllers). >> >> -Don Slutz >> >>> Paul >>> >> >>>> >>>> -Don Slutz >>>> >>>>> Paul >>>>> >>>>>> Also fix the PCI BDF default stderr trace output: bus is seperated >>>>>> by ':', and function is only 1 digit. >>>>>> >>>>>> Here is an example of stderr trace output: >>>>>> >>>>>> xen_map_pcidev id: 1 bdf: 00:00.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:01.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:01.1 >>>>>> xen_map_pcidev id: 1 bdf: 00:01.3 >>>>>> xen_map_pcidev id: 1 bdf: 00:02.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:03.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:04.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:05.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:11.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:15.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:16.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:17.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:18.0 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:04.0 dup: 2 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>>>> xen_map_pcidev id: 1 bdf: ff:03.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 00:17.0 >>>>>> xen_map_pcidev id: 1 bdf: ff:17.0 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>>>> xen_map_pcidev id: 1 bdf: ff:01.0 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 >>>>>> xen_map_pcidev_dup id: 1 bdf: ff:03.0 dup: 2 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:04.0 dup: 1 >>>>>> xen_map_pcidev id: 1 bdf: ff:04.0 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: ff:03.0 dup: 1 >>>>>> xen_map_pcidev id: 1 bdf: 01:03.0 >>>>>> xen_unmap_pcidev id: 1 bdf: ff:17.0 >>>>>> xen_map_pcidev id: 1 bdf: 01:17.0 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 >>>>>> xen_map_pcidev_dup id: 1 bdf: ff:01.0 dup: 2 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: ff:01.0 dup: 1 >>>>>> xen_map_pcidev id: 1 bdf: 02:01.0 >>>>>> xen_unmap_pcidev id: 1 bdf: ff:01.0 >>>>>> xen_map_pcidev id: 1 bdf: 03:01.0 >>>>>> xen_unmap_pcidev id: 1 bdf: ff:03.0 >>>>>> xen_map_pcidev id: 1 bdf: 03:03.0 >>>>>> xen_unmap_pcidev id: 1 bdf: ff:04.0 >>>>>> xen_map_pcidev id: 1 bdf: 03:04.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 00:04.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 00:05.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 01:03.0 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>>>> xen_unmap_pcidev id: 1 bdf: 01:17.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:17.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 03:01.0 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>>>> xen_unmap_pcidev id: 1 bdf: 03:03.0 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:03.0 dup: 3 >>>>>> xen_unmap_pcidev id: 1 bdf: 03:04.0 >>>>>> xen_map_pcidev id: 1 bdf: 00:04.0 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 2 >>>>>> xen_map_pcidev id: 1 bdf: 01:03.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 00:17.0 >>>>>> xen_map_pcidev id: 1 bdf: 01:17.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 02:01.0 >>>>>> xen_map_pcidev_dup id: 1 bdf: 00:01.0 dup: 3 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 2 >>>>>> xen_map_pcidev id: 1 bdf: 02:01.0 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:01.0 dup: 1 >>>>>> xen_map_pcidev id: 1 bdf: 03:01.0 >>>>>> xen_unmap_pcidev_dup id: 1 bdf: 00:03.0 dup: 1 >>>>>> xen_map_pcidev id: 1 bdf: 03:03.0 >>>>>> xen_unmap_pcidev id: 1 bdf: 00:04.0 >>>>>> xen_map_pcidev id: 1 bdf: 03:04.0 >>>>>> >>>>>> Signed-off-by: Don Slutz <dslutz@verizon.com> >>>>>> CC: Don Slutz <don.slutz@gmail.com> >>>>>> --- >>>>>> include/hw/xen/xen_common.h | 53 >>>>>> +++++++++++++++++++++++++++++++++++---------- >>>>>> trace-events | 6 +++-- >>>>>> xen-hvm.c | 15 ++++++++----- >>>>>> 3 files changed, 55 insertions(+), 19 deletions(-) >>>>>> >>>>>> diff --git a/include/hw/xen/xen_common.h >>>>>> b/include/hw/xen/xen_common.h >>>>>> index 6579b78..260ee58 100644 >>>>>> --- a/include/hw/xen/xen_common.h >>>>>> +++ b/include/hw/xen/xen_common.h >>>>>> @@ -223,12 +223,14 @@ static inline void >> xen_unmap_io_section(XenXC >>>> xc, >>>>>> domid_t dom, >>>>>> >>>>>> static inline void xen_map_pcidev(XenXC xc, domid_t dom, >>>>>> ioservid_t ioservid, >>>>>> + GHashTable *pci_bdf, >>>>>> PCIDevice *pci_dev) >>>>>> { >>>>>> } >>>>>> >>>>>> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, >>>>>> ioservid_t ioservid, >>>>>> + GHashTable *pci_bdf, >>>>>> PCIDevice *pci_dev, >>>>>> uint8_t oldbus) >>>>>> { >>>>>> @@ -346,27 +348,54 @@ static inline void >> xen_unmap_io_section(XenXC >>>> xc, >>>>>> domid_t dom, >>>>>> >>>>>> static inline void xen_map_pcidev(XenXC xc, domid_t dom, >>>>>> ioservid_t ioservid, >>>>>> + GHashTable *realizedpci_bdf, >>>>>> PCIDevice *pci_dev) >>>>>> { >>>>>> - trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), >>>>>> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn)); >>>>>> - xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, >>>>>> - 0, pci_bus_num(pci_dev->bus), >>>>>> - PCI_SLOT(pci_dev->devfn), >>>>>> - PCI_FUNC(pci_dev->devfn)); >>>>>> + gpointer bdf_key = GINT_TO_POINTER((pci_bus_num(pci_dev- >>> bus) >>>> << >>>>>> 8) | >>>>>> + pci_dev->devfn); >>>>>> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, >>>>>> bdf_key)); >>>>>> + >>>>>> + if (!bdf_cnt) { >>>>>> + trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus), >>>>>> + PCI_SLOT(pci_dev->devfn), >>>>>> + PCI_FUNC(pci_dev->devfn)); >>>>>> + g_hash_table_insert(pci_bdf, bdf_key, GINT_TO_POINTER(1)); >>>>>> + xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid, >>>>>> + 0, pci_bus_num(pci_dev->bus), >>>>>> + PCI_SLOT(pci_dev->devfn), >>>>>> + PCI_FUNC(pci_dev->devfn)); >>>>>> + } else { >>>>>> + trace_xen_map_pcidev_dup(ioservid, pci_bus_num(pci_dev- >>> bus), >>>>>> + PCI_SLOT(pci_dev->devfn), >>>>>> + PCI_FUNC(pci_dev->devfn), >>>>>> + bdf_cnt + 1); >>>>>> + g_hash_table_replace(pci_bdf, bdf_key, >>>> GINT_TO_POINTER(bdf_cnt + >>>>>> 1)); >>>>>> + } >>>>>> } >>>>>> >>>>>> static inline void xen_unmap_pcidev(XenXC xc, domid_t dom, >>>>>> ioservid_t ioservid, >>>>>> + GHashTable *pci_bdf, >>>>>> PCIDevice *pci_dev, >>>>>> uint8_t oldbus) >>>>>> { >>>>>> - trace_xen_unmap_pcidev(ioservid, oldbus, >>>>>> - PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev- >>> devfn)); >>>>>> - xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, >>>>>> - 0, oldbus, >>>>>> - PCI_SLOT(pci_dev->devfn), >>>>>> - PCI_FUNC(pci_dev->devfn)); >>>>>> + gpointer bdf_key = GINT_TO_POINTER((oldbus << 8) | pci_dev- >>>>> devfn); >>>>>> + gint bdf_cnt = GPOINTER_TO_INT(g_hash_table_lookup(pci_bdf, >>>>>> bdf_key)); >>>>>> + >>>>>> + if (bdf_cnt == 1) { >>>>>> + trace_xen_unmap_pcidev(ioservid, oldbus, >>>>>> + PCI_SLOT(pci_dev->devfn), >>>>>> + PCI_FUNC(pci_dev->devfn)); >>>>>> + g_hash_table_remove(pci_bdf, bdf_key); >>>>>> + xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid, >>>>>> + 0, oldbus, >>>>>> + PCI_SLOT(pci_dev->devfn), >>>>>> + PCI_FUNC(pci_dev->devfn)); >>>>>> + } else { >>>>>> + trace_xen_unmap_pcidev_dup(ioservid, oldbus, >> PCI_SLOT(pci_dev- >>>>>>> devfn), >>>>>> + PCI_FUNC(pci_dev->devfn), bdf_cnt - 1); >>>>>> + g_hash_table_replace(pci_bdf, bdf_key, >>>> GINT_TO_POINTER(bdf_cnt - >>>>>> 1)); >>>>>> + } >>>>>> } >>>>>> >>>>>> static inline int xen_create_ioreq_server(XenXC xc, domid_t dom, >>>>>> diff --git a/trace-events b/trace-events >>>>>> index a589650..58deeaf 100644 >>>>>> --- a/trace-events >>>>>> +++ b/trace-events >>>>>> @@ -930,8 +930,10 @@ xen_map_mmio_range(uint32_t id, uint64_t >>>>>> start_addr, uint64_t end_addr) "id: %u >>>>>> xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t >>>>>> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 >>>>>> xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t >>>> end_addr) >>>>>> "id: %u start: %#"PRIx64" end: %#"PRIx64 >>>>>> xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t >>>>>> end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64 >>>>>> -xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) >> "id: >>>> %u >>>>>> bdf: %02x.%02x.%02x" >>>>>> -xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t >> func) >>>> "id: >>>>>> %u bdf: %02x.%02x.%02x" >>>>>> +xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) >> "id: >>>> %u >>>>>> bdf: %02x:%02x.%x" >>>>>> +xen_map_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, uint8_t >>>> func, >>>>>> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" >>>>>> +xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t >> func) >>>> "id: >>>>>> %u bdf: %02x:%02x.%x" >>>>>> +xen_unmap_pcidev_dup(uint32_t id, uint8_t bus, uint8_t dev, >> uint8_t >>>> func, >>>>>> int dup) "id: %u bdf: %02x:%02x.%x dup: %d" >>>>>> >>>>>> # xen-mapcache.c >>>>>> xen_map_cache(uint64_t phys_addr) "want %#"PRIx64 >>>>>> diff --git a/xen-hvm.c b/xen-hvm.c >>>>>> index 7b6d8f1..f041909 100644 >>>>>> --- a/xen-hvm.c >>>>>> +++ b/xen-hvm.c >>>>>> @@ -123,6 +123,7 @@ typedef struct XenIOState { >>>>>> MemoryListener memory_listener; >>>>>> MemoryListener io_listener; >>>>>> DeviceListener device_listener; >>>>>> + GHashTable *pci_bdf; >>>>>> QLIST_HEAD(, XenPhysmap) physmap; >>>>>> hwaddr free_phys_offset; >>>>>> const XenPhysmap *log_for_dirtybit; >>>>>> @@ -578,7 +579,8 @@ static void xen_device_realize(DeviceListener >>>>>> *listener, >>>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >>>>>> PCIDevice *pci_dev = PCI_DEVICE(dev); >>>>>> >>>>>> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); >>>>>> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- >>>>> pci_bdf, >>>>>> + pci_dev); >>>>>> } >>>>>> } >>>>>> >>>>>> @@ -590,8 +592,8 @@ static void xen_device_unrealize(DeviceListener >>>>>> *listener, >>>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { >>>>>> PCIDevice *pci_dev = PCI_DEVICE(dev); >>>>>> >>>>>> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, >>>>>> - pci_bus_num(pci_dev->bus)); >>>>>> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- >>>>>>> pci_bdf, >>>>>> + pci_dev, pci_bus_num(pci_dev->bus)); >>>>>> } >>>>>> } >>>>>> >>>>>> @@ -600,8 +602,10 @@ static void >>>>>> xen_device_change_pci_bus_num(DeviceListener *listener, >>>>>> { >>>>>> XenIOState *state = container_of(listener, XenIOState, >>>> device_listener); >>>>>> >>>>>> - xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev, >>>>>> oldbus); >>>>>> - xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev); >>>>>> + xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, state- >>>>> pci_bdf, >>>>>> + pci_dev, oldbus); >>>>>> + xen_map_pcidev(xen_xc, xen_domid, state->ioservid, state- >>>>> pci_bdf, >>>>>> + pci_dev); >>>>>> } >>>>>> >>>>>> static void xen_sync_dirty_bitmap(XenIOState *state, >>>>>> @@ -1318,6 +1322,7 @@ int xen_hvm_init(ram_addr_t >>>>>> *below_4g_mem_size, ram_addr_t *above_4g_mem_size, >>>>>> memory_listener_register(&state->io_listener, >> &address_space_io); >>>>>> >>>>>> state->device_listener = xen_device_listener; >>>>>> + state->pci_bdf = g_hash_table_new(NULL, NULL); >>>>>> device_listener_register(&state->device_listener); >>>>>> >>>>>> /* Initialize backend core & drivers */ >>>>>> -- >>>>>> 1.8.4 >>>>> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-08 21:18 [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Don Slutz ` (3 preceding siblings ...) 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server Don Slutz @ 2015-06-09 9:13 ` Michael S. Tsirkin 2015-06-09 9:18 ` Paul Durrant 4 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2015-06-09 9:13 UTC (permalink / raw) To: Don Slutz; +Cc: Paul Durrant, qemu-devel, Stefano Stabellini, xen-devel On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: > changes v1 to v2: > Split v1 patch into 3. > > Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion after > free"). > > Technically this bug fix should be a separate patch, however this > issue only seems to reproduce when this patch set is applied. > > > Michael S. Tsirkin: > "You need some other API that makes sense, probably PCI specific." > This is basically patch #2: "Extend device listener interface..." > > "This is relying on undocumented assumptions and how specific > firmware works. There's nothing special about bus number 255, > and 0 is not very special either (except it happens to be the > reset value)." > Dropped all checking of 0 and 255. > > > Open question by Michael S. Tsirkin: > > >>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: > ... > >>>> It is not clear to me that the complexity of tracking bus > >>>> visibility make sense. Clearly you do. > >>> Well what was the point of the change? > > > > To get config cycles that are valid that you do not get today. > > > >>> If you don't care that we get irrelevant config cycles why not > >>> just send them all to QEMU? > >>> > > > > That would need to be answered by Paul Durrant and/or other people (see > > below) > > > > We could broadcast config space ioreqs to all emulators, the problem > is how do we know (in the case of a read) which emulator is actually > the one supplying the data? At some level Xen needs to know who is > implementing what. > > Paul Durrant Can irrelevant emulators all respond with some kind of nack? Xen would pick the one that did respond correctly. > So this patch set just adjusts Xen to correctly know the current set > of PCI devices and their bus, device, and function. > > It does not attempt to calculate the visibility of the PCI devices. So non-visible devices will appear with invalid bus number values, conflicting with the visible devices. Seems wrong. > > v1: > > Note: Right now hvmloader in Xen does not handle PCI to PCI bridges > and so SeaBIOS does not have access to PCI device(s) on secondary > buses. How ever domU can setup the secondary bus(es) and this patch > will restore access to these PCI devices. > > > Don Slutz (4): > exec: Do not use MemoryRegion after free > Extend device listener interface for PCI to PCI bridges > xen: Add usage of device listener interface for PCI to PCI bridges > xen: Fix map/unmap of pcidev to ioreq server > > exec.c | 8 ++++-- > hw/core/qdev.c | 7 ++++++ > hw/pci/pci_bridge.c | 18 +++++++++++++ > include/hw/qdev-core.h | 3 +++ > include/hw/xen/xen_common.h | 61 ++++++++++++++++++++++++++++++++++----------- > trace-events | 6 +++-- > xen-hvm.c | 20 +++++++++++++-- > 7 files changed, 102 insertions(+), 21 deletions(-) > > -- > 1.8.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-09 9:13 ` [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Michael S. Tsirkin @ 2015-06-09 9:18 ` Paul Durrant 2015-06-09 10:52 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 9:18 UTC (permalink / raw) To: Michael S. Tsirkin, Don Slutz Cc: Stefano Stabellini, qemu-devel@nongnu.org, xen-devel@lists.xen.org > -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 09 June 2015 10:13 > To: Don Slutz > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paul Durrant; > Stefano Stabellini > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges > > On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: > > changes v1 to v2: > > Split v1 patch into 3. > > > > Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion after > > free"). > > > > Technically this bug fix should be a separate patch, however this > > issue only seems to reproduce when this patch set is applied. > > > > > > Michael S. Tsirkin: > > "You need some other API that makes sense, probably PCI specific." > > This is basically patch #2: "Extend device listener interface..." > > > > "This is relying on undocumented assumptions and how specific > > firmware works. There's nothing special about bus number 255, > > and 0 is not very special either (except it happens to be the > > reset value)." > > Dropped all checking of 0 and 255. > > > > > > Open question by Michael S. Tsirkin: > > > > >>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: > > ... > > >>>> It is not clear to me that the complexity of tracking bus > > >>>> visibility make sense. Clearly you do. > > >>> Well what was the point of the change? > > > > > > To get config cycles that are valid that you do not get today. > > > > > >>> If you don't care that we get irrelevant config cycles why not > > >>> just send them all to QEMU? > > >>> > > > > > > That would need to be answered by Paul Durrant and/or other people > (see > > > below) > > > > > > > We could broadcast config space ioreqs to all emulators, the problem > > is how do we know (in the case of a read) which emulator is actually > > the one supplying the data? At some level Xen needs to know who is > > implementing what. > > > > Paul Durrant > > Can irrelevant emulators all respond with some kind of nack? > Xen would pick the one that did respond correctly. > I guess that's possible if we add an extra bit to the ioreq_t, but QEMU would still need to know when to nack and when to ack. It's still much simpler if qemu updates Xen with exact set of (S)BDFs it's handling. Paul > > So this patch set just adjusts Xen to correctly know the current set > > of PCI devices and their bus, device, and function. > > > > It does not attempt to calculate the visibility of the PCI devices. > > So non-visible devices will appear with invalid bus number values, > conflicting with the visible devices. > Seems wrong. > > > > > > v1: > > > > Note: Right now hvmloader in Xen does not handle PCI to PCI bridges > > and so SeaBIOS does not have access to PCI device(s) on secondary > > buses. How ever domU can setup the secondary bus(es) and this patch > > will restore access to these PCI devices. > > > > > > Don Slutz (4): > > exec: Do not use MemoryRegion after free > > Extend device listener interface for PCI to PCI bridges > > xen: Add usage of device listener interface for PCI to PCI bridges > > xen: Fix map/unmap of pcidev to ioreq server > > > > exec.c | 8 ++++-- > > hw/core/qdev.c | 7 ++++++ > > hw/pci/pci_bridge.c | 18 +++++++++++++ > > include/hw/qdev-core.h | 3 +++ > > include/hw/xen/xen_common.h | 61 > ++++++++++++++++++++++++++++++++++----------- > > trace-events | 6 +++-- > > xen-hvm.c | 20 +++++++++++++-- > > 7 files changed, 102 insertions(+), 21 deletions(-) > > > > -- > > 1.8.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-09 9:18 ` Paul Durrant @ 2015-06-09 10:52 ` Michael S. Tsirkin 2015-06-09 10:58 ` Paul Durrant 0 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2015-06-09 10:52 UTC (permalink / raw) To: Paul Durrant Cc: Stefano Stabellini, qemu-devel@nongnu.org, Don Slutz, xen-devel@lists.xen.org On Tue, Jun 09, 2015 at 09:18:49AM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: 09 June 2015 10:13 > > To: Don Slutz > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paul Durrant; > > Stefano Stabellini > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges > > > > On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: > > > changes v1 to v2: > > > Split v1 patch into 3. > > > > > > Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion after > > > free"). > > > > > > Technically this bug fix should be a separate patch, however this > > > issue only seems to reproduce when this patch set is applied. > > > > > > > > > Michael S. Tsirkin: > > > "You need some other API that makes sense, probably PCI specific." > > > This is basically patch #2: "Extend device listener interface..." > > > > > > "This is relying on undocumented assumptions and how specific > > > firmware works. There's nothing special about bus number 255, > > > and 0 is not very special either (except it happens to be the > > > reset value)." > > > Dropped all checking of 0 and 255. > > > > > > > > > Open question by Michael S. Tsirkin: > > > > > > >>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: > > > ... > > > >>>> It is not clear to me that the complexity of tracking bus > > > >>>> visibility make sense. Clearly you do. > > > >>> Well what was the point of the change? > > > > > > > > To get config cycles that are valid that you do not get today. > > > > > > > >>> If you don't care that we get irrelevant config cycles why not > > > >>> just send them all to QEMU? > > > >>> > > > > > > > > That would need to be answered by Paul Durrant and/or other people > > (see > > > > below) > > > > > > > > > > We could broadcast config space ioreqs to all emulators, the problem > > > is how do we know (in the case of a read) which emulator is actually > > > the one supplying the data? At some level Xen needs to know who is > > > implementing what. > > > > > > Paul Durrant > > > > Can irrelevant emulators all respond with some kind of nack? > > Xen would pick the one that did respond correctly. > > > > I guess that's possible if we add an extra bit to the ioreq_t, but QEMU would still need to know when to nack and when to ack. It's simple: ack if we find a device handling the specific BDF. The result would at least be contained. OTOH detecting master aborts in core is useful since it would let us implement error reporting correctly. > It's still much simpler if qemu updates Xen with exact set of (S)BDFs it's handling. > > Paul I suspect this calls for a bigger change, you need to re-scan all of the tree to detect visible devices. Maybe just write some xen-specific code to do this on each config access. > > > So this patch set just adjusts Xen to correctly know the current set > > > of PCI devices and their bus, device, and function. > > > > > > It does not attempt to calculate the visibility of the PCI devices. > > > > So non-visible devices will appear with invalid bus number values, > > conflicting with the visible devices. > > Seems wrong. > > > > > > > > > > v1: > > > > > > Note: Right now hvmloader in Xen does not handle PCI to PCI bridges > > > and so SeaBIOS does not have access to PCI device(s) on secondary > > > buses. How ever domU can setup the secondary bus(es) and this patch > > > will restore access to these PCI devices. > > > > > > > > > Don Slutz (4): > > > exec: Do not use MemoryRegion after free > > > Extend device listener interface for PCI to PCI bridges > > > xen: Add usage of device listener interface for PCI to PCI bridges > > > xen: Fix map/unmap of pcidev to ioreq server > > > > > > exec.c | 8 ++++-- > > > hw/core/qdev.c | 7 ++++++ > > > hw/pci/pci_bridge.c | 18 +++++++++++++ > > > include/hw/qdev-core.h | 3 +++ > > > include/hw/xen/xen_common.h | 61 > > ++++++++++++++++++++++++++++++++++----------- > > > trace-events | 6 +++-- > > > xen-hvm.c | 20 +++++++++++++-- > > > 7 files changed, 102 insertions(+), 21 deletions(-) > > > > > > -- > > > 1.8.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-09 10:52 ` Michael S. Tsirkin @ 2015-06-09 10:58 ` Paul Durrant 2015-06-09 12:29 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 10:58 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stefano Stabellini, qemu-devel@nongnu.org, Don Slutz, xen-devel@lists.xen.org > -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 09 June 2015 11:52 > To: Paul Durrant > Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano > Stabellini > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges > > On Tue, Jun 09, 2015 at 09:18:49AM +0000, Paul Durrant wrote: > > > -----Original Message----- > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > Sent: 09 June 2015 10:13 > > > To: Don Slutz > > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paul Durrant; > > > Stefano Stabellini > > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI > bridges > > > > > > On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: > > > > changes v1 to v2: > > > > Split v1 patch into 3. > > > > > > > > Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion after > > > > free"). > > > > > > > > Technically this bug fix should be a separate patch, however this > > > > issue only seems to reproduce when this patch set is applied. > > > > > > > > > > > > Michael S. Tsirkin: > > > > "You need some other API that makes sense, probably PCI specific." > > > > This is basically patch #2: "Extend device listener interface..." > > > > > > > > "This is relying on undocumented assumptions and how specific > > > > firmware works. There's nothing special about bus number 255, > > > > and 0 is not very special either (except it happens to be the > > > > reset value)." > > > > Dropped all checking of 0 and 255. > > > > > > > > > > > > Open question by Michael S. Tsirkin: > > > > > > > > >>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: > > > > ... > > > > >>>> It is not clear to me that the complexity of tracking bus > > > > >>>> visibility make sense. Clearly you do. > > > > >>> Well what was the point of the change? > > > > > > > > > > To get config cycles that are valid that you do not get today. > > > > > > > > > >>> If you don't care that we get irrelevant config cycles why not > > > > >>> just send them all to QEMU? > > > > >>> > > > > > > > > > > That would need to be answered by Paul Durrant and/or other > people > > > (see > > > > > below) > > > > > > > > > > > > > We could broadcast config space ioreqs to all emulators, the problem > > > > is how do we know (in the case of a read) which emulator is actually > > > > the one supplying the data? At some level Xen needs to know who is > > > > implementing what. > > > > > > > > Paul Durrant > > > > > > Can irrelevant emulators all respond with some kind of nack? > > > Xen would pick the one that did respond correctly. > > > > > > > I guess that's possible if we add an extra bit to the ioreq_t, but QEMU > would still need to know when to nack and when to ack. > > It's simple: ack if we find a device handling the specific BDF. > The result would at least be contained. > OTOH detecting master aborts in core is useful since it would > let us implement error reporting correctly. > > > > It's still much simpler if qemu updates Xen with exact set of (S)BDFs it's > handling. > > > > Paul > > > I suspect this calls for a bigger change, you need to re-scan > all of the tree to detect visible devices. > Maybe just write some xen-specific code to do this on each > config access. Well, that's the thing really... what triggers the re-scan. Do we really need to scan on each access or is there a way to know when the topology is changed? Doing the former doesn't really sound wonderfully efficient and, if the answer to the second is yes, then that's the time to update Xen with the currently valid set of BDFs. Paul > > > > > > So this patch set just adjusts Xen to correctly know the current set > > > > of PCI devices and their bus, device, and function. > > > > > > > > It does not attempt to calculate the visibility of the PCI devices. > > > > > > So non-visible devices will appear with invalid bus number values, > > > conflicting with the visible devices. > > > Seems wrong. > > > > > > > > > > > > > > v1: > > > > > > > > Note: Right now hvmloader in Xen does not handle PCI to PCI bridges > > > > and so SeaBIOS does not have access to PCI device(s) on secondary > > > > buses. How ever domU can setup the secondary bus(es) and this patch > > > > will restore access to these PCI devices. > > > > > > > > > > > > Don Slutz (4): > > > > exec: Do not use MemoryRegion after free > > > > Extend device listener interface for PCI to PCI bridges > > > > xen: Add usage of device listener interface for PCI to PCI bridges > > > > xen: Fix map/unmap of pcidev to ioreq server > > > > > > > > exec.c | 8 ++++-- > > > > hw/core/qdev.c | 7 ++++++ > > > > hw/pci/pci_bridge.c | 18 +++++++++++++ > > > > include/hw/qdev-core.h | 3 +++ > > > > include/hw/xen/xen_common.h | 61 > > > ++++++++++++++++++++++++++++++++++----------- > > > > trace-events | 6 +++-- > > > > xen-hvm.c | 20 +++++++++++++-- > > > > 7 files changed, 102 insertions(+), 21 deletions(-) > > > > > > > > -- > > > > 1.8.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-09 10:58 ` Paul Durrant @ 2015-06-09 12:29 ` Michael S. Tsirkin 2015-06-09 14:14 ` Paul Durrant 0 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2015-06-09 12:29 UTC (permalink / raw) To: Paul Durrant Cc: Stefano Stabellini, qemu-devel@nongnu.org, Don Slutz, xen-devel@lists.xen.org On Tue, Jun 09, 2015 at 10:58:26AM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: 09 June 2015 11:52 > > To: Paul Durrant > > Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano > > Stabellini > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges > > > > On Tue, Jun 09, 2015 at 09:18:49AM +0000, Paul Durrant wrote: > > > > -----Original Message----- > > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > > Sent: 09 June 2015 10:13 > > > > To: Don Slutz > > > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paul Durrant; > > > > Stefano Stabellini > > > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI > > bridges > > > > > > > > On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: > > > > > changes v1 to v2: > > > > > Split v1 patch into 3. > > > > > > > > > > Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion after > > > > > free"). > > > > > > > > > > Technically this bug fix should be a separate patch, however this > > > > > issue only seems to reproduce when this patch set is applied. > > > > > > > > > > > > > > > Michael S. Tsirkin: > > > > > "You need some other API that makes sense, probably PCI specific." > > > > > This is basically patch #2: "Extend device listener interface..." > > > > > > > > > > "This is relying on undocumented assumptions and how specific > > > > > firmware works. There's nothing special about bus number 255, > > > > > and 0 is not very special either (except it happens to be the > > > > > reset value)." > > > > > Dropped all checking of 0 and 255. > > > > > > > > > > > > > > > Open question by Michael S. Tsirkin: > > > > > > > > > > >>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: > > > > > ... > > > > > >>>> It is not clear to me that the complexity of tracking bus > > > > > >>>> visibility make sense. Clearly you do. > > > > > >>> Well what was the point of the change? > > > > > > > > > > > > To get config cycles that are valid that you do not get today. > > > > > > > > > > > >>> If you don't care that we get irrelevant config cycles why not > > > > > >>> just send them all to QEMU? > > > > > >>> > > > > > > > > > > > > That would need to be answered by Paul Durrant and/or other > > people > > > > (see > > > > > > below) > > > > > > > > > > > > > > > > We could broadcast config space ioreqs to all emulators, the problem > > > > > is how do we know (in the case of a read) which emulator is actually > > > > > the one supplying the data? At some level Xen needs to know who is > > > > > implementing what. > > > > > > > > > > Paul Durrant > > > > > > > > Can irrelevant emulators all respond with some kind of nack? > > > > Xen would pick the one that did respond correctly. > > > > > > > > > > I guess that's possible if we add an extra bit to the ioreq_t, but QEMU > > would still need to know when to nack and when to ack. > > > > It's simple: ack if we find a device handling the specific BDF. > > The result would at least be contained. > > OTOH detecting master aborts in core is useful since it would > > let us implement error reporting correctly. > > > > > > > It's still much simpler if qemu updates Xen with exact set of (S)BDFs it's > > handling. > > > > > > Paul > > > > > > I suspect this calls for a bigger change, you need to re-scan > > all of the tree to detect visible devices. > > Maybe just write some xen-specific code to do this on each > > config access. > > Well, that's the thing really... what triggers the re-scan. Do we really need to scan on each access or is there a way to know when the topology is changed? Doing the former doesn't really sound wonderfully efficient and, if the answer to the second is yes, then that's the time to update Xen with the currently valid set of BDFs. > > Paul Several things can trigger a topology change. One other option is switching to a memory API for config accesses, then using a memory listener to detect topology changes. That would be a lot of work I'm afraid. -- MST ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-09 12:29 ` Michael S. Tsirkin @ 2015-06-09 14:14 ` Paul Durrant 2015-06-09 14:27 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Paul Durrant @ 2015-06-09 14:14 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stefano Stabellini, qemu-devel@nongnu.org, Don Slutz, xen-devel@lists.xen.org > -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 09 June 2015 13:30 > To: Paul Durrant > Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano > Stabellini > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges > > On Tue, Jun 09, 2015 at 10:58:26AM +0000, Paul Durrant wrote: > > > -----Original Message----- > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > Sent: 09 June 2015 11:52 > > > To: Paul Durrant > > > Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; > Stefano > > > Stabellini > > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI > bridges > > > > > > On Tue, Jun 09, 2015 at 09:18:49AM +0000, Paul Durrant wrote: > > > > > -----Original Message----- > > > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > > > Sent: 09 June 2015 10:13 > > > > > To: Don Slutz > > > > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paul Durrant; > > > > > Stefano Stabellini > > > > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI > > > bridges > > > > > > > > > > On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: > > > > > > changes v1 to v2: > > > > > > Split v1 patch into 3. > > > > > > > > > > > > Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion > after > > > > > > free"). > > > > > > > > > > > > Technically this bug fix should be a separate patch, however this > > > > > > issue only seems to reproduce when this patch set is applied. > > > > > > > > > > > > > > > > > > Michael S. Tsirkin: > > > > > > "You need some other API that makes sense, probably PCI > specific." > > > > > > This is basically patch #2: "Extend device listener interface..." > > > > > > > > > > > > "This is relying on undocumented assumptions and how specific > > > > > > firmware works. There's nothing special about bus number 255, > > > > > > and 0 is not very special either (except it happens to be the > > > > > > reset value)." > > > > > > Dropped all checking of 0 and 255. > > > > > > > > > > > > > > > > > > Open question by Michael S. Tsirkin: > > > > > > > > > > > > >>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: > > > > > > ... > > > > > > >>>> It is not clear to me that the complexity of tracking bus > > > > > > >>>> visibility make sense. Clearly you do. > > > > > > >>> Well what was the point of the change? > > > > > > > > > > > > > > To get config cycles that are valid that you do not get today. > > > > > > > > > > > > > >>> If you don't care that we get irrelevant config cycles why not > > > > > > >>> just send them all to QEMU? > > > > > > >>> > > > > > > > > > > > > > > That would need to be answered by Paul Durrant and/or other > > > people > > > > > (see > > > > > > > below) > > > > > > > > > > > > > > > > > > > We could broadcast config space ioreqs to all emulators, the > problem > > > > > > is how do we know (in the case of a read) which emulator is actually > > > > > > the one supplying the data? At some level Xen needs to know who > is > > > > > > implementing what. > > > > > > > > > > > > Paul Durrant > > > > > > > > > > Can irrelevant emulators all respond with some kind of nack? > > > > > Xen would pick the one that did respond correctly. > > > > > > > > > > > > > I guess that's possible if we add an extra bit to the ioreq_t, but QEMU > > > would still need to know when to nack and when to ack. > > > > > > It's simple: ack if we find a device handling the specific BDF. > > > The result would at least be contained. > > > OTOH detecting master aborts in core is useful since it would > > > let us implement error reporting correctly. > > > > > > > > > > It's still much simpler if qemu updates Xen with exact set of (S)BDFs it's > > > handling. > > > > > > > > Paul > > > > > > > > > I suspect this calls for a bigger change, you need to re-scan > > > all of the tree to detect visible devices. > > > Maybe just write some xen-specific code to do this on each > > > config access. > > > > Well, that's the thing really... what triggers the re-scan. Do we really need > to scan on each access or is there a way to know when the topology is > changed? Doing the former doesn't really sound wonderfully efficient and, if > the answer to the second is yes, then that's the time to update Xen with the > currently valid set of BDFs. > > > > Paul > > > Several things can trigger a topology change. Well, IMO those need to be enumerated and suitable hooks need to be put in place so that Xen can be informed of the changes. Paul > One other option is switching to a memory API > for config accesses, then using a memory listener to detect > topology changes. That would be a lot of work I'm afraid. > > -- > MST ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-09 14:14 ` Paul Durrant @ 2015-06-09 14:27 ` Michael S. Tsirkin 2015-06-09 15:10 ` Don Slutz 0 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2015-06-09 14:27 UTC (permalink / raw) To: Paul Durrant Cc: Stefano Stabellini, qemu-devel@nongnu.org, Don Slutz, xen-devel@lists.xen.org On Tue, Jun 09, 2015 at 02:14:29PM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: 09 June 2015 13:30 > > To: Paul Durrant > > Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano > > Stabellini > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges > > > > On Tue, Jun 09, 2015 at 10:58:26AM +0000, Paul Durrant wrote: > > > > -----Original Message----- > > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > > Sent: 09 June 2015 11:52 > > > > To: Paul Durrant > > > > Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; > > Stefano > > > > Stabellini > > > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI > > bridges > > > > > > > > On Tue, Jun 09, 2015 at 09:18:49AM +0000, Paul Durrant wrote: > > > > > > -----Original Message----- > > > > > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > > > > > Sent: 09 June 2015 10:13 > > > > > > To: Don Slutz > > > > > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paul Durrant; > > > > > > Stefano Stabellini > > > > > > Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI > > > > bridges > > > > > > > > > > > > On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: > > > > > > > changes v1 to v2: > > > > > > > Split v1 patch into 3. > > > > > > > > > > > > > > Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion > > after > > > > > > > free"). > > > > > > > > > > > > > > Technically this bug fix should be a separate patch, however this > > > > > > > issue only seems to reproduce when this patch set is applied. > > > > > > > > > > > > > > > > > > > > > Michael S. Tsirkin: > > > > > > > "You need some other API that makes sense, probably PCI > > specific." > > > > > > > This is basically patch #2: "Extend device listener interface..." > > > > > > > > > > > > > > "This is relying on undocumented assumptions and how specific > > > > > > > firmware works. There's nothing special about bus number 255, > > > > > > > and 0 is not very special either (except it happens to be the > > > > > > > reset value)." > > > > > > > Dropped all checking of 0 and 255. > > > > > > > > > > > > > > > > > > > > > Open question by Michael S. Tsirkin: > > > > > > > > > > > > > > >>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: > > > > > > > ... > > > > > > > >>>> It is not clear to me that the complexity of tracking bus > > > > > > > >>>> visibility make sense. Clearly you do. > > > > > > > >>> Well what was the point of the change? > > > > > > > > > > > > > > > > To get config cycles that are valid that you do not get today. > > > > > > > > > > > > > > > >>> If you don't care that we get irrelevant config cycles why not > > > > > > > >>> just send them all to QEMU? > > > > > > > >>> > > > > > > > > > > > > > > > > That would need to be answered by Paul Durrant and/or other > > > > people > > > > > > (see > > > > > > > > below) > > > > > > > > > > > > > > > > > > > > > > We could broadcast config space ioreqs to all emulators, the > > problem > > > > > > > is how do we know (in the case of a read) which emulator is actually > > > > > > > the one supplying the data? At some level Xen needs to know who > > is > > > > > > > implementing what. > > > > > > > > > > > > > > Paul Durrant > > > > > > > > > > > > Can irrelevant emulators all respond with some kind of nack? > > > > > > Xen would pick the one that did respond correctly. > > > > > > > > > > > > > > > > I guess that's possible if we add an extra bit to the ioreq_t, but QEMU > > > > would still need to know when to nack and when to ack. > > > > > > > > It's simple: ack if we find a device handling the specific BDF. > > > > The result would at least be contained. > > > > OTOH detecting master aborts in core is useful since it would > > > > let us implement error reporting correctly. > > > > > > > > > > > > > It's still much simpler if qemu updates Xen with exact set of (S)BDFs it's > > > > handling. > > > > > > > > > > Paul > > > > > > > > > > > > I suspect this calls for a bigger change, you need to re-scan > > > > all of the tree to detect visible devices. > > > > Maybe just write some xen-specific code to do this on each > > > > config access. > > > > > > Well, that's the thing really... what triggers the re-scan. Do we really need > > to scan on each access or is there a way to know when the topology is > > changed? Doing the former doesn't really sound wonderfully efficient and, if > > the answer to the second is yes, then that's the time to update Xen with the > > currently valid set of BDFs. > > > > > > Paul > > > > > > Several things can trigger a topology change. > > Well, IMO those need to be enumerated and suitable hooks need to be put in place so that Xen can be informed of the changes. > > Paul pci_data_write and pci_data_read already do bus lookups. It seems that returning a nack there would just require hooks in these two places. > > One other option is switching to a memory API > > for config accesses, then using a memory listener to detect > > topology changes. That would be a lot of work I'm afraid. > > > > -- > > MST ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges 2015-06-09 14:27 ` Michael S. Tsirkin @ 2015-06-09 15:10 ` Don Slutz 0 siblings, 0 replies; 23+ messages in thread From: Don Slutz @ 2015-06-09 15:10 UTC (permalink / raw) To: Michael S. Tsirkin, Paul Durrant Cc: Stefano Stabellini, qemu-devel@nongnu.org, Slutz, Donald Christopher, xen-devel@lists.xen.org On 06/09/15 10:27, Michael S. Tsirkin wrote: > On Tue, Jun 09, 2015 at 02:14:29PM +0000, Paul Durrant wrote: >>> -----Original Message----- >>> From: Michael S. Tsirkin [mailto:mst@redhat.com] >>> Sent: 09 June 2015 13:30 >>> To: Paul Durrant >>> Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; Stefano >>> Stabellini >>> Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges >>> >>> On Tue, Jun 09, 2015 at 10:58:26AM +0000, Paul Durrant wrote: >>>>> -----Original Message----- >>>>> From: Michael S. Tsirkin [mailto:mst@redhat.com] >>>>> Sent: 09 June 2015 11:52 >>>>> To: Paul Durrant >>>>> Cc: Don Slutz; qemu-devel@nongnu.org; xen-devel@lists.xen.org; >>> Stefano >>>>> Stabellini >>>>> Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI >>> bridges >>>>> >>>>> On Tue, Jun 09, 2015 at 09:18:49AM +0000, Paul Durrant wrote: >>>>>>> -----Original Message----- >>>>>>> From: Michael S. Tsirkin [mailto:mst@redhat.com] >>>>>>> Sent: 09 June 2015 10:13 >>>>>>> To: Don Slutz >>>>>>> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; Paul Durrant; >>>>>>> Stefano Stabellini >>>>>>> Subject: Re: [PATCH v2 0/4] Fix device listener interface for PCI to PCI >>>>> bridges >>>>>>> >>>>>>> On Mon, Jun 08, 2015 at 05:18:48PM -0400, Don Slutz wrote: >>>>>>>> changes v1 to v2: >>>>>>>> Split v1 patch into 3. >>>>>>>> >>>>>>>> Added a BUG FIX patch (#1: "exec: Do not use MemoryRegion >>> after >>>>>>>> free"). >>>>>>>> >>>>>>>> Technically this bug fix should be a separate patch, however this >>>>>>>> issue only seems to reproduce when this patch set is applied. >>>>>>>> >>>>>>>> >>>>>>>> Michael S. Tsirkin: >>>>>>>> "You need some other API that makes sense, probably PCI >>> specific." >>>>>>>> This is basically patch #2: "Extend device listener interface..." >>>>>>>> >>>>>>>> "This is relying on undocumented assumptions and how specific >>>>>>>> firmware works. There's nothing special about bus number 255, >>>>>>>> and 0 is not very special either (except it happens to be the >>>>>>>> reset value)." >>>>>>>> Dropped all checking of 0 and 255. >>>>>>>> >>>>>>>> >>>>>>>> Open question by Michael S. Tsirkin: >>>>>>>> >>>>>>>>>>>> On Thu, May 28, 2015 at 07:25:50AM -0400, Don Slutz wrote: >>>>>>>> ... >>>>>>>>>>>> It is not clear to me that the complexity of tracking bus >>>>>>>>>>>> visibility make sense. Clearly you do. >>>>>>>>>>> Well what was the point of the change? >>>>>>>>> >>>>>>>>> To get config cycles that are valid that you do not get today. >>>>>>>>> >>>>>>>>>>> If you don't care that we get irrelevant config cycles why not >>>>>>>>>>> just send them all to QEMU? >>>>>>>>>>> >>>>>>>>> >>>>>>>>> That would need to be answered by Paul Durrant and/or other >>>>> people >>>>>>> (see >>>>>>>>> below) >>>>>>>>> >>>>>>>> >>>>>>>> We could broadcast config space ioreqs to all emulators, the >>> problem >>>>>>>> is how do we know (in the case of a read) which emulator is actually >>>>>>>> the one supplying the data? At some level Xen needs to know who >>> is >>>>>>>> implementing what. >>>>>>>> >>>>>>>> Paul Durrant >>>>>>> >>>>>>> Can irrelevant emulators all respond with some kind of nack? >>>>>>> Xen would pick the one that did respond correctly. >>>>>>> >>>>>> >>>>>> I guess that's possible if we add an extra bit to the ioreq_t, but QEMU >>>>> would still need to know when to nack and when to ack. >>>>> >>>>> It's simple: ack if we find a device handling the specific BDF. >>>>> The result would at least be contained. >>>>> OTOH detecting master aborts in core is useful since it would >>>>> let us implement error reporting correctly. >>>>> >>>>> >>>>>> It's still much simpler if qemu updates Xen with exact set of (S)BDFs it's >>>>> handling. >>>>>> >>>>>> Paul >>>>> >>>>> >>>>> I suspect this calls for a bigger change, you need to re-scan >>>>> all of the tree to detect visible devices. >>>>> Maybe just write some xen-specific code to do this on each >>>>> config access. >>>> >>>> Well, that's the thing really... what triggers the re-scan. Do we really need >>> to scan on each access or is there a way to know when the topology is >>> changed? Doing the former doesn't really sound wonderfully efficient and, if >>> the answer to the second is yes, then that's the time to update Xen with the >>> currently valid set of BDFs. >>>> >>>> Paul >>> >>> >>> Several things can trigger a topology change. >> >> Well, IMO those need to be enumerated and suitable hooks need to be put in place so that Xen can be informed of the changes. >> This patch adds one of the hooks (the one that lets Xen know of the change to BDF). The hook that can report on the accessibility of a given PCI device is much more complex since there is no simple way to say "PCI device pvscsi #2 is not accessible" since there can be multiple PCI devices with the same name and they can start out all having the BDF. One would need to invent a way to describe the PCI tree and pass that around. >> Paul > > pci_data_write and pci_data_read already do bus lookups. > It seems that returning a nack there would just require > hooks in these two places. > Yes, the answer of "can QEMU can reach a BDF right now" is simple. And it may make sense to add. Returning a set of reachable BDFs is much harder. I went with returning the set of BDFs that may be reachable. I.E. every BDF registered with Xen should be sent to this QEMU. At startup there will be ones that either generate nacks or are reached. Proper PCI bridge scanning code (which is missing from hvmloader) will attempt to assign a non-overlapping set of BDF for all PCI devices found. With enough PCI to PCI bridges (greater then 255) this is not possible and I have no idea how many things would not work in this case. I would hope that QEMU would work, but I do not know if this has ever be tested. However guest code can (and will) at times do things like hiding a secondary PCI bus and all PCI devices that can be reached via it. The current interface with Xen does not support the root bus vs non-root bus information. Nor does it have PCI device tree structure. All you have to work with is a set of BDFs. -Don Slutz >>> One other option is switching to a memory API >>> for config accesses, then using a memory listener to detect >>> topology changes. That would be a lot of work I'm afraid. >>> >>> -- >>> MST ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2015-06-09 16:40 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-08 21:18 [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [BUGFIX][PATCH v2 1/4] exec: Do not use MemoryRegion after free Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges Don Slutz 2015-06-09 9:08 ` Paul Durrant 2015-06-09 13:53 ` Don Slutz 2015-06-09 13:55 ` Paul Durrant 2015-06-09 14:00 ` Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 3/4] xen: Add usage of " Don Slutz 2015-06-08 21:18 ` [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server Don Slutz 2015-06-09 9:05 ` Paul Durrant 2015-06-09 13:51 ` Don Slutz 2015-06-09 14:03 ` Paul Durrant 2015-06-09 14:28 ` Don Slutz 2015-06-09 15:11 ` Paul Durrant 2015-06-09 16:40 ` Don Slutz 2015-06-09 9:13 ` [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges Michael S. Tsirkin 2015-06-09 9:18 ` Paul Durrant 2015-06-09 10:52 ` Michael S. Tsirkin 2015-06-09 10:58 ` Paul Durrant 2015-06-09 12:29 ` Michael S. Tsirkin 2015-06-09 14:14 ` Paul Durrant 2015-06-09 14:27 ` Michael S. Tsirkin 2015-06-09 15:10 ` Don Slutz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).