[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq se
From: |
Paul Durrant |
Subject: |
Re: [Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server |
Date: |
Tue, 9 Jun 2015 14:03:10 +0000 |
> -----Original Message-----
> From: Don Slutz [mailto:address@hidden
> Sent: 09 June 2015 14:51
> To: Paul Durrant; Slutz, Donald Christopher; address@hidden; xen-
> address@hidden
> 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:address@hidden
> >> Sent: 08 June 2015 22:19
> >> To: address@hidden; address@hidden
> >> 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 <address@hidden>
> >> CC: Don Slutz <address@hidden>
> >> ---
> >> 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
> >
- [Qemu-devel] [BUGFIX][PATCH v2 1/4] exec: Do not use MemoryRegion after free, (continued)
[Qemu-devel] [PATCH v2 4/4] xen: Fix map/unmap of pcidev to ioreq server, Don Slutz, 2015/06/08
Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges, Michael S. Tsirkin, 2015/06/09
- Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges, Paul Durrant, 2015/06/09
- Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges, Michael S. Tsirkin, 2015/06/09
- Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges, Paul Durrant, 2015/06/09
- Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges, Michael S. Tsirkin, 2015/06/09
- Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges, Paul Durrant, 2015/06/09
- Re: [Qemu-devel] [PATCH v2 0/4] Fix device listener interface for PCI to PCI bridges, Michael S. Tsirkin, 2015/06/09