Re: [PATCH] hw/xen: Remove deadcode
On Tue, Sep 17, 2024 at 01:22:12AM +0100, d...@treblig.org wrote: > From: "Dr. David Alan Gilbert" > > xen_be_copy_grant_refs is unused since 2019's > 19f87870ba ("xen: remove the legacy 'xen_disk' backend") > > xen_config_dev_console is unused since 2018's > 6d7c06c213 ("Remove broken Xen PV domain builder") > > Remove them. > > Signed-off-by: Dr. David Alan Gilbert Acked-by: Anthony PERARD Thanks, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [PATCH v3 3/3] Do not access /dev/mem in MSI-X PCI passthrough on Xen
On Mon, May 06, 2024 at 02:33:22AM +0200, Marek Marczykowski-Górecki wrote: > diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c > index 09cca4e..836cc9c 100644 > --- a/hw/xen/xen_pt_msi.c > +++ b/hw/xen/xen_pt_msi.c > @@ -493,7 +501,12 @@ static uint64_t pci_msix_read(void *opaque, hwaddr addr, > return get_entry_value(&msix->msix_entry[entry_nr], offset); > } else { > /* Pending Bit Array (PBA) */ > -return *(uint32_t *)(msix->phys_iomem_base + addr); > +if (s->msix->phys_iomem_base) { > +return *(uint32_t *)(msix->phys_iomem_base + addr); > +} > +XEN_PT_LOG(&s->dev, "reading PBA, addr 0x%lx, offset 0x%lx\n", > + addr, addr - msix->total_entries * PCI_MSIX_ENTRY_SIZE); > +return 0x; If Xen advertise XENFEAT_dm_msix_all_writes, we are not expecting QEMU to reach this code, right? A comment might be useful. > } > } > > @@ -576,33 +593,40 @@ int xen_pt_msix_init(XenPCIPassthroughState *s, > uint32_t base) > msix->table_base = s->real_device.io_regions[bar_index].base_addr; > XEN_PT_LOG(d, "get MSI-X table BAR base 0x%"PRIx64"\n", > msix->table_base); > > +/* Accessing /dev/mem is needed only on older Xen. */ > +if (!(xc_version_info.submap & (1U << XENFEAT_dm_msix_all_writes))) { Would it be ok to use test_bit() instead? Thanks, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [PATCH v3 2/3] Update Xen's features.h header
On Mon, May 06, 2024 at 02:33:21AM +0200, Marek Marczykowski-Górecki wrote: > Update it to get XENFEAT_dm_msix_all_writes for the next patch. > > Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Anthony PERARD Thanks, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [PATCH v3 1/3] hw/xen/xen_pt: Save back data only for declared registers
On Mon, May 06, 2024 at 02:33:20AM +0200, Marek Marczykowski-Górecki wrote: > Call pci_default_write_config() only after resolving any handlers from > XenPTRegInfo structures, and only with a value updated with those > handlers. This is important for two reasons: > 1. XenPTRegInfo has ro_mask which needs to be enforced - Xen-specific >hooks do that on their own (especially xen_pt_*_reg_write()). > 2. Not setting value early allows hooks to see the old value too. > > If it would be only about the first point, setting PCIDevice.wmask would > probably be sufficient, but given the second point, change those > writes. > > Relevant handlers already save data back to the emulated registers > space, call the pci_default_write_config() only for its side effects. > > Signed-off-by: Marek Marczykowski-Górecki > --- > v3: > - use emulated register value for pci_default_write_config() call, not >the one for writting back to the hardware > - greatly simplify the patch by calling pci_default_write_config() on >the whole value > v2: > - rewrite commit message, previous one was very misleading > - fix loop saving register values > - fix int overflow when calculating write mask Reviewed-by: Anthony PERARD Thanks, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [PATCH] MAINTAINERS: add Edgar as Xen maintainer
On Wed, Jul 10, 2024 at 01:28:52PM -0700, Stefano Stabellini wrote: > Add Edgar as Xen subsystem maintainer in QEMU. Edgar has been a QEMU > maintainer for years, and has already made key changes to one of the > most difficult areas of the Xen subsystem (the mapcache). > > Edgar volunteered helping us maintain the Xen subsystem in QEMU and we > are very happy to welcome him to the team. His knowledge and expertise > with QEMU internals will be of great help. > > Signed-off-by: Stefano Stabellini > > diff --git a/MAINTAINERS b/MAINTAINERS > index 6725913c8b..63e11095a2 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -536,6 +536,7 @@ X86 Xen CPUs > M: Stefano Stabellini > M: Anthony PERARD > M: Paul Durrant > +M: Edgar E. Iglesias > L: xen-de...@lists.xenproject.org > S: Supported > F: */xen* Acked-by: Anthony PERARD Welcome! Cheers, -- Anthony PERARD
Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets
On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > This fixes the clobbering of the entry->next pointer when > unmapping the first entry in a bucket of a mapcache. > > Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets") > Reported-by: Anthony PERARD > Signed-off-by: Edgar E. Iglesias > --- > hw/xen/xen-mapcache.c | 12 +++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c > index 5f23b0adbe..18ba7b1d8f 100644 > --- a/hw/xen/xen-mapcache.c > +++ b/hw/xen/xen-mapcache.c > @@ -597,7 +597,17 @@ static void > xen_invalidate_map_cache_entry_unlocked(MapCache *mc, > pentry->next = entry->next; > g_free(entry); > } else { > -memset(entry, 0, sizeof *entry); > +/* > + * Invalidate mapping but keep entry->next pointing to the rest > + * of the list. > + * > + * Note that lock is already zero here, otherwise we don't unmap. > + */ > +entry->paddr_index = 0; > +entry->vaddr_base = NULL; > +entry->valid_mapping = NULL; > +entry->flags = 0; > +entry->size = 0; This kind of feels like mc->entry should be an array of pointer rather than an array of MapCacheEntry but that seems to work well enough and not the first time entries are been cleared like that. Reviewed-by: Anthony PERARD Thanks, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [PATCH v8 2/8] xen: mapcache: Unmap first entries in buckets
On Mon, Jul 01, 2024 at 04:34:53PM +0200, Edgar E. Iglesias wrote: > On Mon, Jul 1, 2024 at 4:30 PM Edgar E. Iglesias > wrote: > > On Mon, Jul 1, 2024 at 3:58 PM Edgar E. Iglesias > > wrote: > >> Any chance you could try to get a backtrace from QEMU when it failed? Here it is: #3 0x7fa8762f4472 in __GI_abort () at ./stdlib/abort.c:79 save_stage = 1 act = {__sigaction_handler = {sa_handler = 0x20, sa_sigaction = 0x20}, sa_mask = {__val = {94603440166168, 18446744073709551615, 94603406369640, 0, 0, 94603406346720, 94603440166168, 140361486774256, 0, 140361486773376, 94603401285536, 140361496232688, 94603440166096, 140361486773456, 94603401289576, 140360849280256}}, sa_flags = -1804462896, sa_restorer = 0x748f2d40} #4 0x560a92230f0d in qemu_get_ram_block (addr=18446744073709551615) at ../system/physmem.c:801 block = 0x0 #5 0x560a922350ab in qemu_ram_block_from_host (ptr=0x7fa84e8fcd00, round_offset=false, offset=0x7fa8748f2de8) at ../system/physmem.c:2280 ram_addr = 18446744073709551615 _rcu_read_auto = 0x1 block = 0x0 host = 0x7fa84e8fcd00 "" _rcu_read_auto = 0x7fa8751f8288 #6 0x560a92229669 in memory_region_from_host (ptr=0x7fa84e8fcd00, offset=0x7fa8748f2de8) at ../system/memory.c:2440 block = 0x0 #7 0x560a92237418 in address_space_unmap (as=0x560a94b05a20, buffer=0x7fa84e8fcd00, len=32768, is_write=true, access_len=32768) at ../system/physmem.c:3246 mr = 0x0 addr1 = 0 __PRETTY_FUNCTION__ = "address_space_unmap" #8 0x560a91fd6cd3 in dma_memory_unmap (as=0x560a94b05a20, buffer=0x7fa84e8fcd00, len=32768, dir=DMA_DIRECTION_FROM_DEVICE, access_len=32768) at /root/build/qemu/include/sysemu/dma.h:236 #9 0x560a91fd763d in dma_blk_unmap (dbs=0x560a94d87400) at ../system/dma-helpers.c:93 i = 1 #10 0x560a91fd76e6 in dma_complete (dbs=0x560a94d87400, ret=0) at ../system/dma-helpers.c:105 __PRETTY_FUNCTION__ = "dma_complete" #11 0x560a91fd781c in dma_blk_cb (opaque=0x560a94d87400, ret=0) at ../system/dma-helpers.c:129 dbs = 0x560a94d87400 ctx = 0x560a9448da90 cur_addr = 0 cur_len = 0 mem = 0x0 __PRETTY_FUNCTION__ = "dma_blk_cb" #12 0x560a9232e974 in blk_aio_complete (acb=0x560a9448d5f0) at ../block/block-backend.c:1555 #13 0x560a9232ebd1 in blk_aio_read_entry (opaque=0x560a9448d5f0) at ../block/block-backend.c:1610 acb = 0x560a9448d5f0 rwco = 0x560a9448d618 qiov = 0x560a94d87460 __PRETTY_FUNCTION__ = "blk_aio_read_entry" > > One more thing, regarding this specific patch. I don't think we should > > clear the > > entire entry, the next field should be kept, otherwise we'll disconnect > > following > > mappings that will never be found again. IIUC, this could very well be > > causing the problem you see. > > > > Does the following make sense? > > > And here without double-freeing entry->valid_mapping: > > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c > index 5f23b0adbe..667807b3b6 100644 > --- a/hw/xen/xen-mapcache.c > +++ b/hw/xen/xen-mapcache.c > @@ -597,7 +597,13 @@ static void > xen_invalidate_map_cache_entry_unlocked(MapCache *mc, > pentry->next = entry->next; > g_free(entry); > } else { > -memset(entry, 0, sizeof *entry); > +/* Invalidate mapping. */ > +entry->paddr_index = 0; > +entry->vaddr_base = NULL; > +entry->size = 0; > +entry->valid_mapping = NULL; > +entry->flags = 0; > +/* Keep entry->next pointing to the rest of the list. */ > } > } I've tried this patch, and that fix the issue I've seen. I'll run more tests on it, just in case, but there's no reason that would break something else. Cheers, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [PATCH v8 2/8] xen: mapcache: Unmap first entries in buckets
Hi all, Following this commit, a test which install Debian in a guest with OVMF as firmware started to fail. QEMU exit with an error when GRUB is running on the freshly installed Debian (I don't know if GRUB is starting Linux or not). The error is: Bad ram offset Some logs: http://logs.test-lab.xenproject.org/osstest/logs/186611/test-amd64-amd64-xl-qemuu-ovmf-amd64/info.html Any idea? Something is trying to do something with the address "-1" when it shouldn't? Cheers, Anthony On Wed, May 29, 2024 at 04:07:33PM +0200, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > When invalidating memory ranges, if we happen to hit the first > entry in a bucket we were never unmapping it. This was harmless > for foreign mappings but now that we're looking to reuse the > mapcache for transient grant mappings, we must unmap entries > when invalidated. > > Signed-off-by: Edgar E. Iglesias > Reviewed-by: Stefano Stabellini > --- > hw/xen/xen-mapcache.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c > index bc860f4373..ec95445696 100644 > --- a/hw/xen/xen-mapcache.c > +++ b/hw/xen/xen-mapcache.c > @@ -491,18 +491,23 @@ static void > xen_invalidate_map_cache_entry_unlocked(MapCache *mc, > return; > } > entry->lock--; > -if (entry->lock > 0 || pentry == NULL) { > +if (entry->lock > 0) { > return; > } > > -pentry->next = entry->next; > ram_block_notify_remove(entry->vaddr_base, entry->size, entry->size); > if (munmap(entry->vaddr_base, entry->size) != 0) { > perror("unmap fails"); > exit(-1); > } > + > g_free(entry->valid_mapping); > -g_free(entry); > +if (pentry) { > + pentry->next = entry->next; > +g_free(entry); > +} else { > +memset(entry, 0, sizeof *entry); > +} > } > > typedef struct XenMapCacheData { > -- > 2.40.1 > > -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
[PATCH] MAINTAINERS: Update my email address
From: Anthony PERARD Signed-off-by: Anthony PERARD --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 302b6fd00c..ea9672fc52 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -532,7 +532,7 @@ Guest CPU Cores (Xen) - X86 Xen CPUs M: Stefano Stabellini -M: Anthony Perard +M: Anthony PERARD M: Paul Durrant L: xen-de...@lists.xenproject.org S: Supported -- Anthony PERARD
Re: [PATCH] xen-hvm: Avoid livelock while handling buffered ioreqs
On Thu, Apr 04, 2024 at 03:08:33PM +0100, Ross Lagerwall wrote: > diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c > index 1627da739822..1116b3978938 100644 > --- a/hw/xen/xen-hvm-common.c > +++ b/hw/xen/xen-hvm-common.c > @@ -521,22 +521,30 @@ static bool handle_buffered_iopage(XenIOState *state) [...] > > static void handle_buffered_io(void *opaque) > { > +unsigned int handled; > XenIOState *state = opaque; > > -if (handle_buffered_iopage(state)) { > +handled = handle_buffered_iopage(state); > +if (handled >= IOREQ_BUFFER_SLOT_NUM) { > +/* We handled a full page of ioreqs. Schedule a timer to continue > + * processing while giving other stuff a chance to run. > + */ ./scripts/checkpatch.pl report a style issue here: WARNING: Block comments use a leading /* on a separate line I can try to remember to fix that on commit. > timer_mod(state->buffered_io_timer, > -BUFFER_IO_MAX_DELAY + > qemu_clock_get_ms(QEMU_CLOCK_REALTIME)); > -} else { > +qemu_clock_get_ms(QEMU_CLOCK_REALTIME)); > +} else if (handled == 0) { Just curious, why did you check for `handled == 0` here instead of `handled != 0`? That would have avoided to invert the last 2 cases, and the patch would just have introduce a new case without changing the order of the existing ones. But not that important I guess. > timer_del(state->buffered_io_timer); > qemu_xen_evtchn_unmask(state->xce_handle, > state->bufioreq_local_port); > +} else { > +timer_mod(state->buffered_io_timer, > + BUFFER_IO_MAX_DELAY + > qemu_clock_get_ms(QEMU_CLOCK_REALTIME)); > } > } Cheers, -- Anthony PERARD
Re: [PATCH v3 2/2] xen: fix stubdom PCI addr
On Wed, Mar 27, 2024 at 04:05:15AM +0100, Marek Marczykowski-Górecki wrote: > When running in a stubdomain, the config space access via sysfs needs to > use BDF as seen inside stubdomain (connected via xen-pcifront), which is > different from the real BDF. For other purposes (hypercall parameters > etc), the real BDF needs to be used. > Get the in-stubdomain BDF by looking up relevant PV PCI xenstore > entries. > > Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 1/2] hw/xen: detect when running inside stubdomain
On Wed, Mar 27, 2024 at 04:05:14AM +0100, Marek Marczykowski-Górecki wrote: > Introduce global xen_is_stubdomain variable when qemu is running inside > a stubdomain instead of dom0. This will be relevant for subsequent > patches, as few things like accessing PCI config space need to be done > differently. > > Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 19/19] hw/xen: Have most of Xen files become target-agnostic
On Tue, Nov 14, 2023 at 03:38:15PM +0100, Philippe Mathieu-Daudé wrote: > Previous commits re-organized the target-specific bits > from Xen files. We can now build the common files once > instead of per-target. > > Only 4 files call libxen API (thus its CPPFLAGS): > - xen-hvm-common.c, > - xen_pt.c, xen_pt_graphics.c, xen_pt_msi.c > > Signed-off-by: Philippe Mathieu-Daudé > --- > Reworked since v1 so dropping David's R-b tag. > --- > accel/xen/meson.build | 2 +- > hw/block/dataplane/meson.build | 2 +- > hw/xen/meson.build | 21 ++--- > 3 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/accel/xen/meson.build b/accel/xen/meson.build > index 002bdb03c6..455ad5d6be 100644 > --- a/accel/xen/meson.build > +++ b/accel/xen/meson.build > @@ -1 +1 @@ > -specific_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c')) > +system_ss.add(when: 'CONFIG_XEN', if_true: files('xen-all.c')) > diff --git a/hw/block/dataplane/meson.build b/hw/block/dataplane/meson.build > index 025b3b061b..4d8bcb0bb9 100644 > --- a/hw/block/dataplane/meson.build > +++ b/hw/block/dataplane/meson.build > @@ -1,2 +1,2 @@ > system_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c')) > -specific_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-block.c')) > +system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-block.c')) > diff --git a/hw/xen/meson.build b/hw/xen/meson.build > index d887fa9ba4..403cab49cf 100644 > --- a/hw/xen/meson.build > +++ b/hw/xen/meson.build > @@ -7,26 +7,25 @@ system_ss.add(when: ['CONFIG_XEN_BUS'], if_true: files( >'xen_pvdev.c', > )) > > -system_ss.add(when: ['CONFIG_XEN', xen], if_true: files( > +system_ss.add(when: ['CONFIG_XEN'], if_true: files( >'xen-operations.c', > -)) > - > -xen_specific_ss = ss.source_set() > -xen_specific_ss.add(files( >'xen-mapcache.c', > +)) > +system_ss.add(when: ['CONFIG_XEN', xen], if_true: files( >'xen-hvm-common.c', > )) > + > if have_xen_pci_passthrough > - xen_specific_ss.add(files( > + system_ss.add(when: ['CONFIG_XEN'], if_true: files( > 'xen-host-pci-device.c', > -'xen_pt.c', > 'xen_pt_config_init.c', > -'xen_pt_graphics.c', > 'xen_pt_load_rom.c', > + )) > + system_ss.add(when: ['CONFIG_XEN', xen], if_true: files( > +'xen_pt.c', > +'xen_pt_graphics.c', How is it useful to separate those source files? In the commit description, there's a talk about "CPPFLAGS", but having `when: [xen]` doesn't change the flags used to build those objects, so the talk about "CPPFLAGS" is confusing. Second, if for some reason the dependency `xen` is false, but `CONFIG_XEN` is true, then we wouldn't be able to build QEMU. Try linking a binary with "xen_pt_config_init.o" but without "xen_pt.o", that's not going to work. So even if that first source file doesn't directly depend on the Xen libraries, it depends on "xen_pt.o" which depends on the Xen libraries. So ultimately, I think all those source files should have the same condition: ['CONFIG_XEN', xen]. I've only checked the xen_pt* source files, I don't know if the same applies to "xen-operations.c" or "xen-mapcache.c". Beside this, QEMU built with Xen support still seems to works fine, so adding the objects to `system_ss` instead of `specific_ss` seems alright. Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 16/19] hw/xen/xen_pt: Add missing license
On Tue, Nov 14, 2023 at 03:38:12PM +0100, Philippe Mathieu-Daudé wrote: > Commit eaab4d60d3 ("Introduce Xen PCI Passthrough, qdevice") > introduced both xen_pt.[ch], but only added the license to > xen_pt.c. Use the same license for xen_pt.h. > > Suggested-by: David Woodhouse > Signed-off-by: Philippe Mathieu-Daudé Fine by me. Looks like there was a license header before: https://xenbits.xen.org/gitweb/?p=qemu-xen-unstable.git;a=blob;f=hw/pass-through.h;h=0b5822414e24d199a064abccc4d378dcaf569bd6;hb=HEAD I don't know why I didn't copied it over here. Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 15/19] hw/xen: Reduce inclusion of 'cpu.h' to target-specific sources
On Tue, Nov 14, 2023 at 03:38:11PM +0100, Philippe Mathieu-Daudé wrote: > We rarely need to include "cpu.h" in headers. Including it > 'taint' headers to be target-specific. Here only the i386/arm > implementations requires "cpu.h", so include it there and > remove from the "hw/xen/xen-hvm-common.h" *common* header. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Richard Henderson > Reviewed-by: David Woodhouse Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC PATCH-for-9.0 v2 13/19] hw/xen: Remove use of 'target_ulong' in handle_ioreq()
On Tue, Nov 14, 2023 at 03:38:09PM +0100, Philippe Mathieu-Daudé wrote: > Per commit f17068c1c7 ("xen-hvm: reorganize xen-hvm and move common > function to xen-hvm-common"), handle_ioreq() is expected to be > target-agnostic. However it uses 'target_ulong', which is a target > specific definition. > > Per xen/include/public/hvm/ioreq.h header: > > struct ioreq { > uint64_t addr; /* physical address */ > uint64_t data; /* data (or paddr of data) */ > uint32_t count; /* for rep prefixes */ > uint32_t size; /* size in bytes */ > uint32_t vp_eport; /* evtchn for notifications to/from device model > */ > uint16_t _pad0; > uint8_t state:4; > uint8_t data_is_ptr:1; /* if 1, data above is the guest paddr > * of the real data to use. */ > uint8_t dir:1; /* 1=read, 0=write */ > uint8_t df:1; > uint8_t _pad1:1; > uint8_t type; /* I/O type */ > }; > typedef struct ioreq ioreq_t; > > If 'data' is not a pointer, it is a u64. > > - In PIO / VMWARE_PORT modes, only 32-bit are used. > > - In MMIO COPY mode, memory is accessed by chunks of 64-bit Looks like it could also be 8, 16, or 32 as well, depending on req->size. > - In PCI_CONFIG mode, access is u8 or u16 or u32. > > - None of TIMEOFFSET / INVALIDATE use 'req'. > > - Fallback is only used in x86 for VMWARE_PORT. > > Masking the upper bits of 'data' to keep 'req->size' low bits > is irrelevant of the target word size. Remove the word size > check and always extract the relevant bits. When building QEMU for Xen, we tend to build the target "i386-softmmu", which looks like to have target_ulong == uint32_t. So the `data` clamping would only apply to size 8 and 16. The clamping with target_ulong was introduce long time ago, here: https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b4a663b87df3954557434a2d31bff7f6b2706ec1 and they were more IOREQ types. So my guess is it isn't relevant anymore, but extending the clamping to 32-bits request should be fine, when using qemu-system-i386 that is, as it is already be done if one use qemu-system-x86_64. So I think the patch is fine, and the tests I've ran so far worked fine. > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 12/19] hw/xen: Merge 'hw/xen/arch_hvm.h' in 'hw/xen/xen-hvm-common.h'
On Tue, Nov 14, 2023 at 03:38:08PM +0100, Philippe Mathieu-Daudé wrote: > We don't need a target-specific header for common target-specific > prototypes. Declare xen_arch_handle_ioreq() and xen_arch_set_memory() > in "hw/xen/xen-hvm-common.h". > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: David Woodhouse > Reviewed-by: Richard Henderson Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 11/19] hw/xen/xen_arch_hvm: Rename prototypes using 'xen_arch_' prefix
On Tue, Nov 14, 2023 at 03:38:07PM +0100, Philippe Mathieu-Daudé wrote: > Use a common 'xen_arch_' prefix for architecture-specific functions. > Rename xen_arch_set_memory() and xen_arch_handle_ioreq(). > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: David Woodhouse > Reviewed-by: Richard Henderson Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [RFC PATCH-for-9.0 v2 09/19] hw/block/xen_blkif: Align structs with QEMU_ALIGNED() instead of #pragma
On Tue, Nov 14, 2023 at 03:38:05PM +0100, Philippe Mathieu-Daudé wrote: > Except imported source files, QEMU code base uses > the QEMU_ALIGNED() macro to align its structures. This patch only convert the alignment, but discard pack. We need both or the struct is changed. > --- > hw/block/xen_blkif.h | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h > index 99733529c1..c1d154d502 100644 > --- a/hw/block/xen_blkif.h > +++ b/hw/block/xen_blkif.h > @@ -18,7 +18,6 @@ struct blkif_common_response { > }; > > /* i386 protocol version */ > -#pragma pack(push, 4) > struct blkif_x86_32_request { > uint8_toperation;/* BLKIF_OP_??? > */ > uint8_tnr_segments; /* number of segments > */ > @@ -26,7 +25,7 @@ struct blkif_x86_32_request { > uint64_t id; /* private guest value, echoed in resp > */ > blkif_sector_t sector_number;/* start sector idx on disk (r/w only) > */ > struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST]; > -}; > +} QEMU_ALIGNED(4); E.g. for this one, I've compare the output of `pahole --class_name=blkif_x86_32_request build/qemu-system-i386`: --- before +++ after @@ -1,11 +1,15 @@ struct blkif_x86_32_request { uint8_toperation;/* 0 1 */ uint8_tnr_segments; /* 1 1 */ uint16_t handle; /* 2 2 */ - uint64_t id; /* 4 8 */ - uint64_t sector_number;/*12 8 */ - struct blkif_request_segment seg[11];/*2088 */ - /* size: 108, cachelines: 2, members: 6 */ - /* last cacheline: 44 bytes */ -} __attribute__((__packed__)); + /* XXX 4 bytes hole, try to pack */ + + uint64_t id; /* 8 8 */ + uint64_t sector_number;/*16 8 */ + struct blkif_request_segment seg[11];/*2488 */ + + /* size: 112, cachelines: 2, members: 6 */ + /* sum members: 108, holes: 1, sum holes: 4 */ + /* last cacheline: 48 bytes */ +} __attribute__((__aligned__(8))); Thanks, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 08/19] hw/xen: Remove unused Xen stubs
On Tue, Nov 14, 2023 at 03:38:04PM +0100, Philippe Mathieu-Daudé wrote: > All these stubs are protected by a 'if (xen_enabled())' check. Are you sure? There's still nothing that prevent a compiler from wanting those, I don't think. Sure, often compilers will remove dead code in `if(0){...}`, but there's no guaranty, is there? Cheers, -- Anthony PERARD
Re: [PATCH-for-9.0 v2 05/19] hw/display: Restrict xen_register_framebuffer() call to Xen
On Tue, Nov 14, 2023 at 03:38:01PM +0100, Philippe Mathieu-Daudé wrote: > Only call xen_register_framebuffer() when Xen is enabled. > > Signed-off-by: Philippe Mathieu-Daudé I don't think this patch is very useful but it's fine, so: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 2/2] xen: fix stubdom PCI addr
First things first, could you fix the coding style? Run something like `./scripts/checkpatch.pl @^..` or `./scripts/checkpatch.pl master..`. Patchew might have run that for you if the patch series had a cover letter. On Tue, Mar 05, 2024 at 08:12:30PM +0100, Marek Marczykowski-Górecki wrote: > diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c > index 8c6e9a1716..8ea2a5a4af 100644 > --- a/hw/xen/xen-host-pci-device.c > +++ b/hw/xen/xen-host-pci-device.c > @@ -9,6 +9,8 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "qemu/cutils.h" > +#include "hw/xen/xen-legacy-backend.h" I'd like to avoid this header here, that would be complicated at the moment, as the global variable `xenstore` would be missing. So for now, that's fine. I guess that could be rework if something like Philippe talked about at https://lore.kernel.org/qemu-devel/429a5a27-21b9-45bd-a1a6-a1c2ccc48...@linaro.org/ materialise. Beside the coding style, the patch looks file. Thanks, -- Anthony PERARD
Re: [PATCH v2 1/2] hw/xen: detect when running inside stubdomain
On Tue, Mar 05, 2024 at 08:12:29PM +0100, Marek Marczykowski-Górecki wrote: > diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c > index 124dd5f3d6..6bd4e6eb2f 100644 > --- a/hw/xen/xen-legacy-backend.c > +++ b/hw/xen/xen-legacy-backend.c > @@ -603,6 +603,20 @@ static void xen_set_dynamic_sysbus(void) > machine_class_allow_dynamic_sysbus_dev(mc, TYPE_XENSYSDEV); > } > > +static bool xen_check_stubdomain(void) > +{ > +char *dm_path = g_strdup_printf("/local/domain/%d/image", xen_domid); > +int32_t dm_domid; > +bool is_stubdom = false; > + > +if (!xenstore_read_int(dm_path, "device-model-domid", &dm_domid)) { > +is_stubdom = dm_domid != 0; > +} > + > +g_free(dm_path); > +return is_stubdom; > +} > + > void xen_be_init(void) > { > xenstore = qemu_xen_xs_open(); > @@ -616,6 +630,8 @@ void xen_be_init(void) > exit(1); > } > > +xen_is_stubdomain = xen_check_stubdomain(); This isn't really a backend specific information, and xen_be_init() is all about old backend implementation support. (qdisk which have been the first to be rewritten doesn't need xen_be_init(), or shouldn't). Could we move the initialisation elsewhere? Is this relevant PV guests? If not, we could move the initialisation to xen_hvm_init_pc(). Also, avoid having xen_check_stubdomain() depending on "xen-legacy-backend", if possible. (In xen_hvm_init_pc(), a call to xen_register_ioreq() opens another xenstore, as `state->xenstore`.) (There's already been effort to build QEMU without legacy backends, that stubdom check would break in this scenario.) Thanks, -- Anthony PERARD
[PULL 1/3] xen/pt: Emulate multifunction bit in header type
From: Ross Lagerwall The intention of the code appears to have been to unconditionally set the multifunction bit but since the emulation mask is 0x00 it has no effect. Instead, emulate the bit and set it based on the multifunction property of the PCIDevice (which can be set using QAPI). This allows making passthrough devices appear as functions in a Xen guest. Signed-off-by: Ross Lagerwall Reviewed-by: Paul Durrant Message-Id: <20231103172601.1319375-1-ross.lagerw...@citrix.com> Signed-off-by: Anthony PERARD --- hw/xen/xen_pt_config_init.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c index ba4cd78238..3edaeab1e3 100644 --- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -292,7 +292,10 @@ static int xen_pt_header_type_reg_init(XenPCIPassthroughState *s, uint32_t *data) { /* read PCI_HEADER_TYPE */ -*data = reg->init_val | 0x80; +*data = reg->init_val; +if ((PCI_DEVICE(s)->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)) { +*data |= PCI_HEADER_TYPE_MULTI_FUNCTION; +} return 0; } @@ -677,7 +680,7 @@ static XenPTRegInfo xen_pt_emu_reg_header0[] = { .size = 1, .init_val = 0x00, .ro_mask= 0xFF, -.emu_mask = 0x00, +.emu_mask = PCI_HEADER_TYPE_MULTI_FUNCTION, .init = xen_pt_header_type_reg_init, .u.b.read = xen_pt_byte_reg_read, .u.b.write = xen_pt_byte_reg_write, -- Anthony PERARD
[PULL 0/3] Xen queue 2024-03-12
The following changes since commit 8f3f329f5e0117bd1a23a79ab751f8a7d3471e4b: Merge tag 'migration-20240311-pull-request' of https://gitlab.com/peterx/qemu into staging (2024-03-12 11:35:41 +) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20240312 for you to fetch changes up to 918a7f706b69a8c725bac0694971d2831f688ebb: i386: load kernel on xen using DMA (2024-03-12 14:13:08 +) Xen queue: * In Xen PCI passthrough, emulate multifunction bit. * Fix in Xen mapcache. * Improve performance of kernel+initrd loading in an Xen HVM Direct Kernel Boot scenario. Marek Marczykowski-Górecki (1): i386: load kernel on xen using DMA Peng Fan (1): xen: Drop out of coroutine context xen_invalidate_map_cache_entry Ross Lagerwall (1): xen/pt: Emulate multifunction bit in header type hw/i386/pc.c| 3 ++- hw/xen/xen-mapcache.c | 30 -- hw/xen/xen_pt_config_init.c | 7 +-- 3 files changed, 35 insertions(+), 5 deletions(-)
[PULL 2/3] xen: Drop out of coroutine context xen_invalidate_map_cache_entry
From: Peng Fan xen_invalidate_map_cache_entry is not expected to run in a coroutine. Without this, there is crash: signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44 threadid=) at pthread_kill.c:78 at /usr/src/debug/glibc/2.38+git-r0/sysdeps/posix/raise.c:26 fmt=0x9e1ca8a8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0xe0d25740 "!qemu_in_coroutine()", file=file@entry=0xe0d301a8 "../qemu-xen-dir-remote/block/graph-lock.c", line=line@entry=260, function=function@entry=0xe0e522c0 <__PRETTY_FUNCTION__.3> "bdrv_graph_rdlock_main_loop") at assert.c:92 assertion=assertion@entry=0xe0d25740 "!qemu_in_coroutine()", file=file@entry=0xe0d301a8 "../qemu-xen-dir-remote/block/graph-lock.c", line=line@entry=260, function=function@entry=0xe0e522c0 <__PRETTY_FUNCTION__.3> "bdrv_graph_rdlock_main_loop") at assert.c:101 at ../qemu-xen-dir-remote/block/graph-lock.c:260 at /home/Freenix/work/sw-stash/xen/upstream/tools/qemu-xen-dir-remote/include/block/graph-lock.h:259 host=host@entry=0x742c8000, size=size@entry=2097152) at ../qemu-xen-dir-remote/block/io.c:3362 host=0x742c8000, size=2097152) at ../qemu-xen-dir-remote/block/block-backend.c:2859 host=, size=, max_size=) at ../qemu-xen-dir-remote/block/block-ram-registrar.c:33 size=2097152, max_size=2097152) at ../qemu-xen-dir-remote/hw/core/numa.c:883 buffer=buffer@entry=0x743c5000 "") at ../qemu-xen-dir-remote/hw/xen/xen-mapcache.c:475 buffer=buffer@entry=0x743c5000 "") at ../qemu-xen-dir-remote/hw/xen/xen-mapcache.c:487 as=as@entry=0xe1ca3ae8 , buffer=0x743c5000, len=, is_write=is_write@entry=true, access_len=access_len@entry=32768) at ../qemu-xen-dir-remote/system/physmem.c:3199 dir=DMA_DIRECTION_FROM_DEVICE, len=, buffer=, as=0xe1ca3ae8 ) at /home/Freenix/work/sw-stash/xen/upstream/tools/qemu-xen-dir-remote/include/sysemu/dma.h:236 elem=elem@entry=0xf620aa30, len=len@entry=32769) at ../qemu-xen-dir-remote/hw/virtio/virtio.c:758 elem=elem@entry=0xf620aa30, len=len@entry=32769, idx=idx@entry=0) at ../qemu-xen-dir-remote/hw/virtio/virtio.c:919 elem=elem@entry=0xf620aa30, len=32769) at ../qemu-xen-dir-remote/hw/virtio/virtio.c:994 req=req@entry=0xf620aa30, status=status@entry=0 '\000') at ../qemu-xen-dir-remote/hw/block/virtio-blk.c:67 ret=0) at ../qemu-xen-dir-remote/hw/block/virtio-blk.c:136 at ../qemu-xen-dir-remote/block/block-backend.c:1559 --Type for more, q to quit, c to continue without paging-- at ../qemu-xen-dir-remote/block/block-backend.c:1614 i1=) at ../qemu-xen-dir-remote/util/coroutine-ucontext.c:177 at ../sysdeps/unix/sysv/linux/aarch64/setcontext.S:123 Signed-off-by: Peng Fan Reviewed-by: Stefano Stabellini Message-Id: <20240124021450.21656-1-peng@oss.nxp.com> Signed-off-by: Anthony PERARD --- hw/xen/xen-mapcache.c | 30 -- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c index 4f956d048e..7f59080ba7 100644 --- a/hw/xen/xen-mapcache.c +++ b/hw/xen/xen-mapcache.c @@ -476,11 +476,37 @@ static void xen_invalidate_map_cache_entry_unlocked(uint8_t *buffer) g_free(entry); } -void xen_invalidate_map_cache_entry(uint8_t *buffer) +typedef struct XenMapCacheData { +Coroutine *co; +uint8_t *buffer; +} XenMapCacheData; + +static void xen_invalidate_map_cache_entry_bh(void *opaque) { +XenMapCacheData *data = opaque; + mapcache_lock(); -xen_invalidate_map_cache_entry_unlocked(buffer); +xen_invalidate_map_cache_entry_unlocked(data->buffer); mapcache_unlock(); + +aio_co_wake(data->co); +} + +void coroutine_mixed_fn xen_invalidate_map_cache_entry(uint8_t *buffer) +{ +if (qemu_in_coroutine()) { +XenMapCacheData data = { +.co = qemu_coroutine_self(), +.buffer = buffer, +}; +aio_bh_schedule_oneshot(qemu_get_current_aio_context(), +xen_invalidate_map_cache_entry_bh, &data); +qemu_coroutine_yield(); +} else { +mapcache_lock(); + xen_invalidate_map_cache_entry_unlocked(buffer); +mapcache_unlock(); +} } void xen_invalidate_map_cache(void) -- Anthony PERARD
[PULL 3/3] i386: load kernel on xen using DMA
From: Marek Marczykowski-Górecki Kernel on Xen is loaded via fw_cfg. Previously it used non-DMA version, which loaded the kernel (and initramfs) byte by byte. Change this to DMA, to load in bigger chunks. This change alone reduces load time of a (big) kernel+initramfs from ~10s down to below 1s. This change was suggested initially here: https://lore.kernel.org/xen-devel/20180216204031.5...@gmail.com/ Apparently this alone is already enough to get massive speedup. Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Alex Bennée Reviewed-by: Anthony PERARD Message-Id: <20210426034709.595432-1-marma...@invisiblethingslab.com> Signed-off-by: Anthony PERARD --- hw/i386/pc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index f5ff970acf..4f322e0856 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -718,7 +718,8 @@ void xen_load_linux(PCMachineState *pcms) assert(MACHINE(pcms)->kernel_filename != NULL); -fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE); +fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, +&address_space_memory); fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus); rom_set_fw(fw_cfg); -- Anthony PERARD
[PATCH] migration: Fix format in error message
From: Anthony PERARD In file_write_ramblock_iov(), "offset" is "uintptr_t" and not "ram_addr_t". While usually they are both equivalent, this is not the case with CONFIG_XEN_BACKEND. Use the right format. This will fix build on 32-bit. Fixes: f427d90b9898 ("migration/multifd: Support outgoing mapped-ram stream format") Signed-off-by: Anthony PERARD --- migration/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration/file.c b/migration/file.c index 164b079966..5054a60851 100644 --- a/migration/file.c +++ b/migration/file.c @@ -191,7 +191,7 @@ int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov, */ offset = (uintptr_t) iov[slice_idx].iov_base - (uintptr_t) block->host; if (offset >= block->used_length) { -error_setg(errp, "offset " RAM_ADDR_FMT +error_setg(errp, "offset %" PRIxPTR "outside of ramblock %s range", offset, block->idstr); ret = -1; break; -- Anthony PERARD
Re: [PATCH v3 01/29] bulk: Access existing variables initialized to &S->F when available
On Mon, Jan 29, 2024 at 05:44:43PM +0100, Philippe Mathieu-Daudé wrote: > When a variable is initialized to &struct->field, use it > in place. Rationale: while this makes the code more concise, > this also helps static analyzers. > > Mechanical change using the following Coccinelle spatch script: > > @@ > type S, F; > identifier s, m, v; > @@ > S *s; > ... > F *v = &s->m; > <+... > -&s->m > +v > ...+> > > Inspired-by: Zhao Liu > Signed-off-by: Philippe Mathieu-Daudé > --- > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index 36e6f93c37..10ddf6bc91 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -710,7 +710,7 @@ static void xen_pt_destroy(PCIDevice *d) { > uint8_t intx; > int rc; > > -if (machine_irq && !xen_host_pci_device_closed(&s->real_device)) { > +if (machine_irq && !xen_host_pci_device_closed(host_dev)) { > intx = xen_pt_pci_intx(s); > rc = xc_domain_unbind_pt_irq(xen_xc, xen_domid, machine_irq, > PT_IRQ_TYPE_PCI, > @@ -759,8 +759,8 @@ static void xen_pt_destroy(PCIDevice *d) { > memory_listener_unregister(&s->io_listener); > s->listener_set = false; > } > -if (!xen_host_pci_device_closed(&s->real_device)) { > - xen_host_pci_device_put(&s->real_device); > +if (!xen_host_pci_device_closed(host_dev)) { > +xen_host_pci_device_put(host_dev); For the Xen part: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH] i386: load kernel on xen using DMA
On Fri, Jun 18, 2021 at 09:54:14AM +0100, Alex Bennée wrote: > > Marek Marczykowski-Górecki writes: > > > Kernel on Xen is loaded via fw_cfg. Previously it used non-DMA version, > > which loaded the kernel (and initramfs) byte by byte. Change this > > to DMA, to load in bigger chunks. > > This change alone reduces load time of a (big) kernel+initramfs from > > ~10s down to below 1s. > > > > This change was suggested initially here: > > https://lore.kernel.org/xen-devel/20180216204031.5...@gmail.com/ > > Apparently this alone is already enough to get massive speedup. > > > > Signed-off-by: Marek Marczykowski-Górecki > > --- > > hw/i386/pc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 8a84b25a03..14e43d4da4 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -839,7 +839,8 @@ void xen_load_linux(PCMachineState *pcms) > > > > assert(MACHINE(pcms)->kernel_filename != NULL); > > > > -fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE); > > +fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, > > +&address_space_memory); > > fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, x86ms->boot_cpus); > > rom_set_fw(fw_cfg); > > Gentle ping. The fix looks perfectly sane to me but I don't have any x86 > Xen HW to test this one. Are the x86 maintainers happy to take this on? Yes. It looks like it works well with both SeaBIOS and OVMF, so the patch is good. > FWIW: > > Reviewed-by: Alex Bennée Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH 14/16] hw/char/xen_console: Fix missing ERRP_GUARD() for error_prepend()
On Thu, Feb 29, 2024 at 12:37:21AM +0800, Zhao Liu wrote: > From: Zhao Liu > > As the comment in qapi/error, passing @errp to error_prepend() requires > ERRP_GUARD(): > > * = Why, when and how to use ERRP_GUARD() = > * > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > ... > * - It should not be passed to error_prepend(), error_vprepend() or > * error_append_hint(), because that doesn't work with &error_fatal. > * ERRP_GUARD() lifts these restrictions. > * > * To use ERRP_GUARD(), add it right at the beginning of the function. > * @errp can then be used without worrying about the argument being > * NULL or &error_fatal. > > ERRP_GUARD() could avoid the case when @errp is the pointer of > error_fatal, the user can't see this additional information, because > exit() happens in error_setg earlier than information is added [1]. > > The xen_console_connect() passes @errp to error_prepend() without > ERRP_GUARD(). > > There're 2 places will call xen_console_connect(): > - xen_console_realize(): the @errp is from DeviceClass.realize()'s > parameter. > - xen_console_frontend_changed(): the @errp points its caller's >@local_err. > > To avoid the issue like [1] said, add missing ERRP_GUARD() at the > beginning of xen_console_connect(). > > [1]: Issue description in the commit message of commit ae7c80a7bd73 > ("error: New macro ERRP_GUARD()"). > > Cc: Stefano Stabellini > Cc: Anthony Perard > Cc: Paul Durrant > Cc: "Marc-André Lureau" > Cc: Paolo Bonzini > Signed-off-by: Zhao Liu Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH 02/17] hw/net/xen_nic: Fix missing ERRP_GUARD() for error_prepend()
On Thu, Feb 29, 2024 at 06:25:40PM +0100, Thomas Huth wrote: > On 29/02/2024 15.38, Zhao Liu wrote: > > From: Zhao Liu > > > > As the comment in qapi/error, passing @errp to error_prepend() requires > > ERRP_GUARD(): > > > > * = Why, when and how to use ERRP_GUARD() = > > * > > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > > ... > > * - It should not be passed to error_prepend(), error_vprepend() or > > * error_append_hint(), because that doesn't work with &error_fatal. > > * ERRP_GUARD() lifts these restrictions. > > * > > * To use ERRP_GUARD(), add it right at the beginning of the function. > > * @errp can then be used without worrying about the argument being > > * NULL or &error_fatal. > > > > ERRP_GUARD() could avoid the case when @errp is the pointer of > > error_fatal, the user can't see this additional information, because > > exit() happens in error_setg earlier than information is added [1]. > > > > The xen_netdev_connect() passes @errp to error_prepend(), and its @errp > > parameter is from xen_device_frontend_changed(). > > > > Though its @errp points to @local_err of xen_device_frontend_changed(), > > to follow the requirement of @errp, add missing ERRP_GUARD() at the > > beginning of this function. > > > > [1]: Issue description in the commit message of commit ae7c80a7bd73 > > ("error: New macro ERRP_GUARD()"). > > > > Cc: Stefano Stabellini > > Cc: Anthony Perard > > Cc: Paul Durrant > > Cc: Jason Wang > > Signed-off-by: Zhao Liu > > --- > > hw/net/xen_nic.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c > > index 453fdb981983..89487b49baf9 100644 > > --- a/hw/net/xen_nic.c > > +++ b/hw/net/xen_nic.c > > @@ -351,6 +351,7 @@ static bool net_event(void *_xendev) > > static bool xen_netdev_connect(XenDevice *xendev, Error **errp) > > { > > +ERRP_GUARD(); > > XenNetDev *netdev = XEN_NET_DEVICE(xendev); > > unsigned int port, rx_copy; > > Reviewed-by: Thomas Huth Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH V8 00/12] fix migration of suspended runstate
On Mon, Dec 18, 2023 at 01:14:51PM +0800, Peter Xu wrote: > On Wed, Dec 13, 2023 at 10:35:33AM -0500, Steven Sistare wrote: > > Hi Peter, all have RB's, with all i's dotted and t's crossed - steve > > Yes this seems to be more migration related so maybe good candidate for a > pull from migration submodule. > > But since this is still solving a generic issue, I'm copying a few more > people from get_maintainers.pl that this series touches, just in case > they'll have something to say before dev cycle starts. I did a quick smoke test of migrating a Xen guest. It works fine for me. Thanks, -- Anthony PERARD
Re: [PATCH] fix qemu build with xen-4.18.0
On Tue, Dec 12, 2023 at 03:35:50PM +, Volodymyr Babchuk wrote: > Hi Anthony > > Anthony PERARD writes: > > > On Fri, Dec 08, 2023 at 02:49:27PM -0800, Stefano Stabellini wrote: > >> On Fri, 8 Dec 2023, Daniel P. Berrangé wrote: > >> > On Thu, Dec 07, 2023 at 11:12:48PM +, Michael Young wrote: > >> > > Builds of qemu-8.2.0rc2 with xen-4.18.0 are currently failing > >> > > with errors like > >> > > ../hw/arm/xen_arm.c:74:5: error: ‘GUEST_VIRTIO_MMIO_SPI_LAST’ > >> > > undeclared (first use in this function) > >> > >74 |(GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST) > >> > > | ^~ > >> > > > >> > > as there is an incorrect comparision in include/hw/xen/xen_native.h > >> > > which means that settings like GUEST_VIRTIO_MMIO_SPI_LAST > >> > > aren't being defined for xen-4.18.0 > >> > > >> > The conditions in arch-arm.h for xen 4.18 show: > >> > > >> > $ cppi arch-arm.h | grep -E '(#.*if)|MMIO' > >> > #ifndef __XEN_PUBLIC_ARCH_ARM_H__ > >> > # if defined(__XEN__) || defined(__XEN_TOOLS__) || defined(__GNUC__) > >> > # endif > >> > # ifndef __ASSEMBLY__ > >> > # if defined(__XEN__) || defined(__XEN_TOOLS__) > >> > # if defined(__GNUC__) && !defined(__STRICT_ANSI__) > >> > # endif > >> > # endif /* __XEN__ || __XEN_TOOLS__ */ > >> > # endif > >> > # if defined(__XEN__) || defined(__XEN_TOOLS__) > >> > # define PSR_MODE_BIT 0x10U /* Set iff AArch32 */ > >> > /* Virtio MMIO mappings */ > >> > # define GUEST_VIRTIO_MMIO_BASE xen_mk_ullong(0x0200) > >> > # define GUEST_VIRTIO_MMIO_SIZE xen_mk_ullong(0x0010) > >> > # define GUEST_VIRTIO_MMIO_SPI_FIRST 33 > >> > # define GUEST_VIRTIO_MMIO_SPI_LAST43 > >> > # endif > >> > # ifndef __ASSEMBLY__ > >> > # endif > >> > #endif /* __XEN_PUBLIC_ARCH_ARM_H__ */ > >> > > >> > So the MMIO constants are available if __XEN__ or __XEN_TOOLS__ > >> > are defined. This is no different to the condition that was > >> > present in Xen 4.17. > >> > > >> > What you didn't mention was that the Fedora build failure is > >> > seen on an x86_64 host, when building the aarch64 target QEMU, > >> > and I think this is the key issue. > >> > >> Hi Daniel, thanks for looking into it. > >> > >> - you are building on a x86_64 host > >> - the target is aarch64 > >> - the target is the aarch64 Xen PVH machine (xen_arm.c) > >> > >> But is the resulting QEMU binary expected to be an x86 binary? Or are > >> you cross compiling ARM binaries on a x86 host? > >> > >> In other word, is the resulting QEMU binary expected to run on ARM or > >> x86? > >> > >> > >> > Are we expecting to build Xen support for non-arch native QEMU > >> > system binaries or not ? > >> > >> The ARM xenpvh machine (xen_arm.c) is meant to work with Xen on ARM, not > >> Xen on x86. So this is only expected to work if you are > >> cross-compiling. But you can cross-compile both Xen and QEMU, and I am > >> pretty sure that Yocto is able to build Xen, Xen userspace tools, and > >> QEMU for Xen/ARM on an x86 host today. > >> > >> > >> > The constants are defined in arch-arm.h, which is only included > >> > under: > >> > > >> > #if defined(__i386__) || defined(__x86_64__) > >> > #include "arch-x86/xen.h" > >> > #elif defined(__arm__) || defined (__aarch64__) > >> > #include "arch-arm.h" > >> > #else > >> > #error "Unsupported architecture" > >> > #endif > >> > > >> > > >> > When we are building on an x86_64 host, we not going to get > >> > arch-arm.h included, even if we're trying to build the aarch64 > >> > system emulator. > >> > > >> > I don't know how this is supposed to work ? > >> > >> It looks like a host vs. target architecture mismatch: the #if defined > >> (__aarch64__) check should pass I think. > > > > > > Building qemu with something like: > > ./configure --enable-xen --cpu=x86_64 > > used to work. Can we fix that? It still works with v8.1.0. > > At least, it works on x86, I never really try to build qemu for arm. > > Notice that there's no "--target-list" on the configure command line. > > I don't know if --cpu is useful here. > > > > Looks like the first commit where the build doesn't work is > > 7899f6589b78 ("xen_arm: Add virtual PCIe host bridge support"). > > I am currently trying to upstream this patch. It is in the QEMU mailing > list but it was never accepted. It is not reviewed in fact. I'll take a > look at it, but I don't understand how did you get in the first place. Sorry, I got the wrong commit pasted, I actually meant: 0c8ab1cddd6c ("xen_arm: Create virtio-mmio devices during initialization") -- Anthony PERARD
Re: [RFC PATCH v4 4/6] xen: add option to disable legacy backends
On Sat, Dec 02, 2023 at 01:41:22AM +, Volodymyr Babchuk wrote: > diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c > index 9f9f137f99..03a55f345c 100644 > --- a/hw/xenpv/xen_machine_pv.c > +++ b/hw/xenpv/xen_machine_pv.c > @@ -37,7 +37,9 @@ static void xen_init_pv(MachineState *machine) > setup_xen_backend_ops(); > > /* Initialize backend core & drivers */ > +#ifdef CONFIG_XEN_LEGACY_BACKENDS > xen_be_init(); > +#endif There's more code that depends on legacy backend support in this function: Call to xen_be_register() and xen_config_dev_nic() and symbol xen_config_cleanup, and the code commented with "configure framebuffer". I've tried to build this on x86. > > switch (xen_mode) { > case XEN_ATTACH: > diff --git a/meson.build b/meson.build > index ec01f8b138..c8a43dd97d 100644 > --- a/meson.build > +++ b/meson.build > @@ -2219,6 +,7 @@ config_host_data.set('CONFIG_DBUS_DISPLAY', > dbus_display) > config_host_data.set('CONFIG_CFI', get_option('cfi')) > config_host_data.set('CONFIG_SELINUX', selinux.found()) > config_host_data.set('CONFIG_XEN_BACKEND', xen.found()) > +config_host_data.set('CONFIG_XEN_LEGACY_BACKENDS', have_xen_legacy_backends) I don't know if "config_host_data" is the right place to have "#define CONFIG_XEN_LEGACY_BACKENDS", but the alternative is probably to define a Kconfig value, but I don't know if that would be correct as well. I guess this is fine here, for now. > config_host_data.set('CONFIG_LIBDW', libdw.found()) > if xen.found() ># protect from xen.version() having less than three components > @@ -3049,6 +3053,7 @@ config_all += config_targetos > config_all += config_all_disas > config_all += { >'CONFIG_XEN': xen.found(), > + 'CONFIG_XEN_LEGACY_BACKENDS': have_xen_legacy_backends, I don't think this is useful here, or even wanted. I think things added to config_all are used only in "meson.build" files, for things like "system_ss.add(when: ['CONFIG_XEN_LEGACY_BACKENDS'] ..." But you use "if have_xen_legacy_backends" instead, which is probably ok (because objects also depends on CONFIG_XEN_BUS). >'CONFIG_SYSTEM_ONLY': have_system, >'CONFIG_USER_ONLY': have_user, >'CONFIG_ALL': true, > diff --git a/meson_options.txt b/meson_options.txt > index c9baeda639..91dd677257 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -77,6 +77,8 @@ option('nvmm', type: 'feature', value: 'auto', > description: 'NVMM acceleration support') > option('xen', type: 'feature', value: 'auto', > description: 'Xen backend support') > +option('xen-legacy-backends', type: 'feature', value: 'auto', Every other meson options are using '_', I haven't found any single '-'. Shouldn't this new option follow the same trend and be named "xen_legacy_backends" ? > + description: 'Xen legacy backends (9pfs, fb, qusb) support') This description feels a bit wrong somehow. "Legacy backend" is internal to QEMU's code, and meant that the backends are implemented using legacy support that we want to retire. But the backends them self, as seen by a guest aren't going to change, and are not legacy. Also, a few month ago, "qnic" would have been part of the list. Maybe a description like "Xen backends based on legacy support" might be more appropriate. I'm not sure listing the different backend in the description is a good idea, as we will have to remember to change it whenever one of those backend is been upgraded. Cheers, -- Anthony PERARD
Re: [PATCH] fix qemu build with xen-4.18.0
On Fri, Dec 08, 2023 at 02:49:27PM -0800, Stefano Stabellini wrote: > On Fri, 8 Dec 2023, Daniel P. Berrangé wrote: > > On Thu, Dec 07, 2023 at 11:12:48PM +, Michael Young wrote: > > > Builds of qemu-8.2.0rc2 with xen-4.18.0 are currently failing > > > with errors like > > > ../hw/arm/xen_arm.c:74:5: error: ‘GUEST_VIRTIO_MMIO_SPI_LAST’ undeclared > > > (first use in this function) > > >74 |(GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST) > > > | ^~ > > > > > > as there is an incorrect comparision in include/hw/xen/xen_native.h > > > which means that settings like GUEST_VIRTIO_MMIO_SPI_LAST > > > aren't being defined for xen-4.18.0 > > > > The conditions in arch-arm.h for xen 4.18 show: > > > > $ cppi arch-arm.h | grep -E '(#.*if)|MMIO' > > #ifndef __XEN_PUBLIC_ARCH_ARM_H__ > > # if defined(__XEN__) || defined(__XEN_TOOLS__) || defined(__GNUC__) > > # endif > > # ifndef __ASSEMBLY__ > > # if defined(__XEN__) || defined(__XEN_TOOLS__) > > # if defined(__GNUC__) && !defined(__STRICT_ANSI__) > > # endif > > # endif /* __XEN__ || __XEN_TOOLS__ */ > > # endif > > # if defined(__XEN__) || defined(__XEN_TOOLS__) > > # define PSR_MODE_BIT 0x10U /* Set iff AArch32 */ > > /* Virtio MMIO mappings */ > > # define GUEST_VIRTIO_MMIO_BASE xen_mk_ullong(0x0200) > > # define GUEST_VIRTIO_MMIO_SIZE xen_mk_ullong(0x0010) > > # define GUEST_VIRTIO_MMIO_SPI_FIRST 33 > > # define GUEST_VIRTIO_MMIO_SPI_LAST43 > > # endif > > # ifndef __ASSEMBLY__ > > # endif > > #endif /* __XEN_PUBLIC_ARCH_ARM_H__ */ > > > > So the MMIO constants are available if __XEN__ or __XEN_TOOLS__ > > are defined. This is no different to the condition that was > > present in Xen 4.17. > > > > What you didn't mention was that the Fedora build failure is > > seen on an x86_64 host, when building the aarch64 target QEMU, > > and I think this is the key issue. > > Hi Daniel, thanks for looking into it. > > - you are building on a x86_64 host > - the target is aarch64 > - the target is the aarch64 Xen PVH machine (xen_arm.c) > > But is the resulting QEMU binary expected to be an x86 binary? Or are > you cross compiling ARM binaries on a x86 host? > > In other word, is the resulting QEMU binary expected to run on ARM or > x86? > > > > Are we expecting to build Xen support for non-arch native QEMU > > system binaries or not ? > > The ARM xenpvh machine (xen_arm.c) is meant to work with Xen on ARM, not > Xen on x86. So this is only expected to work if you are > cross-compiling. But you can cross-compile both Xen and QEMU, and I am > pretty sure that Yocto is able to build Xen, Xen userspace tools, and > QEMU for Xen/ARM on an x86 host today. > > > > The constants are defined in arch-arm.h, which is only included > > under: > > > > #if defined(__i386__) || defined(__x86_64__) > > #include "arch-x86/xen.h" > > #elif defined(__arm__) || defined (__aarch64__) > > #include "arch-arm.h" > > #else > > #error "Unsupported architecture" > > #endif > > > > > > When we are building on an x86_64 host, we not going to get > > arch-arm.h included, even if we're trying to build the aarch64 > > system emulator. > > > > I don't know how this is supposed to work ? > > It looks like a host vs. target architecture mismatch: the #if defined > (__aarch64__) check should pass I think. Building qemu with something like: ./configure --enable-xen --cpu=x86_64 used to work. Can we fix that? It still works with v8.1.0. At least, it works on x86, I never really try to build qemu for arm. Notice that there's no "--target-list" on the configure command line. I don't know if --cpu is useful here. Looks like the first commit where the build doesn't work is 7899f6589b78 ("xen_arm: Add virtual PCIe host bridge support"). Could we get that fixed? I'm sure distribution will appreciate to be able to build a single qemu package for xen and other, rather than having a dedicated qemu-xen package. Cheers, -- Anthony PERARD
Re: [PATCH v4 2/6] xen: backends: don't overwrite XenStore nodes created by toolstack
On Sat, Dec 02, 2023 at 01:41:21AM +, Volodymyr Babchuk wrote: > Xen PV devices in QEMU can be created in two ways: either by QEMU > itself, if they were passed via command line, or by Xen toolstack. In > the latter case, QEMU scans XenStore entries and configures devices > accordingly. > > In the second case we don't want QEMU to write/delete front-end > entries for two reasons: it might have no access to those entries if > it is running in un-privileged domain and it is just incorrect to > overwrite entries already provided by Xen toolstack, because toolstack > manages those nodes. For example, it might read backend- or frontend- > state to be sure that they are both disconnected and it is safe to > destroy a domain. > > This patch checks presence of xendev->backend to check if Xen PV > device was configured by Xen toolstack to decide if it should touch > frontend entries in XenStore. Also, when we need to remove XenStore > entries during device teardown only if they weren't created by Xen > toolstack. If they were created by toolstack, then it is toolstack's > job to do proper clean-up. > > Suggested-by: Paul Durrant > Suggested-by: David Woodhouse > Co-Authored-by: Oleksandr Tyshchenko > Signed-off-by: Volodymyr Babchuk > Reviewed-by: David Woodhouse > Hi Volodymyr, There's something wrong with this patch. The block backend doesn't work when creating a guest via libxl, an x86 hvm guest with qdisk. Error from guest kernel: "2 reading backend fields at /local/domain/0/backend/qdisk/23/768" It seems that "sector-size" is missing for the disk. Thanks, -- Anthony PERARD
Re: [QEMU][PATCH v4 1/2] xen_arm: Create virtio-mmio devices during initialization
On Wed, Oct 11, 2023 at 12:22:46PM -0700, Vikram Garhwal wrote: > Hi Anthony, > On Thu, Oct 05, 2023 at 11:40:57AM +0100, Anthony PERARD wrote: > > Hi Vikram, > > > > This patch prevent QEMU from been build with Xen 4.15. See comments. > > > > Also, why didn't you CC all the maintainers of > > include/hw/xen/xen_native.h? > I missed it. Initial version didn't have this file change and i missed > updating > my cc list. I use `cccmd` to never miss anyone, and I don't have to build a cc list ;-) $ git config sendemail.cccmd scripts/get_maintainer.pl --noroles --norolestats --nogit --nogit-fallback > > > +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle > > > *dmod, > > > + domid_t domid, uint32_t > > > irq, > > > + unsigned int level) > > > +{ > > > +return 0; > > > > Shouldn't this return something like -ENOSYS, instead of returning a > > success? > Changed return to -ENOSYS for older version. Actually, at least on linux, looks like the function would return either -1 or 0, and set errno. It seems that xendevicemodel_set_irq_level() ultimately called ioctl(), but also the code in xen.git/tools/libs/devicemodel/ also only returns -1 or 0. So it's probably best to set errno=ENOSYS and return -1. > > > > > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > > > index 1d3e6d481a..7393b37355 100644 > > > --- a/hw/arm/xen_arm.c > > > +++ b/hw/arm/xen_arm.c > > > + > > > +static void xen_set_irq(void *opaque, int irq, int level) > > > +{ > > > +xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level); > > > > So, you just ignore the return value here. Shouldn't there be some kind > > of error check? > > > > And is it OK to create a virtio-mmio device without an error, even when > > we could find out that it never going to work (e.g. on Xen 4.14)? > This is something Oleksandr can answer better as it was written by him. But > I think we can print an error "virtio init failed" and exit the > machine init. Does that aligns with your thinking? Something like that, yes, if possible. It would be a bit difficult because xen_set_irq() seems to only be a handler which might only be called after the machine as started. So I'm not sure what would be best to do here. Thanks, -- Anthony PERARD
Re: [QEMU][PATCH v4 1/2] xen_arm: Create virtio-mmio devices during initialization
Hi Vikram, This patch prevent QEMU from been build with Xen 4.15. See comments. Also, why didn't you CC all the maintainers of include/hw/xen/xen_native.h? On Tue, Aug 29, 2023 at 09:35:17PM -0700, Vikram Garhwal wrote: > diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h > index 4dce905fde..a4b1aa9e5d 100644 > --- a/include/hw/xen/xen_native.h > +++ b/include/hw/xen/xen_native.h > @@ -523,4 +523,20 @@ static inline int xen_set_ioreq_server_state(domid_t dom, > enable); > } > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION <= 41500 xendevicemodel_set_irq_level() was introduced in Xen 4.15, so this should say '<' and not '<=', otherwise, we have: include/hw/xen/xen_native.h:527:19: error: static declaration of ‘xendevicemodel_set_irq_level’ follows non-static declaration > +static inline int xendevicemodel_set_irq_level(xendevicemodel_handle *dmod, > + domid_t domid, uint32_t irq, > + unsigned int level) > +{ > +return 0; Shouldn't this return something like -ENOSYS, instead of returning a success? > diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c > index 1d3e6d481a..7393b37355 100644 > --- a/hw/arm/xen_arm.c > +++ b/hw/arm/xen_arm.c > + > +static void xen_set_irq(void *opaque, int irq, int level) > +{ > +xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level); So, you just ignore the return value here. Shouldn't there be some kind of error check? And is it OK to create a virtio-mmio device without an error, even when we could find out that it never going to work (e.g. on Xen 4.14)? Cheers, -- Anthony PERARD
Re: [PATCH 6/7] block: Clean up local variable shadowing
On Thu, Aug 31, 2023 at 03:25:45PM +0200, Markus Armbruster wrote: > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index 3906b9058b..a07cd7eb5d 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -369,7 +369,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, > const char *name, > case XEN_BLOCK_VDEV_TYPE_XVD: > case XEN_BLOCK_VDEV_TYPE_HD: > case XEN_BLOCK_VDEV_TYPE_SD: { > -char *name = disk_to_vbd_name(vdev->disk); > +char *vbd_name = disk_to_vbd_name(vdev->disk); > > str = g_strdup_printf("%s%s%lu", >(vdev->type == XEN_BLOCK_VDEV_TYPE_XVD) ? > @@ -377,8 +377,8 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, > const char *name, >(vdev->type == XEN_BLOCK_VDEV_TYPE_HD) ? >"hd" : >"sd", > - name, vdev->partition); > -g_free(name); > + vbd_name, vdev->partition); > + g_free(vbd_name); > break; > } > default: Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v1 00/23] Q35 support for Xen
Hi Joel, We had a design session about Q35 support during Xen Summit, and I think the result of it is that some more changes are going to be needed, right? So, is it worth it for me to spend some time on review this patch series in its current form, or should I wait until the next revision? And same question for the xen toolstack side. Cheers, -- Anthony PERARD
[PULL 0/5] Misc fixes, for thread-pool, xen, and xen-emulate
The following changes since commit 802341823f1720511dd5cf53ae40285f7978c61b: Merge tag 'pull-tcg-20230731' of https://gitlab.com/rth7680/qemu into staging (2023-07-31 14:02:51 -0700) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20230801 for you to fetch changes up to 856ca10f9ce1fcffeab18546b36a64f79017c905: xen-platform: do full PCI reset during unplug of IDE devices (2023-08-01 10:22:33 +0100) Misc fixes, for thread-pool, xen, and xen-emulate * fix an access to `request_cond` QemuCond in thread-pool * fix issue with PCI devices when unplugging IDE devices in Xen guest * several fixes for issues pointed out by Coverity -------- Anthony PERARD (2): xen-block: Avoid leaks on new error path thread-pool: signal "request_cond" while locked David Woodhouse (1): hw/xen: Clarify (lack of) error handling in transaction_commit() Olaf Hering (1): xen-platform: do full PCI reset during unplug of IDE devices Peter Maydell (1): xen: Don't pass MemoryListener around by value hw/arm/xen_arm.c| 4 ++-- hw/block/xen-block.c| 11 ++- hw/i386/kvm/xenstore_impl.c | 12 +++- hw/i386/xen/xen-hvm.c | 4 ++-- hw/i386/xen/xen_platform.c | 7 --- hw/xen/xen-hvm-common.c | 8 include/hw/xen/xen-hvm-common.h | 2 +- util/thread-pool.c | 2 +- 8 files changed, 31 insertions(+), 19 deletions(-)
[PULL 5/5] xen-platform: do full PCI reset during unplug of IDE devices
From: Olaf Hering The IDE unplug function needs to reset the entire PCI device, to make sure all state is initialized to defaults. This is done by calling pci_device_reset, which resets not only the chip specific registers, but also all PCI state. This fixes "unplug" in a Xen HVM domU with the modular legacy xenlinux PV drivers. Commit ee358e919e38 ("hw/ide/piix: Convert reset handler to DeviceReset") changed the way how the the disks are unplugged. Prior this commit the PCI device remained unchanged. After this change, piix_ide_reset is exercised after the "unplug" command, which was not the case prior that commit. This function resets the command register. As a result the ata_piix driver inside the domU will see a disabled PCI device. The generic PCI code will reenable the PCI device. On the qemu side, this runs pci_default_write_config/pci_update_mappings. Here a changed address is returned by pci_bar_address, this is the address which was truncated in piix_ide_reset. In case of a Xen HVM domU, the address changes from 0xc120 to 0xc100. This truncation was a bug in piix_ide_reset, which was fixed in commit 230dfd9257 ("hw/ide/piix: properly initialize the BMIBA register"). If pci_xen_ide_unplug had used pci_device_reset, the PCI registers would have been properly reset, and commit ee358e919e38 would have not introduced a regression for this specific domU environment. While the unplug is supposed to hide the IDE disks, the changed BMIBA address broke the UHCI device. In case the domU has an USB tablet configured, to recive absolute pointer coordinates for the GUI, it will cause a hang during device discovery of the partly discovered USB hid device. Reading the USBSTS word size register will fail. The access ends up in the QEMU piix-bmdma device, instead of the expected uhci device. Here a byte size request is expected, and a value of ~0 is returned. As a result the UCHI driver sees an error state in the register, and turns off the UHCI controller. Signed-off-by: Olaf Hering Reviewed-by: Paul Durrant Message-Id: <20230720072950.20198-1-o...@aepfle.de> Signed-off-by: Anthony PERARD --- hw/i386/xen/xen_platform.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c index 57f1d742c1..17457ff3de 100644 --- a/hw/i386/xen/xen_platform.c +++ b/hw/i386/xen/xen_platform.c @@ -164,8 +164,9 @@ static void pci_unplug_nics(PCIBus *bus) * * [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc */ -static void pci_xen_ide_unplug(DeviceState *dev, bool aux) +static void pci_xen_ide_unplug(PCIDevice *d, bool aux) { +DeviceState *dev = DEVICE(d); PCIIDEState *pci_ide; int i; IDEDevice *idedev; @@ -195,7 +196,7 @@ static void pci_xen_ide_unplug(DeviceState *dev, bool aux) blk_unref(blk); } } -device_cold_reset(dev); +pci_device_reset(d); } static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque) @@ -210,7 +211,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque) switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) { case PCI_CLASS_STORAGE_IDE: -pci_xen_ide_unplug(DEVICE(d), aux); +pci_xen_ide_unplug(d, aux); break; case PCI_CLASS_STORAGE_SCSI: -- Anthony PERARD
[PULL 1/5] hw/xen: Clarify (lack of) error handling in transaction_commit()
From: David Woodhouse Coverity was unhappy (CID 1508359) because we didn't check the return of init_walk_op() in transaction_commit(), despite doing so at every other call site. Strictly speaking, this is a false positive since it can never fail. It only fails for invalid user input (transaction ID or path), and both of those are hard-coded to known sane values in this invocation. But Coverity doesn't know that, and neither does the casual reader of the code. Returning an error here would be weird, since the transaction *is* committed by this point; all the walk_op is doing is firing watches on the newly-committed changed nodes. So make it a g_assert(!ret), since it really should never happen. Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Message-Id: <20076888f6bdf06a65aafc5cf954260965d45b97.ca...@infradead.org> Signed-off-by: Anthony PERARD --- hw/i386/kvm/xenstore_impl.c | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/hw/i386/kvm/xenstore_impl.c b/hw/i386/kvm/xenstore_impl.c index 305fe75519..d9732b567e 100644 --- a/hw/i386/kvm/xenstore_impl.c +++ b/hw/i386/kvm/xenstore_impl.c @@ -1022,6 +1022,7 @@ static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) { struct walk_op op; XsNode **n; +int ret; if (s->root_tx != tx->base_tx) { return EAGAIN; @@ -1032,7 +1033,16 @@ static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) s->root_tx = tx->tx_id; s->nr_nodes = tx->nr_nodes; -init_walk_op(s, &op, XBT_NULL, tx->dom_id, "/", &n); +ret = init_walk_op(s, &op, XBT_NULL, tx->dom_id, "/", &n); +/* + * There are two reasons why init_walk_op() may fail: an invalid tx_id, + * or an invalid path. We pass XBT_NULL and "/", and it cannot fail. + * If it does, the world is broken. And returning 'ret' would be weird + * because the transaction *was* committed, and all this tree walk is + * trying to do is fire the resulting watches on newly-committed nodes. + */ + g_assert(!ret); + op.deleted_in_tx = false; op.mutating = true; -- Anthony PERARD
[PULL 2/5] xen-block: Avoid leaks on new error path
From: Anthony PERARD Commit 189829399070 ("xen-block: Use specific blockdev driver") introduced a new error path, without taking care of allocated resources. So only allocate the qdicts after the error check, and free both `filename` and `driver` when we are about to return and thus taking care of both success and error path. Coverity only spotted the leak of qdicts (*_layer variables). Reported-by: Peter Maydell Fixes: Coverity CID 1508722, 1398649 Fixes: 189829399070 ("xen-block: Use specific blockdev driver") Signed-off-by: Anthony PERARD Reviewed-by: Paul Durrant Reviewed-by: Peter Maydell Message-Id: <20230704171819.42564-1-anthony.per...@citrix.com> Signed-off-by: Anthony PERARD --- hw/block/xen-block.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index f099914831..3906b9058b 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -781,14 +781,15 @@ static XenBlockDrive *xen_block_drive_create(const char *id, drive = g_new0(XenBlockDrive, 1); drive->id = g_strdup(id); -file_layer = qdict_new(); -driver_layer = qdict_new(); - rc = stat(filename, &st); if (rc) { error_setg_errno(errp, errno, "Could not stat file '%s'", filename); goto done; } + +file_layer = qdict_new(); +driver_layer = qdict_new(); + if (S_ISBLK(st.st_mode)) { qdict_put_str(file_layer, "driver", "host_device"); } else { @@ -796,7 +797,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id, } qdict_put_str(file_layer, "filename", filename); -g_free(filename); if (mode && *mode != 'w') { qdict_put_bool(file_layer, "read-only", true); @@ -831,7 +831,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id, qdict_put_str(file_layer, "locking", "off"); qdict_put_str(driver_layer, "driver", driver); -g_free(driver); qdict_put(driver_layer, "file", file_layer); @@ -842,6 +841,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id, qobject_unref(driver_layer); done: +g_free(filename); +g_free(driver); if (*errp) { xen_block_drive_destroy(drive, NULL); return NULL; -- Anthony PERARD
[PULL 3/5] thread-pool: signal "request_cond" while locked
From: Anthony PERARD thread_pool_free() might have been called on the `pool`, which would be a reason for worker_thread() to quit. In this case, `pool->request_cond` is been destroyed. If worker_thread() didn't managed to signal `request_cond` before it been destroyed by thread_pool_free(), we got: util/qemu-thread-posix.c:198: qemu_cond_signal: Assertion `cond->initialized' failed. One backtrace: __GI___assert_fail (assertion=0x5614abcb "cond->initialized", file=0x5614ab88 "util/qemu-thread-posix.c", line=198, function=0x5614ad80 <__PRETTY_FUNCTION__.17104> "qemu_cond_signal") at assert.c:101 qemu_cond_signal (cond=0x7fffb800db30) at util/qemu-thread-posix.c:198 worker_thread (opaque=0x7fffb800dab0) at util/thread-pool.c:129 qemu_thread_start (args=0x7fffb8000b20) at util/qemu-thread-posix.c:505 start_thread (arg=) at pthread_create.c:486 Reported here: https://lore.kernel.org/all/ZJwoK50FcnTSfFZ8@MacBook-Air-de-Roger.local/T/#u To avoid issue, keep lock while sending a signal to `request_cond`. Fixes: 900fa208f506 ("thread-pool: replace semaphore with condition variable") Signed-off-by: Anthony PERARD Reviewed-by: Stefan Hajnoczi Message-Id: <20230714152720.5077-1-anthony.per...@citrix.com> Signed-off-by: Anthony PERARD --- util/thread-pool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/thread-pool.c b/util/thread-pool.c index 0d97888df0..e3d8292d14 100644 --- a/util/thread-pool.c +++ b/util/thread-pool.c @@ -120,13 +120,13 @@ static void *worker_thread(void *opaque) pool->cur_threads--; qemu_cond_signal(&pool->worker_stopped); -qemu_mutex_unlock(&pool->lock); /* * Wake up another thread, in case we got a wakeup but decided * to exit due to pool->cur_threads > pool->max_threads. */ qemu_cond_signal(&pool->request_cond); +qemu_mutex_unlock(&pool->lock); return NULL; } -- Anthony PERARD
[PULL 4/5] xen: Don't pass MemoryListener around by value
From: Peter Maydell Coverity points out (CID 1513106, 1513107) that MemoryListener is a 192 byte struct which we are passing around by value. Switch to passing a const pointer into xen_register_ioreq() and then to xen_do_ioreq_register(). We can also make the file-scope MemoryListener variables const, since nothing changes them. Signed-off-by: Peter Maydell Acked-by: Anthony PERARD Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20230718101057.1110979-1-peter.mayd...@linaro.org> Signed-off-by: Anthony PERARD --- hw/arm/xen_arm.c| 4 ++-- hw/i386/xen/xen-hvm.c | 4 ++-- hw/xen/xen-hvm-common.c | 8 include/hw/xen/xen-hvm-common.h | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c index 044093fec7..1d3e6d481a 100644 --- a/hw/arm/xen_arm.c +++ b/hw/arm/xen_arm.c @@ -37,7 +37,7 @@ #define TYPE_XEN_ARM MACHINE_TYPE_NAME("xenpvh") OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM) -static MemoryListener xen_memory_listener = { +static const MemoryListener xen_memory_listener = { .region_add = xen_region_add, .region_del = xen_region_del, .log_start = NULL, @@ -108,7 +108,7 @@ static void xen_arm_init(MachineState *machine) xam->state = g_new0(XenIOState, 1); -xen_register_ioreq(xam->state, machine->smp.cpus, xen_memory_listener); +xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener); #ifdef CONFIG_TPM if (xam->cfg.tpm_base_addr) { diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c index 3da5a2b23f..f42621e674 100644 --- a/hw/i386/xen/xen-hvm.c +++ b/hw/i386/xen/xen-hvm.c @@ -458,7 +458,7 @@ static void xen_log_global_stop(MemoryListener *listener) xen_in_migration = false; } -static MemoryListener xen_memory_listener = { +static const MemoryListener xen_memory_listener = { .name = "xen-memory", .region_add = xen_region_add, .region_del = xen_region_del, @@ -582,7 +582,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory) state = g_new0(XenIOState, 1); -xen_register_ioreq(state, max_cpus, xen_memory_listener); +xen_register_ioreq(state, max_cpus, &xen_memory_listener); QLIST_INIT(&xen_physmap); xen_read_physmap(state); diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c index 886c3ee944..565dc39c8f 100644 --- a/hw/xen/xen-hvm-common.c +++ b/hw/xen/xen-hvm-common.c @@ -765,8 +765,8 @@ void xen_shutdown_fatal_error(const char *fmt, ...) } static void xen_do_ioreq_register(XenIOState *state, - unsigned int max_cpus, - MemoryListener xen_memory_listener) + unsigned int max_cpus, + const MemoryListener *xen_memory_listener) { int i, rc; @@ -824,7 +824,7 @@ static void xen_do_ioreq_register(XenIOState *state, qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state); -state->memory_listener = xen_memory_listener; +state->memory_listener = *xen_memory_listener; memory_listener_register(&state->memory_listener, &address_space_memory); state->io_listener = xen_io_listener; @@ -842,7 +842,7 @@ static void xen_do_ioreq_register(XenIOState *state, } void xen_register_ioreq(XenIOState *state, unsigned int max_cpus, -MemoryListener xen_memory_listener) +const MemoryListener *xen_memory_listener) { int rc; diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h index f9559e2885..4e9904f1a6 100644 --- a/include/hw/xen/xen-hvm-common.h +++ b/include/hw/xen/xen-hvm-common.h @@ -93,7 +93,7 @@ void xen_device_unrealize(DeviceListener *listener, DeviceState *dev); void xen_hvm_change_state_handler(void *opaque, bool running, RunState rstate); void xen_register_ioreq(XenIOState *state, unsigned int max_cpus, -MemoryListener xen_memory_listener); +const MemoryListener *xen_memory_listener); void cpu_ioreq_pio(ioreq_t *req); #endif /* HW_XEN_HVM_COMMON_H */ -- Anthony PERARD
Re: [PATCH for-8.1] xen: Don't pass MemoryListener around by value
On Tue, Jul 18, 2023 at 11:10:57AM +0100, Peter Maydell wrote: > Coverity points out (CID 1513106, 1513107) that MemoryListener is a > 192 byte struct which we are passing around by value. Switch to > passing a const pointer into xen_register_ioreq() and then to > xen_do_ioreq_register(). We can also make the file-scope > MemoryListener variables const, since nothing changes them. > > Signed-off-by: Peter Maydell Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: QEMU assert (was: [xen-unstable test] 181558: regressions - FAIL)
On Tue, Jul 04, 2023 at 11:56:54AM +0200, Roger Pau Monné wrote: > On Tue, Jul 04, 2023 at 10:37:38AM +0100, Anthony PERARD wrote: > > On Wed, Jun 28, 2023 at 02:31:39PM +0200, Roger Pau Monné wrote: > > > On Fri, Jun 23, 2023 at 03:04:21PM +, osstest service owner wrote: > > > > flight 181558 xen-unstable real [real] > > > > http://logs.test-lab.xenproject.org/osstest/logs/181558/ > > > > > > > > Regressions :-( > > > > > > > > Tests which did not succeed and are blocking, > > > > including tests which could not be run: > > > > test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail REGR. > > > > vs. 181545 > > > > > > The test failing here is hitting the assert in qemu_cond_signal() as > > > called by worker_thread(): > > > > > > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > > > #1 0x7740b535 in __GI_abort () at abort.c:79 > > > #2 0x7740b40f in __assert_fail_base (fmt=0x7756cef0 > > > "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5614abcb > > > "cond->initialized", > > > file=0x5614ab88 > > > "../qemu-xen-dir-remote/util/qemu-thread-posix.c", line=198, > > > function=) at assert.c:92 > > > #3 0x774191a2 in __GI___assert_fail (assertion=0x5614abcb > > > "cond->initialized", file=0x5614ab88 > > > "../qemu-xen-dir-remote/util/qemu-thread-posix.c", line=198, > > > function=0x5614ad80 <__PRETTY_FUNCTION__.17104> > > > "qemu_cond_signal") at assert.c:101 > > > #4 0x55f1c8d2 in qemu_cond_signal (cond=0x7fffb800db30) at > > > ../qemu-xen-dir-remote/util/qemu-thread-posix.c:198 > > > #5 0x55f36973 in worker_thread (opaque=0x7fffb800dab0) at > > > ../qemu-xen-dir-remote/util/thread-pool.c:129 > > > #6 0x55f1d1d2 in qemu_thread_start (args=0x7fffb8000b20) at > > > ../qemu-xen-dir-remote/util/qemu-thread-posix.c:505 > > > #7 0x775b0fa3 in start_thread (arg=) at > > > pthread_create.c:486 > > > #8 0x774e206f in clone () at > > > ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 > > > > > > I've been trying to figure out how it can get in such state, but so > > > far I had no luck. I'm not a QEMU expert, so it's probably better if > > > someone else could handle this. > > > > > > In the failures I've seen, and the reproduction I have, the assert > > > triggers in the QEMU dom0 instance responsible for locally-attaching > > > the disk to dom0 in order to run pygrub. > > > > > > This is also with QEMU 7.2, as testing with upstream QEMU is blocked > > > ATM, so there's a chance it has already been fixed upstream. > > > > > > Thanks, Roger. > > > > So, I've run a test with the latest QEMU and I can still reproduce the > > issue. The test also fails with QEMU 7.1.0. > > > > But, QEMU 7.0 seems to pass the test, even with a start-stop loop of 200 > > iteration. So I'll try to find out if something change in that range. > > Or try to find out why would the thread pool be not initialised > > properly. > > Thanks for looking into this. > > There are a set of changes from Paolo Bonzini: > > 232e9255478f thread-pool: remove stopping variable > 900fa208f506 thread-pool: replace semaphore with condition variable > 3c7b72ddca9c thread-pool: optimize scheduling of completion bottom half > > That landed in 7.1 that seem like possible candidates. I think I've figured out the issue. I've sent a patch: https://lore.kernel.org/qemu-devel/20230714152720.5077-1-anthony.per...@citrix.com/ I did run osstest with this patch, and 200 iteration of stop/start, no more issue of qemu for dom0 disapearing. The issue I've found is osstest not able to ssh to the guest, which seems to be started. And qemu for dom0 is still running. While the report exist: http://logs.test-lab.xenproject.org/osstest/logs/181785/ Cheers, -- Anthony PERARD
[PATCH] thread-pool: signal "request_cond" while locked
From: Anthony PERARD thread_pool_free() might have been called on the `pool`, which would be a reason for worker_thread() to quit. In this case, `pool->request_cond` is been destroyed. If worker_thread() didn't managed to signal `request_cond` before it been destroyed by thread_pool_free(), we got: util/qemu-thread-posix.c:198: qemu_cond_signal: Assertion `cond->initialized' failed. One backtrace: __GI___assert_fail (assertion=0x5614abcb "cond->initialized", file=0x5614ab88 "util/qemu-thread-posix.c", line=198, function=0x5614ad80 <__PRETTY_FUNCTION__.17104> "qemu_cond_signal") at assert.c:101 qemu_cond_signal (cond=0x7fffb800db30) at util/qemu-thread-posix.c:198 worker_thread (opaque=0x7fffb800dab0) at util/thread-pool.c:129 qemu_thread_start (args=0x7fffb8000b20) at util/qemu-thread-posix.c:505 start_thread (arg=) at pthread_create.c:486 Reported here: https://lore.kernel.org/all/ZJwoK50FcnTSfFZ8@MacBook-Air-de-Roger.local/T/#u To avoid issue, keep lock while sending a signal to `request_cond`. Fixes: 900fa208f506 ("thread-pool: replace semaphore with condition variable") Signed-off-by: Anthony PERARD --- There's maybe an issue in thread_pool_submit_aio() as well with signalling `request_cond`, but maybe it's much less likely to be an issue? --- util/thread-pool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/thread-pool.c b/util/thread-pool.c index 0d97888df0..e3d8292d14 100644 --- a/util/thread-pool.c +++ b/util/thread-pool.c @@ -120,13 +120,13 @@ static void *worker_thread(void *opaque) pool->cur_threads--; qemu_cond_signal(&pool->worker_stopped); -qemu_mutex_unlock(&pool->lock); /* * Wake up another thread, in case we got a wakeup but decided * to exit due to pool->cur_threads > pool->max_threads. */ qemu_cond_signal(&pool->request_cond); +qemu_mutex_unlock(&pool->lock); return NULL; } -- Anthony PERARD
[PATCH] xen-block: Avoid leaks on new error path
From: Anthony PERARD Commit 189829399070 ("xen-block: Use specific blockdev driver") introduced a new error path, without taking care of allocated resources. So only allocate the qdicts after the error check, and free both `filename` and `driver` when we are about to return and thus taking care of both success and error path. Coverity only spotted the leak of qdicts (*_layer variables). Reported-by: Peter Maydell Fixes: Coverity CID 1508722, 1398649 Fixes: 189829399070 ("xen-block: Use specific blockdev driver") Signed-off-by: Anthony PERARD --- hw/block/xen-block.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index f099914831..3906b9058b 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -781,14 +781,15 @@ static XenBlockDrive *xen_block_drive_create(const char *id, drive = g_new0(XenBlockDrive, 1); drive->id = g_strdup(id); -file_layer = qdict_new(); -driver_layer = qdict_new(); - rc = stat(filename, &st); if (rc) { error_setg_errno(errp, errno, "Could not stat file '%s'", filename); goto done; } + +file_layer = qdict_new(); +driver_layer = qdict_new(); + if (S_ISBLK(st.st_mode)) { qdict_put_str(file_layer, "driver", "host_device"); } else { @@ -796,7 +797,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id, } qdict_put_str(file_layer, "filename", filename); -g_free(filename); if (mode && *mode != 'w') { qdict_put_bool(file_layer, "read-only", true); @@ -831,7 +831,6 @@ static XenBlockDrive *xen_block_drive_create(const char *id, qdict_put_str(file_layer, "locking", "off"); qdict_put_str(driver_layer, "driver", driver); -g_free(driver); qdict_put(driver_layer, "file", file_layer); @@ -842,6 +841,8 @@ static XenBlockDrive *xen_block_drive_create(const char *id, qobject_unref(driver_layer); done: + g_free(filename); +g_free(driver); if (*errp) { xen_block_drive_destroy(drive, NULL); return NULL; -- Anthony PERARD
Re: QEMU assert (was: [xen-unstable test] 181558: regressions - FAIL)
On Wed, Jun 28, 2023 at 02:31:39PM +0200, Roger Pau Monné wrote: > On Fri, Jun 23, 2023 at 03:04:21PM +, osstest service owner wrote: > > flight 181558 xen-unstable real [real] > > http://logs.test-lab.xenproject.org/osstest/logs/181558/ > > > > Regressions :-( > > > > Tests which did not succeed and are blocking, > > including tests which could not be run: > > test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail REGR. vs. > > 181545 > > The test failing here is hitting the assert in qemu_cond_signal() as > called by worker_thread(): > > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > #1 0x7740b535 in __GI_abort () at abort.c:79 > #2 0x7740b40f in __assert_fail_base (fmt=0x7756cef0 "%s%s%s:%u: > %s%sAssertion `%s' failed.\n%n", assertion=0x5614abcb "cond->initialized", > file=0x5614ab88 "../qemu-xen-dir-remote/util/qemu-thread-posix.c", > line=198, function=) at assert.c:92 > #3 0x774191a2 in __GI___assert_fail (assertion=0x5614abcb > "cond->initialized", file=0x5614ab88 > "../qemu-xen-dir-remote/util/qemu-thread-posix.c", line=198, > function=0x5614ad80 <__PRETTY_FUNCTION__.17104> "qemu_cond_signal") > at assert.c:101 > #4 0x55f1c8d2 in qemu_cond_signal (cond=0x7fffb800db30) at > ../qemu-xen-dir-remote/util/qemu-thread-posix.c:198 > #5 0x55f36973 in worker_thread (opaque=0x7fffb800dab0) at > ../qemu-xen-dir-remote/util/thread-pool.c:129 > #6 0x55f1d1d2 in qemu_thread_start (args=0x7fffb8000b20) at > ../qemu-xen-dir-remote/util/qemu-thread-posix.c:505 > #7 0x775b0fa3 in start_thread (arg=) at > pthread_create.c:486 > #8 0x774e206f in clone () at > ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 > > I've been trying to figure out how it can get in such state, but so > far I had no luck. I'm not a QEMU expert, so it's probably better if > someone else could handle this. > > In the failures I've seen, and the reproduction I have, the assert > triggers in the QEMU dom0 instance responsible for locally-attaching > the disk to dom0 in order to run pygrub. > > This is also with QEMU 7.2, as testing with upstream QEMU is blocked > ATM, so there's a chance it has already been fixed upstream. > > Thanks, Roger. So, I've run a test with the latest QEMU and I can still reproduce the issue. The test also fails with QEMU 7.1.0. But, QEMU 7.0 seems to pass the test, even with a start-stop loop of 200 iteration. So I'll try to find out if something change in that range. Or try to find out why would the thread pool be not initialised properly. Cheers, -- Anthony PERARD
Re: [PATCH v4 0/7] Resolve TYPE_PIIX3_XEN_DEVICE
On Thu, Jun 08, 2023 at 03:43:32PM -0700, Stefano Stabellini wrote: > On Mon, 5 Jun 2023, Bernhard Beschow wrote: > > Am 22. Mai 2023 15:42:03 UTC schrieb Bernhard Beschow : > > > > > > > > >Am 15. Mai 2023 20:52:40 UTC schrieb Stefano Stabellini > > >: > > >>On Sat, 13 May 2023, Bernhard Beschow wrote: > > >>> Am 21. April 2023 07:38:10 UTC schrieb "Michael S. Tsirkin" > > >>> : > > >>> >On Mon, Apr 03, 2023 at 09:41:17AM +0200, Bernhard Beschow wrote: > > >>> >> There is currently a dedicated PIIX3 device model for use under Xen. > > >>> >> By reusing > > >>> >> existing PCI API during initialization this device model can be > > >>> >> eliminated and > > >>> >> the plain PIIX3 device model can be used instead. > > >>> >> > > >>> >> Resolving TYPE_PIIX3_XEN_DEVICE results in less code while also > > >>> >> making Xen > > >>> >> agnostic towards the precise south bridge being used in the PC > > >>> >> machine. The > > >>> >> latter might become particularily interesting once PIIX4 becomes > > >>> >> usable in the > > >>> >> PC machine, avoiding the "Frankenstein" use of PIIX4_ACPI in PIIX3. > > >>> > > > >>> >xen stuff so I assume that tree? > > >>> > > >>> Ping > > >> > > >>I am OK either way. Michael, what do you prefer? > > >> > > >>Normally I would suggest for you to pick up the patches. But as it > > >>happens I'll have to likely send another pull request in a week or two > > >>and I can add these patches to it. > > >> > > >>Let me know your preference and I am happy to follow it. > > > > > >Hi Stefano, > > > > > >Michael's PR was merged last week. How about including this series into > > >your PR then? > > > > Ping > > Sorry for the late reply, it looks like patch #3 breaks the build: Hi Stefano, Sorry I forgot to reply to these mails. I've sent a pull request for this earlier this week (along with other patches I had to send), so the series should be applied now. I guess the build issue is due to trying to apply the same patch again. Cheers, -- Anthony PERARD
[PULL 07/12] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()
From: Bernhard Beschow This is a preparational patch for the next one to make the following more obvious: First, pci_bus_irqs() is now called twice in case of Xen where the second call overrides the pci_set_irq_fn with the Xen variant. Second, pci_bus_set_route_irq_fn() is now also called in Xen mode. Signed-off-by: Bernhard Beschow Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Tested-by: Chuck Zmudzinski Message-Id: <20230312120221.99183-3-shen...@gmail.com> Message-Id: <20230403074124.3925-4-shen...@gmail.com> Signed-off-by: Anthony PERARD --- hw/isa/piix3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c index 6651521a46..800b80f2bb 100644 --- a/hw/isa/piix3.c +++ b/hw/isa/piix3.c @@ -409,7 +409,7 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp) PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev); PCIBus *pci_bus = pci_get_bus(dev); -pci_piix3_realize(dev, errp); +piix3_realize(dev, errp); if (*errp) { return; } -- Anthony PERARD
[PULL 02/12] hw/xen: Fix memory leak in libxenstore_open() for Xen
From: David Woodhouse There was a superfluous allocation of the XS handle, leading to it being leaked on both the error path and the success path (where it gets allocated again). Spotted by Coverity (CID 1508098). Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to internal emulation") Suggested-by: Peter Maydell Signed-off-by: David Woodhouse Reviewed-by: Peter Maydell Reviewed-by: Paul Durrant Message-Id: <20230412185102.441523-3-dw...@infradead.org> Signed-off-by: Anthony PERARD --- hw/xen/xen-operations.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c index 4b78fbf4bd..3d213d28df 100644 --- a/hw/xen/xen-operations.c +++ b/hw/xen/xen-operations.c @@ -287,7 +287,7 @@ static void watch_event(void *opaque) static struct qemu_xs_handle *libxenstore_open(void) { struct xs_handle *xsh = xs_open(0); -struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1); +struct qemu_xs_handle *h; if (!xsh) { return NULL; -- Anthony PERARD
[PULL 11/12] hw/isa/piix3: Resolve redundant TYPE_PIIX3_XEN_DEVICE
From: Bernhard Beschow During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of TYPE_PIIX3_DEVICE. Remove this redundancy. Signed-off-by: Bernhard Beschow Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Tested-by: Chuck Zmudzinski Message-Id: <20230312120221.99183-7-shen...@gmail.com> Message-Id: <20230403074124.3925-8-shen...@gmail.com> Signed-off-by: Anthony PERARD --- hw/i386/pc_piix.c | 5 ++--- hw/isa/piix3.c| 15 --- include/hw/southbridge/piix.h | 1 - 3 files changed, 2 insertions(+), 19 deletions(-) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 2ed2b3f3c6..42af03dbb4 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -239,8 +239,6 @@ static void pc_init1(MachineState *machine, if (pcmc->pci_enabled) { PIIX3State *piix3; PCIDevice *pci_dev; -const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE - : TYPE_PIIX3_DEVICE; pci_bus = i440fx_init(pci_type, i440fx_host, @@ -253,7 +251,8 @@ static void pc_init1(MachineState *machine, : pc_pci_slot_get_pirq); pcms->bus = pci_bus; -pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type); +pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, + TYPE_PIIX3_DEVICE); if (xen_enabled()) { pci_device_set_intx_routing_notifier( diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c index 0549e043ca..117024e450 100644 --- a/hw/isa/piix3.c +++ b/hw/isa/piix3.c @@ -30,7 +30,6 @@ #include "hw/irq.h" #include "hw/qdev-properties.h" #include "hw/isa/isa.h" -#include "hw/xen/xen.h" #include "sysemu/runstate.h" #include "migration/vmstate.h" #include "hw/acpi/acpi_aml_interface.h" @@ -381,24 +380,10 @@ static const TypeInfo piix3_info = { .class_init= piix3_class_init, }; -static void piix3_xen_class_init(ObjectClass *klass, void *data) -{ -PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - -k->realize = piix3_realize; -} - -static const TypeInfo piix3_xen_info = { -.name = TYPE_PIIX3_XEN_DEVICE, -.parent= TYPE_PIIX3_PCI_DEVICE, -.class_init= piix3_xen_class_init, -}; - static void piix3_register_types(void) { type_register_static(&piix3_pci_type_info); type_register_static(&piix3_info); -type_register_static(&piix3_xen_info); } type_init(piix3_register_types) diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h index a840340308..278171752f 100644 --- a/include/hw/southbridge/piix.h +++ b/include/hw/southbridge/piix.h @@ -67,7 +67,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE, TYPE_PIIX3_PCI_DEVICE) #define TYPE_PIIX3_DEVICE "PIIX3" -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" #endif -- Anthony PERARD
[PULL 00/12] xen queue
From: Anthony PERARD The following changes since commit f5e6786de4815751b0a3d2235c760361f228ea48: Merge tag 'pull-target-arm-20230606' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2023-06-06 12:11:34 -0700) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20230607 for you to fetch changes up to 9000666052f99ed4217e75b73636acae61e6fc2c: xen-block: fix segv on unrealize (2023-06-07 15:07:10 +0100) Xen queue - fix for xen-block segv - Resolve TYPE_PIIX3_XEN_DEVICE - Xen emulation build/Coverity fixes -------- Anthony PERARD (1): xen-block: fix segv on unrealize Bernhard Beschow (7): include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq() hw/pci/pci.c: Don't leak PCIBus::irq_count[] in pci_bus_irqs() hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize() hw/isa/piix3: Wire up Xen PCI IRQ handling outside of PIIX3 hw/isa/piix3: Avoid Xen-specific variant of piix3_write_config() hw/isa/piix3: Resolve redundant k->config_write assignments hw/isa/piix3: Resolve redundant TYPE_PIIX3_XEN_DEVICE David Woodhouse (4): hw/xen: Simplify emulated Xen platform init hw/xen: Fix memory leak in libxenstore_open() for Xen xen: Drop support for Xen versions below 4.7.1 hw/xen: Fix broken check for invalid state in xs_be_open() hw/i386/kvm/xen_evtchn.c | 40 hw/i386/kvm/xen_evtchn.h | 3 +- hw/i386/kvm/xen_xenstore.c| 2 +- hw/i386/pc.c | 13 ++--- hw/i386/pc_piix.c | 36 -- hw/i386/xen/xen-hvm.c | 2 +- hw/isa/piix3.c| 60 +-- hw/pci/pci.c | 2 + hw/xen/xen-bus.c | 6 ++- hw/xen/xen-operations.c | 59 +-- include/hw/southbridge/piix.h | 1 - include/hw/xen/xen.h | 2 +- include/hw/xen/xen_native.h | 107 +- meson.build | 5 +- scripts/xen-detect.c | 60 --- stubs/xen-hw-stub.c | 2 +- 16 files changed, 73 insertions(+), 327 deletions(-)
[PULL 10/12] hw/isa/piix3: Resolve redundant k->config_write assignments
From: Bernhard Beschow The previous patch unified handling of piix3_write_config() accross the PIIX3 device models which allows for assigning k->config_write once in the base class. Signed-off-by: Bernhard Beschow Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Tested-by: Chuck Zmudzinski Message-Id: <20230312120221.99183-6-shen...@gmail.com> Message-Id: <20230403074124.3925-7-shen...@gmail.com> Signed-off-by: Anthony PERARD --- hw/isa/piix3.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c index 5e8ac98ae0..0549e043ca 100644 --- a/hw/isa/piix3.c +++ b/hw/isa/piix3.c @@ -322,6 +322,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data) PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass); +k->config_write = piix3_write_config; dc->reset = piix3_reset; dc->desc= "ISA bridge"; dc->vmsd= &vmstate_piix3; @@ -371,7 +372,6 @@ static void piix3_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); -k->config_write = piix3_write_config; k->realize = piix3_realize; } @@ -385,7 +385,6 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); -k->config_write = piix3_write_config; k->realize = piix3_realize; } -- Anthony PERARD
[PULL 06/12] hw/pci/pci.c: Don't leak PCIBus::irq_count[] in pci_bus_irqs()
From: Bernhard Beschow When calling pci_bus_irqs() multiple times on the same object without calling pci_bus_irqs_cleanup() in between PCIBus::irq_count[] is currently leaked. Let's fix this because Xen will do just that in a few commits, and because calling pci_bus_irqs_cleanup() in between seems fragile and cumbersome. Note that pci_bus_irqs_cleanup() now has to NULL irq_count such that pci_bus_irqs() doesn't do a double free. Signed-off-by: Bernhard Beschow Reviewed-by: Michael S. Tsirkin Tested-by: Chuck Zmudzinski Message-Id: <20230403074124.3925-3-shen...@gmail.com> Signed-off-by: Anthony PERARD --- hw/pci/pci.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 1cc7c89036..9b7b4d7c18 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -560,6 +560,7 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, bus->set_irq = set_irq; bus->irq_opaque = irq_opaque; bus->nirq = nirq; +g_free(bus->irq_count); bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0])); } @@ -575,6 +576,7 @@ void pci_bus_irqs_cleanup(PCIBus *bus) bus->irq_opaque = NULL; bus->nirq = 0; g_free(bus->irq_count); +bus->irq_count = NULL; } PCIBus *pci_register_root_bus(DeviceState *parent, const char *name, -- Anthony PERARD
[PULL 08/12] hw/isa/piix3: Wire up Xen PCI IRQ handling outside of PIIX3
From: Bernhard Beschow xen_intx_set_irq() doesn't depend on PIIX3State. In order to resolve TYPE_PIIX3_XEN_DEVICE and in order to make Xen agnostic about the precise south bridge being used, set up Xen's PCI IRQ handling of PIIX3 in the board. Signed-off-by: Bernhard Beschow Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Tested-by: Chuck Zmudzinski Message-Id: <20230312120221.99183-4-shen...@gmail.com> Message-Id: <20230403074124.3925-5-shen...@gmail.com> Signed-off-by: Anthony PERARD --- hw/i386/pc_piix.c | 13 + hw/isa/piix3.c| 24 +--- 2 files changed, 14 insertions(+), 23 deletions(-) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index d5b0dcd1fe..11af191513 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -71,6 +71,7 @@ #include "kvm/kvm-cpu.h" #define MAX_IDE_BUS 2 +#define XEN_IOAPIC_NUM_PIRQS 128ULL #ifdef CONFIG_IDE_ISA static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 }; @@ -238,6 +239,18 @@ static void pc_init1(MachineState *machine, pcms->bus = pci_bus; pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type); + +if (xen_enabled()) { +/* + * Xen supports additional interrupt routes from the PCI devices to + * the IOAPIC: the four pins of each PCI device on the bus are also + * connected to the IOAPIC directly. + * These additional routes can be discovered through ACPI. + */ +pci_bus_irqs(pci_bus, xen_intx_set_irq, pci_dev, + XEN_IOAPIC_NUM_PIRQS); +} + piix3 = PIIX3_PCI_DEVICE(pci_dev); piix3->pic = x86ms->gsi; piix3_devfn = piix3->dev.devfn; diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c index 800b80f2bb..0b7e9ade0d 100644 --- a/hw/isa/piix3.c +++ b/hw/isa/piix3.c @@ -35,8 +35,6 @@ #include "migration/vmstate.h" #include "hw/acpi/acpi_aml_interface.h" -#define XEN_IOAPIC_NUM_PIRQS128ULL - static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq) { qemu_set_irq(piix3->pic[pic_irq], @@ -403,32 +401,12 @@ static const TypeInfo piix3_info = { .class_init= piix3_class_init, }; -static void piix3_xen_realize(PCIDevice *dev, Error **errp) -{ -ERRP_GUARD(); -PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev); -PCIBus *pci_bus = pci_get_bus(dev); - -piix3_realize(dev, errp); -if (*errp) { -return; -} - -/* - * Xen supports additional interrupt routes from the PCI devices to - * the IOAPIC: the four pins of each PCI device on the bus are also - * connected to the IOAPIC directly. - * These additional routes can be discovered through ACPI. - */ -pci_bus_irqs(pci_bus, xen_intx_set_irq, piix3, XEN_IOAPIC_NUM_PIRQS); -} - static void piix3_xen_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); k->config_write = piix3_write_config_xen; -k->realize = piix3_xen_realize; +k->realize = piix3_realize; } static const TypeInfo piix3_xen_info = { -- Anthony PERARD
[PULL 01/12] hw/xen: Simplify emulated Xen platform init
From: David Woodhouse I initially put the basic platform init (overlay pages, grant tables, event channels) into mc->kvm_type because that was the earliest place that could sensibly test for xen_mode==XEN_EMULATE. The intent was to do this early enough that we could then initialise the XenBus and other parts which would have depended on them, from a generic location for both Xen and KVM/Xen in the PC-specific code, as seen in https://lore.kernel.org/qemu-devel/20230116221919.1124201-16-dw...@infradead.org/ However, then the Xen on Arm patches came along, and *they* wanted to do the XenBus init from a 'generic' Xen-specific location instead: https://lore.kernel.org/qemu-devel/20230210222729.957168-4-sstabell...@kernel.org/ Since there's no generic location that covers all three, I conceded to do it for XEN_EMULATE mode in pc_basic_devices_init(). And now there's absolutely no point in having some of the platform init done from pc_machine_kvm_type(); we can move it all up to live in a single place in pc_basic_devices_init(). This has the added benefit that we can drop the separate xen_evtchn_connect_gsis() function completely, and pass just the system GSIs in directly to xen_evtchn_create(). While I'm at it, it does no harm to explicitly pass in the *number* of said GSIs, because it does make me twitch a bit to pass an array of impicit size. During the lifetime of the KVM/Xen patchset, that had already changed (albeit just cosmetically) from GSI_NUM_PINS to IOAPIC_NUM_PINS. And document a bit better that this is for the *output* GSI for raising CPU0's events when the per-CPU vector isn't available. The fact that we create a whole set of them and then only waggle the one we're told to, instead of having a single output and only *connecting* it to the GSI that it should be connected to, is still non-intuitive for me. Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Message-Id: <20230412185102.441523-2-dw...@infradead.org> Signed-off-by: Anthony PERARD --- hw/i386/kvm/xen_evtchn.c | 40 hw/i386/kvm/xen_evtchn.h | 3 +-- hw/i386/pc.c | 13 - 3 files changed, 25 insertions(+), 31 deletions(-) diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c index 3048329474..3d810dbd59 100644 --- a/hw/i386/kvm/xen_evtchn.c +++ b/hw/i386/kvm/xen_evtchn.c @@ -147,7 +147,10 @@ struct XenEvtchnState { QemuMutex port_lock; uint32_t nr_ports; XenEvtchnPort port_table[EVTCHN_2L_NR_CHANNELS]; -qemu_irq gsis[IOAPIC_NUM_PINS]; + +/* Connected to the system GSIs for raising callback as GSI / INTx */ +unsigned int nr_callback_gsis; +qemu_irq *callback_gsis; struct xenevtchn_handle *be_handles[EVTCHN_2L_NR_CHANNELS]; @@ -299,7 +302,7 @@ static void gsi_assert_bh(void *opaque) } } -void xen_evtchn_create(void) +void xen_evtchn_create(unsigned int nr_gsis, qemu_irq *system_gsis) { XenEvtchnState *s = XEN_EVTCHN(sysbus_create_simple(TYPE_XEN_EVTCHN, -1, NULL)); @@ -310,8 +313,19 @@ void xen_evtchn_create(void) qemu_mutex_init(&s->port_lock); s->gsi_bh = aio_bh_new(qemu_get_aio_context(), gsi_assert_bh, s); -for (i = 0; i < IOAPIC_NUM_PINS; i++) { -sysbus_init_irq(SYS_BUS_DEVICE(s), &s->gsis[i]); +/* + * These are the *output* GSI from event channel support, for + * signalling CPU0's events via GSI or PCI INTx instead of the + * per-CPU vector. We create a *set* of irqs and connect one to + * each of the system GSIs which were passed in from the platform + * code, and then just trigger the right one as appropriate from + * xen_evtchn_set_callback_level(). + */ +s->nr_callback_gsis = nr_gsis; +s->callback_gsis = g_new0(qemu_irq, nr_gsis); +for (i = 0; i < nr_gsis; i++) { +sysbus_init_irq(SYS_BUS_DEVICE(s), &s->callback_gsis[i]); +sysbus_connect_irq(SYS_BUS_DEVICE(s), i, system_gsis[i]); } /* @@ -336,20 +350,6 @@ void xen_evtchn_create(void) xen_evtchn_ops = &emu_evtchn_backend_ops; } -void xen_evtchn_connect_gsis(qemu_irq *system_gsis) -{ -XenEvtchnState *s = xen_evtchn_singleton; -int i; - -if (!s) { -return; -} - -for (i = 0; i < IOAPIC_NUM_PINS; i++) { -sysbus_connect_irq(SYS_BUS_DEVICE(s), i, system_gsis[i]); -} -} - static void xen_evtchn_register_types(void) { type_register_static(&xen_evtchn_info); @@ -430,8 +430,8 @@ void xen_evtchn_set_callback_level(int level) return; } -if (s->callback_gsi && s->callback_gsi < IOAPIC_NUM_PINS) { -qemu_set_irq(s->gsis[s->callback_gsi], level); +if (s->callback_gsi && s->callback_gsi < s->nr_callback_gsis) { +qemu_set_irq(s->callback_gsis[s->callback_gsi]
[PULL 05/12] include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq()
From: Bernhard Beschow xen_piix3_set_irq() isn't PIIX specific: PIIX is a single PCI device while xen_piix3_set_irq() maps multiple PCI devices to their respective IRQs, which is board-specific. Rename xen_piix3_set_irq() to communicate this. Also rename XEN_PIIX_NUM_PIRQS to XEN_IOAPIC_NUM_PIRQS since the Xen's IOAPIC rather than PIIX has this many interrupt routes. Signed-off-by: Bernhard Beschow Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Tested-by: Chuck Zmudzinski Message-Id: <20230312120221.99183-2-shen...@gmail.com> Message-Id: <20230403074124.3925-2-shen...@gmail.com> Signed-off-by: Anthony PERARD --- hw/i386/xen/xen-hvm.c | 2 +- hw/isa/piix3.c| 4 ++-- include/hw/xen/xen.h | 2 +- stubs/xen-hw-stub.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c index 56641a550e..ab8f1b61ee 100644 --- a/hw/i386/xen/xen-hvm.c +++ b/hw/i386/xen/xen-hvm.c @@ -143,7 +143,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num) return irq_num + (PCI_SLOT(pci_dev->devfn) << 2); } -void xen_piix3_set_irq(void *opaque, int irq_num, int level) +void xen_intx_set_irq(void *opaque, int irq_num, int level) { xen_set_pci_intx_level(xen_domid, 0, 0, irq_num >> 2, irq_num & 3, level); diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c index f9103ea45a..6651521a46 100644 --- a/hw/isa/piix3.c +++ b/hw/isa/piix3.c @@ -35,7 +35,7 @@ #include "migration/vmstate.h" #include "hw/acpi/acpi_aml_interface.h" -#define XEN_PIIX_NUM_PIRQS 128ULL +#define XEN_IOAPIC_NUM_PIRQS128ULL static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq) { @@ -420,7 +420,7 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp) * connected to the IOAPIC directly. * These additional routes can be discovered through ACPI. */ -pci_bus_irqs(pci_bus, xen_piix3_set_irq, piix3, XEN_PIIX_NUM_PIRQS); +pci_bus_irqs(pci_bus, xen_intx_set_irq, piix3, XEN_IOAPIC_NUM_PIRQS); } static void piix3_xen_class_init(ObjectClass *klass, void *data) diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h index 2bd8ec742d..37ecc91fc3 100644 --- a/include/hw/xen/xen.h +++ b/include/hw/xen/xen.h @@ -39,7 +39,7 @@ extern bool xen_domid_restrict; int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num); int xen_set_pci_link_route(uint8_t link, uint8_t irq); -void xen_piix3_set_irq(void *opaque, int irq_num, int level); +void xen_intx_set_irq(void *opaque, int irq_num, int level); void xen_hvm_inject_msi(uint64_t addr, uint32_t data); int xen_is_pirq_msi(uint32_t msi_data); diff --git a/stubs/xen-hw-stub.c b/stubs/xen-hw-stub.c index 34a22f2ad7..7d7ffe83a9 100644 --- a/stubs/xen-hw-stub.c +++ b/stubs/xen-hw-stub.c @@ -15,7 +15,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num) return -1; } -void xen_piix3_set_irq(void *opaque, int irq_num, int level) +void xen_intx_set_irq(void *opaque, int irq_num, int level) { } -- Anthony PERARD
[PULL 12/12] xen-block: fix segv on unrealize
From: Anthony PERARD Backtrace: qemu_lockcnt_lock (lockcnt=0xb4) at ../util/lockcnt.c:238 aio_set_fd_handler (ctx=0x0, fd=51, is_external=true, io_read=0x0, io_write=0x0, io_poll=0x0, io_poll_ready=0x0, opaque=0x0) at ../util/aio-posix.c:119 xen_device_unbind_event_channel (xendev=0x55c6da5b5000, channel=0x55c6da6c4c80, errp=0x7fff641ac608) at ../hw/xen/xen-bus.c:926 xen_block_dataplane_stop (dataplane=0x55c6da6ddbe0) at ../hw/block/dataplane/xen-block.c:719 xen_block_disconnect (xendev=0x55c6da5b5000, errp=0x0) at ../hw/block/xen-block.c:48 xen_block_unrealize (xendev=0x55c6da5b5000) at ../hw/block/xen-block.c:154 xen_device_unrealize (dev=0x55c6da5b5000) at ../hw/xen/xen-bus.c:956 xen_device_exit (n=0x55c6da5b50d0, data=0x0) at ../hw/xen/xen-bus.c:985 notifier_list_notify (list=0x55c6d91f9820 , data=0x0) at ../util/notify.c:39 qemu_run_exit_notifiers () at ../softmmu/runstate.c:760 Fixes: f6eac904f682 ("xen-block: implement BlockDevOps->drained_begin()") Signed-off-by: Anthony PERARD Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Stefan Hajnoczi Message-Id: <20230606131605.55596-1-anthony.per...@citrix.com> Signed-off-by: Anthony PERARD --- hw/xen/xen-bus.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index 1e08cf027a..ece8ec40cd 100644 --- a/hw/xen/xen-bus.c +++ b/hw/xen/xen-bus.c @@ -923,8 +923,10 @@ void xen_device_unbind_event_channel(XenDevice *xendev, QLIST_REMOVE(channel, list); -aio_set_fd_handler(channel->ctx, qemu_xen_evtchn_fd(channel->xeh), - NULL, NULL, NULL, NULL, NULL); +if (channel->ctx) { +aio_set_fd_handler(channel->ctx, qemu_xen_evtchn_fd(channel->xeh), + NULL, NULL, NULL, NULL, NULL); +} if (qemu_xen_evtchn_unbind(channel->xeh, channel->local_port) < 0) { error_setg_errno(errp, errno, "xenevtchn_unbind failed"); -- Anthony PERARD
[PULL 04/12] hw/xen: Fix broken check for invalid state in xs_be_open()
From: David Woodhouse Coverity points out that if (!s && !s->impl) isn't really what we intended to do here. CID 1508131. Fixes: 032475127225 ("hw/xen: Add emulated implementation of XenStore operations") Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Reviewed-by: Peter Maydell Message-Id: <20230412185102.441523-6-dw...@infradead.org> Signed-off-by: Anthony PERARD --- hw/i386/kvm/xen_xenstore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c index 0b189c6ab8..133d89e953 100644 --- a/hw/i386/kvm/xen_xenstore.c +++ b/hw/i386/kvm/xen_xenstore.c @@ -1688,7 +1688,7 @@ static struct qemu_xs_handle *xs_be_open(void) XenXenstoreState *s = xen_xenstore_singleton; struct qemu_xs_handle *h; -if (!s && !s->impl) { +if (!s || !s->impl) { errno = -ENOSYS; return NULL; } -- Anthony PERARD
[PULL 03/12] xen: Drop support for Xen versions below 4.7.1
From: David Woodhouse In restructuring to allow for internal emulation of Xen functionality, I broke compatibility for Xen 4.6 and earlier. Fix this by explicitly removing support for anything older than 4.7.1, which is also ancient but it does still build, and the compatibility support for it is fairly unintrusive. Fixes: 15e283c5b684 ("hw/xen: Add foreignmem operations to allow redirection to internal emulation") Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Message-Id: <20230412185102.441523-4-dw...@infradead.org> Signed-off-by: Anthony PERARD --- hw/xen/xen-operations.c | 57 +-- include/hw/xen/xen_native.h | 107 +--- meson.build | 5 +- scripts/xen-detect.c| 60 4 files changed, 3 insertions(+), 226 deletions(-) diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c index 3d213d28df..e00983ec44 100644 --- a/hw/xen/xen-operations.c +++ b/hw/xen/xen-operations.c @@ -28,46 +28,13 @@ #include /* - * We don't support Xen prior to 4.2.0. + * We don't support Xen prior to 4.7.1. */ -/* Xen 4.2 through 4.6 */ -#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701 - -typedef xc_evtchn xenevtchn_handle; -typedef evtchn_port_or_error_t xenevtchn_port_or_error_t; - -#define xenevtchn_open(l, f) xc_evtchn_open(l, f); -#define xenevtchn_close(h) xc_evtchn_close(h) -#define xenevtchn_fd(h) xc_evtchn_fd(h) -#define xenevtchn_pending(h) xc_evtchn_pending(h) -#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p) -#define xenevtchn_bind_interdomain(h, d, p) xc_evtchn_bind_interdomain(h, d, p) -#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p) -#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p) - -typedef xc_gnttab xengnttab_handle; - -#define xengnttab_open(l, f) xc_gnttab_open(l, f) -#define xengnttab_close(h) xc_gnttab_close(h) -#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n) -#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h, d, r, p) -#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n) -#define xengnttab_map_grant_refs(h, c, d, r, p) \ -xc_gnttab_map_grant_refs(h, c, d, r, p) -#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \ -xc_gnttab_map_domain_grant_refs(h, c, d, r, p) - -typedef xc_interface xenforeignmemory_handle; - -#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */ - #include #include #include -#endif - /* Xen before 4.8 */ static int libxengnttab_fallback_grant_copy(xengnttab_handle *xgt, @@ -223,26 +190,6 @@ static struct gnttab_backend_ops libxengnttab_backend_ops = { .unmap = libxengnttab_backend_unmap, }; -#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701 - -static void *libxenforeignmem_backend_map(uint32_t dom, void *addr, int prot, - size_t pages, xfn_pfn_t *pfns, - int *errs) -{ -if (errs) { -return xc_map_foreign_bulk(xen_xc, dom, prot, pfns, errs, pages); -} else { -return xc_map_foreign_pages(xen_xc, dom, prot, pfns, pages); -} -} - -static int libxenforeignmem_backend_unmap(void *addr, size_t pages) -{ -return munmap(addr, pages * XC_PAGE_SIZE); -} - -#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */ - static void *libxenforeignmem_backend_map(uint32_t dom, void *addr, int prot, size_t pages, xen_pfn_t *pfns, int *errs) @@ -256,8 +203,6 @@ static int libxenforeignmem_backend_unmap(void *addr, size_t pages) return xenforeignmemory_unmap(xen_fmem, addr, pages); } -#endif - struct foreignmem_backend_ops libxenforeignmem_backend_ops = { .map = libxenforeignmem_backend_map, .unmap = libxenforeignmem_backend_unmap, diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h index 6bcc83baf9..f11eb423e3 100644 --- a/include/hw/xen/xen_native.h +++ b/include/hw/xen/xen_native.h @@ -24,23 +24,11 @@ extern xc_interface *xen_xc; /* - * We don't support Xen prior to 4.2.0. + * We don't support Xen prior to 4.7.1. */ -/* Xen 4.2 through 4.6 */ -#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701 - -typedef xc_interface xenforeignmemory_handle; - -#define xenforeignmemory_open(l, f) xen_xc -#define xenforeignmemory_close(h) - -#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */ - #include -#endif - extern xenforeignmemory_handle *xen_fmem; #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900 @@ -148,8 +136,6 @@ static inline xendevicemodel_handle *xendevicemodel_open( return xen_xc; } -#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40500 - static inline int xendevicemodel_create_ioreq_server( xendevicemodel_handle *dmod, domid_t domid, int handle_bufioreq, ioservid_t *id) @@ -211,8 +197,6 @@ static inline int xendevicemodel_set_ioreq_server_state( return xc_hvm_se
[PULL 09/12] hw/isa/piix3: Avoid Xen-specific variant of piix3_write_config()
From: Bernhard Beschow Subscribe to pci_bus_fire_intx_routing_notifier() instead which allows for having a common piix3_write_config() for the PIIX3 device models. While at it, move the subscription into machine code to facilitate resolving TYPE_PIIX3_XEN_DEVICE. In a possible future followup, pci_bus_fire_intx_routing_notifier() could be adjusted in such a way that subscribing to it doesn't require knowledge of the device firing it. Signed-off-by: Bernhard Beschow Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Tested-by: Chuck Zmudzinski Message-Id: <20230312120221.99183-5-shen...@gmail.com> Message-Id: <20230403074124.3925-6-shen...@gmail.com> Signed-off-by: Anthony PERARD --- hw/i386/pc_piix.c | 18 ++ hw/isa/piix3.c| 22 +- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 11af191513..2ed2b3f3c6 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -90,6 +90,21 @@ static int pc_pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx) return (pci_intx + slot_addend) & 3; } +static void piix_intx_routing_notifier_xen(PCIDevice *dev) +{ +int i; + +/* Scan for updates to PCI link routes (0x60-0x63). */ +for (i = 0; i < PIIX_NUM_PIRQS; i++) { +uint8_t v = dev->config_read(dev, PIIX_PIRQCA + i, 1); +if (v & 0x80) { +v = 0; +} +v &= 0xf; +xen_set_pci_link_route(i, v); +} +} + /* PC hardware initialisation */ static void pc_init1(MachineState *machine, const char *host_type, const char *pci_type) @@ -241,6 +256,9 @@ static void pc_init1(MachineState *machine, pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type); if (xen_enabled()) { +pci_device_set_intx_routing_notifier( +pci_dev, piix_intx_routing_notifier_xen); + /* * Xen supports additional interrupt routes from the PCI devices to * the IOAPIC: the four pins of each PCI device on the bus are also diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c index 0b7e9ade0d..5e8ac98ae0 100644 --- a/hw/isa/piix3.c +++ b/hw/isa/piix3.c @@ -122,26 +122,6 @@ static void piix3_write_config(PCIDevice *dev, } } -static void piix3_write_config_xen(PCIDevice *dev, - uint32_t address, uint32_t val, int len) -{ -int i; - -/* Scan for updates to PCI link routes (0x60-0x63). */ -for (i = 0; i < len; i++) { -uint8_t v = (val >> (8 * i)) & 0xff; -if (v & 0x80) { -v = 0; -} -v &= 0xf; -if (((address + i) >= PIIX_PIRQCA) && ((address + i) <= PIIX_PIRQCD)) { -xen_set_pci_link_route(address + i - PIIX_PIRQCA, v); -} -} - -piix3_write_config(dev, address, val, len); -} - static void piix3_reset(DeviceState *dev) { PIIX3State *d = PIIX3_PCI_DEVICE(dev); @@ -405,7 +385,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); -k->config_write = piix3_write_config_xen; +k->config_write = piix3_write_config; k->realize = piix3_realize; } -- Anthony PERARD
[PATCH] xen-block: fix segv on unrealize
From: Anthony PERARD Backtrace: qemu_lockcnt_lock (lockcnt=0xb4) at ../util/lockcnt.c:238 aio_set_fd_handler (ctx=0x0, fd=51, is_external=true, io_read=0x0, io_write=0x0, io_poll=0x0, io_poll_ready=0x0, opaque=0x0) at ../util/aio-posix.c:119 xen_device_unbind_event_channel (xendev=0x55c6da5b5000, channel=0x55c6da6c4c80, errp=0x7fff641ac608) at ../hw/xen/xen-bus.c:926 xen_block_dataplane_stop (dataplane=0x55c6da6ddbe0) at ../hw/block/dataplane/xen-block.c:719 xen_block_disconnect (xendev=0x55c6da5b5000, errp=0x0) at ../hw/block/xen-block.c:48 xen_block_unrealize (xendev=0x55c6da5b5000) at ../hw/block/xen-block.c:154 xen_device_unrealize (dev=0x55c6da5b5000) at ../hw/xen/xen-bus.c:956 xen_device_exit (n=0x55c6da5b50d0, data=0x0) at ../hw/xen/xen-bus.c:985 notifier_list_notify (list=0x55c6d91f9820 , data=0x0) at ../util/notify.c:39 qemu_run_exit_notifiers () at ../softmmu/runstate.c:760 Fixes: f6eac904f682 ("xen-block: implement BlockDevOps->drained_begin()") Signed-off-by: Anthony PERARD -- CC: Stefan Hajnoczi --- hw/xen/xen-bus.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index 1e08cf027a..ece8ec40cd 100644 --- a/hw/xen/xen-bus.c +++ b/hw/xen/xen-bus.c @@ -923,8 +923,10 @@ void xen_device_unbind_event_channel(XenDevice *xendev, QLIST_REMOVE(channel, list); -aio_set_fd_handler(channel->ctx, qemu_xen_evtchn_fd(channel->xeh), - NULL, NULL, NULL, NULL, NULL); +if (channel->ctx) { +aio_set_fd_handler(channel->ctx, qemu_xen_evtchn_fd(channel->xeh), + NULL, NULL, NULL, NULL, NULL); +} if (qemu_xen_evtchn_unbind(channel->xeh, channel->local_port) < 0) { error_setg_errno(errp, errno, "xenevtchn_unbind failed"); -- Anthony PERARD
Re: [PATCH v5 12/21] xen-block: implement BlockDevOps->drained_begin()
On Thu, May 04, 2023 at 03:53:18PM -0400, Stefan Hajnoczi wrote: > @@ -819,11 +841,9 @@ void xen_block_dataplane_start(XenBlockDataPlane > *dataplane, > blk_set_aio_context(dataplane->blk, dataplane->ctx, NULL); > aio_context_release(old_context); > > -/* Only reason for failure is a NULL channel */ > -aio_context_acquire(dataplane->ctx); > -xen_device_set_event_channel_context(xendev, dataplane->event_channel, > - dataplane->ctx, &error_abort); > -aio_context_release(dataplane->ctx); > +if (!blk_in_drain(dataplane->blk)) { There's maybe something missing in the patch. xen_block_dataplane_start() calls xen_device_bind_event_channel() just before xen_block_dataplane_attach(). And xen_device_bind_event_channel() sets the event context to qemu_get_aio_context() instead of NULL. So, even if we don't call xen_block_dataplane_attach() while in drain, there's already a fd_handler attach to the fd. So should xen_device_bind_event_channel() be changed as well? Or maybe a call to xen_block_dataplane_detach() would be enough. (There's only one user of xen_device_bind_event_channel() at the moment so I don't know if other implementation making use of this API will want to call set_event_channel_context or not.) > + xen_block_dataplane_attach(dataplane); > +} > > return; > -- Anthony PERARD
Re: [PATCH v5 13/21] hw/xen: do not set is_external=true on evtchn fds
On Thu, May 04, 2023 at 03:53:19PM -0400, Stefan Hajnoczi wrote: > is_external=true suspends fd handlers between aio_disable_external() and > aio_enable_external(). The block layer's drain operation uses this > mechanism to prevent new I/O from sneaking in between > bdrv_drained_begin() and bdrv_drained_end(). > > The previous commit converted the xen-block device to use BlockDevOps > .drained_begin/end() callbacks. It no longer relies on is_external=true > so it is safe to pass is_external=false. > > This is part of ongoing work to remove the aio_disable_external() API. > > Signed-off-by: Stefan Hajnoczi Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v5 12/21] xen-block: implement BlockDevOps->drained_begin()
On Thu, May 04, 2023 at 03:53:18PM -0400, Stefan Hajnoczi wrote: > Detach event channels during drained sections to stop I/O submission > from the ring. xen-block is no longer reliant on aio_disable_external() > after this patch. This will allow us to remove the > aio_disable_external() API once all other code that relies on it is > converted. > > Extend xen_device_set_event_channel_context() to allow ctx=NULL. The > event channel still exists but the event loop does not monitor the file > descriptor. Event channel processing can resume by calling > xen_device_set_event_channel_context() with a non-NULL ctx. > > Factor out xen_device_set_event_channel_context() calls in > hw/block/dataplane/xen-block.c into attach/detach helper functions. > Incidentally, these don't require the AioContext lock because > aio_set_fd_handler() is thread-safe. > > It's safer to register BlockDevOps after the dataplane instance has been > created. The BlockDevOps .drained_begin/end() callbacks depend on the > dataplane instance, so move the blk_set_dev_ops() call after > xen_block_dataplane_create(). > > Signed-off-by: Stefan Hajnoczi Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH] xen: Fix host pci for stubdom
On Sun, Mar 19, 2023 at 08:05:54PM -0400, Jason Andryuk wrote: > diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c > index 8c6e9a1716..51a72b432d 100644 > --- a/hw/xen/xen-host-pci-device.c > +++ b/hw/xen/xen-host-pci-device.c > @@ -33,13 +34,101 @@ > #define IORESOURCE_PREFETCH 0x1000 /* No side effects */ > #define IORESOURCE_MEM_64 0x0010 > > +/* > + * Non-passthrough (dom0) accesses are local PCI devices and use the given > BDF > + * Passthough (stubdom) accesses are through PV frontend PCI device. Those > + * either have a BDF identical to the backend's BFD > (xen-backend.passthrough=1) > + * or a local virtual BDF (xen-backend.passthrough=0) > + * > + * We are always given the backend's BDF and need to lookup the appropriate > + * local BDF for sysfs access. > + */ > +static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp) > +{ > +unsigned int num_devs, len, i; > +unsigned int domain, bus, dev, func; > +char *be_path; > +char path[80]; > +char *msg; > + > +be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len); > +if (!be_path) { > +/* > + * be_path doesn't exist, so we are dealing with a local > + * (non-passthough) device. > + */ > +d->local_domain = d->domain; > +d->local_bus = d->bus; > +d->local_dev = d->dev; > +d->local_func = d->func; > + > +return; > +} > + > +snprintf(path, sizeof(path), "%s/num_devs", be_path); Is 80 bytes for `path` enough? What if the path is truncated due to the limit? There's xs_node_scanf() which might be useful. It does the error handling and call scanf(). But I'm not sure if it can be used here, in this file. > +msg = qemu_xen_xs_read(xenstore, 0, path, &len); > +if (!msg) { > +goto err_out; > +} > + > +if (sscanf(msg, "%u", &num_devs) != 1) { libxl writes `num_devs` as "%d". So I think qemu should read a %d. > +error_setg(errp, "Failed to parse %s (%s)", msg, path); > + goto err_out; > +} > +free(msg); > + > +for (i = 0; i < num_devs; i++) { > +snprintf(path, sizeof(path), "%s/dev-%u", be_path, i); Same here, the path is written with a %d, even if that doesn't change the result. Thanks, -- Anthony PERARD
Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()
On Sat, Apr 01, 2023 at 10:36:45PM +, Bernhard Beschow wrote: > > > Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD > : > >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote: > >> This is a preparational patch for the next one to make the following > >> more obvious: > >> > >> First, pci_bus_irqs() is now called twice in case of Xen where the > >> second call overrides the pci_set_irq_fn with the Xen variant. > > > >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in > >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if > >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs() > >call, or maybe some other way to avoid the leak? > > Thanks for catching this! I'll post a v4. > > I think the most fool-proof way to fix this is to free irq_count just before > the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute > such that pci_bus_irqs() can be called afterwards. > > BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen > guest with my pc-piix4 branch without success. This branch essentially just > provides slightly different PCI IDs for PIIX. Does xl or something else in > Xen check these? If not then this means I'm still missing something. Under > KVM this branch works just fine. Any idea? Maybe the ACPI tables provided by libxl needs to be updated. Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the id (I know that the PCI id of the root bus is checked, but I don't know if that's the one that's been changed). Thanks, -- Anthony PERARD
Re: [PATCH v3 6/6] hw/isa/piix3: Resolve redundant TYPE_PIIX3_XEN_DEVICE
On Sun, Mar 12, 2023 at 01:02:21PM +0100, Bernhard Beschow wrote: > During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of > TYPE_PIIX3_DEVICE. Remove this redundancy. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 5/6] hw/isa/piix3: Resolve redundant k->config_write assignments
On Sun, Mar 12, 2023 at 01:02:20PM +0100, Bernhard Beschow wrote: > The previous patch unified handling of piix3_write_config() accross the > PIIX3 device models which allows for assigning k->config_write once in the > base class. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 4/6] hw/isa/piix3: Avoid Xen-specific variant of piix3_write_config()
On Sun, Mar 12, 2023 at 01:02:19PM +0100, Bernhard Beschow wrote: > Subscribe to pci_bus_fire_intx_routing_notifier() instead which allows for > having a common piix3_write_config() for the PIIX3 device models. > > While at it, move the subscription into machine code to facilitate resolving > TYPE_PIIX3_XEN_DEVICE. > > In a possible future followup, pci_bus_fire_intx_routing_notifier() could > be adjusted in such a way that subscribing to it doesn't require > knowledge of the device firing it. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 3/6] hw/isa/piix3: Wire up Xen PCI IRQ handling outside of PIIX3
On Sun, Mar 12, 2023 at 01:02:18PM +0100, Bernhard Beschow wrote: > xen_intx_set_irq() doesn't depend on PIIX3State. In order to resolve > TYPE_PIIX3_XEN_DEVICE and in order to make Xen agnostic about the > precise south bridge being used, set up Xen's PCI IRQ handling of PIIX3 > in the board. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()
On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote: > This is a preparational patch for the next one to make the following > more obvious: > > First, pci_bus_irqs() is now called twice in case of Xen where the > second call overrides the pci_set_irq_fn with the Xen variant. pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs() call, or maybe some other way to avoid the leak? > Second, pci_bus_set_route_irq_fn() is now also called in Xen mode. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Michael S. Tsirkin Beside the leak which I think can happen only once, patch is fine: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v3 1/6] include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq()
On Sun, Mar 12, 2023 at 01:02:16PM +0100, Bernhard Beschow wrote: > xen_piix3_set_irq() isn't PIIX specific: PIIX is a single PCI device > while xen_piix3_set_irq() maps multiple PCI devices to their respective > IRQs, which is board-specific. Rename xen_piix3_set_irq() to communicate > this. > > Also rename XEN_PIIX_NUM_PIRQS to XEN_IOAPIC_NUM_PIRQS since the Xen's > IOAPIC rather than PIIX has this many interrupt routes. > > Signed-off-by: Bernhard Beschow > Reviewed-by: Michael S. Tsirkin Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
[PULL 2/2] hw/xenpv: Initialize Xen backend operations
From: David Woodhouse As the Xen backend operations were abstracted out into a function table to allow for internally emulated Xen support, we missed the xen_init_pv() code path which also needs to install the operations for the true Xen libraries. Add the missing call to setup_xen_backend_ops(). Fixes: b6cacfea0b38 ("hw/xen: Add evtchn operations to allow redirection to internal emulation") Reported-by: Anthony PERARD Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Tested-by: Anthony PERARD Message-Id: <5dfb65342d4502c1ce2f890c97cff20bf25b3860.ca...@infradead.org> Signed-off-by: Anthony PERARD --- hw/xenpv/xen_machine_pv.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c index 2e759d0619..17cda5ec13 100644 --- a/hw/xenpv/xen_machine_pv.c +++ b/hw/xenpv/xen_machine_pv.c @@ -35,6 +35,8 @@ static void xen_init_pv(MachineState *machine) DriveInfo *dinfo; int i; +setup_xen_backend_ops(); + /* Initialize backend core & drivers */ xen_be_init(); -- Anthony PERARD
[PULL 1/2] accel/xen: Fix DM state change notification in dm_restrict mode
From: David Woodhouse When dm_restrict is set, QEMU isn't permitted to update the XenStore node to indicate its running status. Previously, the xs_write() call would fail but the failure was ignored. However, in refactoring to allow for emulated XenStore operations, a new call to xs_open() was added. That one didn't fail gracefully, causing a fatal error when running in dm_restrict mode. Partially revert the offending patch, removing the additional call to xs_open() because the global 'xenstore' variable is still available; it just needs to be used with qemu_xen_xs_write() now instead of directly with the xs_write() libxenstore function. Also make the whole thing conditional on !xen_domid_restrict. There's no point even registering the state change handler to attempt to update the XenStore node when we know it's destined to fail. Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to internal emulation") Reported-by: Jason Andryuk Co-developed-by: Jason Andryuk Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Tested-by: Jason Andryuk Message-Id: <1f141995bb61af32c2867ef5559e253f39b0949c.ca...@infradead.org> Signed-off-by: Anthony PERARD --- accel/xen/xen-all.c | 27 ++- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c index 00221e23c5..5ff0cb8bd9 100644 --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -32,28 +32,13 @@ xendevicemodel_handle *xen_dmod; static void xenstore_record_dm_state(const char *state) { -struct xs_handle *xs; char path[50]; -/* We now have everything we need to set the xenstore entry. */ -xs = xs_open(0); -if (xs == NULL) { -fprintf(stderr, "Could not contact XenStore\n"); -exit(1); -} - snprintf(path, sizeof (path), "device-model/%u/state", xen_domid); -/* - * This call may fail when running restricted so don't make it fatal in - * that case. Toolstacks should instead use QMP to listen for state changes. - */ -if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) && -!xen_domid_restrict) { +if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) { error_report("error recording dm state"); exit(1); } - -xs_close(xs); } @@ -111,7 +96,15 @@ static int xen_init(MachineState *ms) xc_interface_close(xen_xc); return -1; } -qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); + +/* + * The XenStore write would fail when running restricted so don't attempt + * it in that case. Toolstacks should instead use QMP to listen for state + * changes. + */ +if (!xen_domid_restrict) { +qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); + } /* * opt out of system RAM being allocated by generic code */ -- Anthony PERARD
Re: [PULL 0/1] xen queue
On Thu, Mar 23, 2023 at 10:01:59AM +, Anthony PERARD wrote: > The following changes since commit 60ca584b8af0de525656f959991a440f8c191f12: > > Merge tag 'pull-for-8.0-220323-1' of https://gitlab.com/stsquad/qemu into > staging (2023-03-22 17:58:12 +) > > are available in the Git repository at: > > https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git > tags/pull-xen-20230323 > > for you to fetch changes up to f75e4f2234e7339c16c1dba048bf131a2a948f84: > > accel/xen: Fix DM state change notification in dm_restrict mode (2023-03-23 > 09:56:54 +) > > > Xen queue > > - fix guest creation when -xen-domid-restrict is used Hi Peter, I'd like to cancel this pull request. I've got another patch to add to it. Is that fine? If I don't have any reply, I'll create a new PR later today and consider this one cancel. Cheers, -- Anthony PERARD
[PULL 0/2] xen queue
The following changes since commit 60ca584b8af0de525656f959991a440f8c191f12: Merge tag 'pull-for-8.0-220323-1' of https://gitlab.com/stsquad/qemu into staging (2023-03-22 17:58:12 +) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20230324 for you to fetch changes up to 670d8c6ebf7a2c425575bbd6fbaeb27d21edd6c6: hw/xenpv: Initialize Xen backend operations (2023-03-24 14:52:14 +) Xen queue - fix guest creation when -xen-domid-restrict is used. - fix Xen PV guest creation. David Woodhouse (2): accel/xen: Fix DM state change notification in dm_restrict mode hw/xenpv: Initialize Xen backend operations accel/xen/xen-all.c | 27 ++- hw/xenpv/xen_machine_pv.c | 2 ++ 2 files changed, 12 insertions(+), 17 deletions(-)
Re: [PATCH] hw/xenpv: Initialize Xen backend operations
On Thu, Mar 23, 2023 at 10:57:34AM +, David Woodhouse wrote: > From: David Woodhouse > > As the Xen backend operations were abstracted out into a function table to > allow for internally emulated Xen support, we missed the xen_init_pv() > code path which also needs to install the operations for the true Xen > libraries. Add the missing call to setup_xen_backend_ops(). > > Fixes: b6cacfea0b38 ("hw/xen: Add evtchn operations to allow redirection to > internal emulation") > Reported-by: Anthony PERARD > Signed-off-by: David Woodhouse Tested-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v2 09/27] hw/xen: Add evtchn operations to allow redirection to internal emulation
On Tue, Mar 07, 2023 at 05:17:32PM +, David Woodhouse wrote: > From: David Woodhouse > > The existing implementation calling into the real libxenevtchn moves to > a new file hw/xen/xen-operations.c, and is called via a function table > which in a subsequent commit will also be able to invoke the emulated > event channel support. > > Signed-off-by: David Woodhouse > Reviewed-by: Paul Durrant Hi David, Paul, This patch prevents existing use case from booting, that is even with the state change notification fix. It seems that trying to create a PV guest with libvirt fails, with "xen be core: can't connect to xenstored" in QEMU's log but it doesn't says if that's the reason qemu failed to start. But it's probably not related to libvirt. Our bisector pointed out this patch, see details and logs: https://lore.kernel.org/xen-devel/e1pdvdx-0006lh...@osstest.test-lab.xenproject.org/ https://lore.kernel.org/xen-devel/e1pcg3g-ns...@osstest.test-lab.xenproject.org/ https://lore.kernel.org/xen-devel/e1pf9hf-0005eb...@osstest.test-lab.xenproject.org/ I did run a test with patch "Fix DM state change notification in dm_restrict mode", but I think only the *dmrestict* tests have been fixed. http://logs.test-lab.xenproject.org/osstest/logs/179868/ Some failures of running PV guests without libvirt, from that flight: http://logs.test-lab.xenproject.org/osstest/logs/179868/test-amd64-amd64-xl-qcow2/info.html http://logs.test-lab.xenproject.org/osstest/logs/179868/test-amd64-i386-xl-vhd/info.html Any idea of what's wrong? Thanks, -- Anthony PERARD
[PULL 1/1] accel/xen: Fix DM state change notification in dm_restrict mode
From: David Woodhouse When dm_restrict is set, QEMU isn't permitted to update the XenStore node to indicate its running status. Previously, the xs_write() call would fail but the failure was ignored. However, in refactoring to allow for emulated XenStore operations, a new call to xs_open() was added. That one didn't fail gracefully, causing a fatal error when running in dm_restrict mode. Partially revert the offending patch, removing the additional call to xs_open() because the global 'xenstore' variable is still available; it just needs to be used with qemu_xen_xs_write() now instead of directly with the xs_write() libxenstore function. Also make the whole thing conditional on !xen_domid_restrict. There's no point even registering the state change handler to attempt to update the XenStore node when we know it's destined to fail. Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to internal emulation") Reported-by: Jason Andryuk Co-developed-by: Jason Andryuk Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant Tested-by: Jason Andryuk Message-Id: <1f141995bb61af32c2867ef5559e253f39b0949c.ca...@infradead.org> Signed-off-by: Anthony PERARD --- accel/xen/xen-all.c | 27 ++- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c index 00221e23c5..5ff0cb8bd9 100644 --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -32,28 +32,13 @@ xendevicemodel_handle *xen_dmod; static void xenstore_record_dm_state(const char *state) { -struct xs_handle *xs; char path[50]; -/* We now have everything we need to set the xenstore entry. */ -xs = xs_open(0); -if (xs == NULL) { -fprintf(stderr, "Could not contact XenStore\n"); -exit(1); -} - snprintf(path, sizeof (path), "device-model/%u/state", xen_domid); -/* - * This call may fail when running restricted so don't make it fatal in - * that case. Toolstacks should instead use QMP to listen for state changes. - */ -if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) && -!xen_domid_restrict) { +if (!qemu_xen_xs_write(xenstore, XBT_NULL, path, state, strlen(state))) { error_report("error recording dm state"); exit(1); } - -xs_close(xs); } @@ -111,7 +96,15 @@ static int xen_init(MachineState *ms) xc_interface_close(xen_xc); return -1; } -qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); + +/* + * The XenStore write would fail when running restricted so don't attempt + * it in that case. Toolstacks should instead use QMP to listen for state + * changes. + */ +if (!xen_domid_restrict) { +qemu_add_vm_change_state_handler(xen_change_state_handler, NULL); + } /* * opt out of system RAM being allocated by generic code */ -- Anthony PERARD
[PULL 0/1] xen queue
The following changes since commit 60ca584b8af0de525656f959991a440f8c191f12: Merge tag 'pull-for-8.0-220323-1' of https://gitlab.com/stsquad/qemu into staging (2023-03-22 17:58:12 +) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20230323 for you to fetch changes up to f75e4f2234e7339c16c1dba048bf131a2a948f84: accel/xen: Fix DM state change notification in dm_restrict mode (2023-03-23 09:56:54 +) Xen queue - fix guest creation when -xen-domid-restrict is used David Woodhouse (1): accel/xen: Fix DM state change notification in dm_restrict mode accel/xen/xen-all.c | 27 ++- 1 file changed, 10 insertions(+), 17 deletions(-)
[PULL 0/2] xen queue
The following changes since commit 2946e1af2704bf6584f57d4e3aec49d1d5f3ecc0: configure: Disable thread-safety warnings on macOS (2023-03-04 14:03:46 +) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20230306 for you to fetch changes up to 3856734d80fbf46683e4080117ed961f5ab1300b: hw/xen/xen_pt: fix uninitialized variable (2023-03-06 11:27:37 +) Xen queue: - fix for graphic passthrough with 'xenfv' machine - fix uninitialized variable Chuck Zmudzinski (1): xen/pt: reserve PCI slot 2 for Intel igd-passthru Marek Marczykowski-Górecki (1): hw/xen/xen_pt: fix uninitialized variable hw/i386/pc_piix.c | 1 + hw/xen/xen_pt.c | 64 + hw/xen/xen_pt.h | 20 ++ hw/xen/xen_pt_config_init.c | 2 +- hw/xen/xen_pt_stub.c| 4 +++ 5 files changed, 79 insertions(+), 12 deletions(-)
[PULL 2/2] hw/xen/xen_pt: fix uninitialized variable
From: Marek Marczykowski-Górecki xen_pt_config_reg_init() reads only that many bytes as the size of the register that is being initialized. It uses xen_host_pci_get_{byte,word,long} and casts its last argument to expected pointer type. This means for smaller registers higher bits of 'val' are not initialized. Then, the function fails if any of those higher bits are set. Fix this by initializing 'val' with zero. Signed-off-by: Marek Marczykowski-Górecki Reviewed-by: Stefano Stabellini Message-Id: <20230127050815.4155276-1-marma...@invisiblethingslab.com> Signed-off-by: Anthony PERARD --- hw/xen/xen_pt_config_init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c index cde898b744..8b9b554352 100644 --- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -1924,7 +1924,7 @@ static void xen_pt_config_reg_init(XenPCIPassthroughState *s, if (reg->init) { uint32_t host_mask, size_mask; unsigned int offset; -uint32_t val; +uint32_t val = 0; /* initialize emulate register */ rc = reg->init(s, reg_entry->reg, -- Anthony PERARD
[PULL 1/2] xen/pt: reserve PCI slot 2 for Intel igd-passthru
From: Chuck Zmudzinski Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus, as noted in docs/igd-assign.txt in the Qemu source code. Currently, when the xl toolstack is used to configure a Xen HVM guest with Intel IGD passthrough to the guest with the Qemu upstream device model, a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy a different slot. This problem often prevents the guest from booting. The only available workarounds are not good: Configure Xen HVM guests to use the old and no longer maintained Qemu traditional device model available from xenbits.xen.org which does reserve slot 2 for the Intel IGD or use the "pc" machine type instead of the "xenfv" machine type and add the xen platform device at slot 3 using a command line option instead of patching qemu to fix the "xenfv" machine type directly. The second workaround causes some degredation in startup performance such as a longer boot time and reduced resolution of the grub menu that is displayed on the monitor. This patch avoids that reduced startup performance when using the Qemu upstream device model for Xen HVM guests configured with the igd-passthru=on option. To implement this feature in the Qemu upstream device model for Xen HVM guests, introduce the following new functions, types, and macros: * XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE * XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS * typedef XenPTQdevRealize function pointer * XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2 * xen_igd_reserve_slot and xen_igd_clear_slot functions Michael Tsirkin: * Introduce XEN_PCI_IGD_DOMAIN, XEN_PCI_IGD_BUS, XEN_PCI_IGD_DEV, and XEN_PCI_IGD_FN - use them to compute the value of XEN_PCI_IGD_SLOT_MASK The new xen_igd_reserve_slot function uses the existing slot_reserved_mask member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using the xl toolstack with the gfx_passthru option enabled, which sets the igd-passthru=on option to Qemu for the Xen HVM machine type. The new xen_igd_reserve_slot function also needs to be implemented in hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough, in which case it does nothing. The new xen_igd_clear_slot function overrides qdev->realize of the parent PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was created in hw/i386/pc_piix.c for the case when igd-passthru=on. Move the call to xen_host_pci_device_get, and the associated error handling, from xen_pt_realize to the new xen_igd_clear_slot function to initialize the device class and vendor values which enables the checks for the Intel IGD to succeed. The verification that the host device is an Intel IGD to be passed through is done by checking the domain, bus, slot, and function values as well as by checking that gfx_passthru is enabled, the device class is VGA, and the device vendor in Intel. Signed-off-by: Chuck Zmudzinski Reviewed-by: Michael S. Tsirkin Reviewed-by: Stefano Stabellini Message-Id: Signed-off-by: Anthony PERARD --- hw/i386/pc_piix.c| 1 + hw/xen/xen_pt.c | 64 hw/xen/xen_pt.h | 20 ++ hw/xen/xen_pt_stub.c | 4 +++ 4 files changed, 78 insertions(+), 11 deletions(-) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 2f16011bab..4bf15f9c1f 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -422,6 +422,7 @@ static void pc_xen_hvm_init(MachineState *machine) } pc_xen_hvm_init_pci(machine); +xen_igd_reserve_slot(pcms->bus); pci_create_simple(pcms->bus, -1, "xen-platform"); } #endif diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index 8db0532632..85c93cffcf 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -57,6 +57,7 @@ #include #include "hw/pci/pci.h" +#include "hw/pci/pci_bus.h" #include "hw/qdev-properties.h" #include "hw/qdev-properties-system.h" #include "hw/xen/xen.h" @@ -780,15 +781,6 @@ static void xen_pt_realize(PCIDevice *d, Error **errp) s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function, s->dev.devfn); -xen_host_pci_device_get(&s->real_device, -s->hostaddr.domain, s->hostaddr.bus, -s->hostaddr.slot, s->hostaddr.function, -errp); -if (*errp) { -error_append_hint(errp, "Failed to \"open\" the real pci device"); -return; -} - s->is_virtfn = s->real_device.is_virtfn; if (s->is_virtfn) { XEN_PT_LOG(d, "%04x:%02x:%02x.%d is a SR-IOV Virtual Function\n&
Re: [XEN PATCH v2 0/3] Configure qemu upstream correctly by default for igd-passthru
On Tue, Jan 10, 2023 at 02:32:01AM -0500, Chuck Zmudzinski wrote: > I call attention to the commit message of the first patch which points > out that using the "pc" machine and adding the xen platform device on > the qemu upstream command line is not functionally equivalent to using > the "xenfv" machine which automatically adds the xen platform device > earlier in the guest creation process. As a result, there is a noticeable > reduction in the performance of the guest during startup with the "pc" > machne type even if the xen platform device is added via the qemu > command line options, although eventually both Linux and Windows guests > perform equally well once the guest operating system is fully loaded. There shouldn't be a difference between "xenfv" machine or using the "pc" machine while adding the "xen-platform" device, at least with regards to access to disk or network. The first patch of the series is using the "pc" machine without any "xen-platform" device, so we can't compare startup performance based on that. > Specifically, startup time is longer and neither the grub vga drivers > nor the windows vga drivers in early startup perform as well when the > xen platform device is added via the qemu command line instead of being > added immediately after the other emulated i440fx pci devices when the > "xenfv" machine type is used. The "xen-platform" device is mostly an hint to a guest that they can use pv-disk and pv-network devices. I don't think it would change anything with regards to graphics. > For example, when using the "pc" machine, which adds the xen platform > device using a command line option, the Linux guest could not display > the grub boot menu at the native resolution of the monitor, but with the > "xenfv" machine, the grub menu is displayed at the full 1920x1080 > native resolution of the monitor for testing. So improved startup > performance is an advantage for the patch for qemu. I've just found out that when doing IGD passthrough, both machine "xenfv" and "pc" are much more different than I though ... :-( pc_xen_hvm_init_pci() in QEMU changes the pci-host device, which in turns copy some informations from the real host bridge. I guess this new host bridge help when the firmware setup the graphic for grub. > I also call attention to the last point of the commit message of the > second patch and the comments for reviewers section of the second patch. > This approach, as opposed to fixing this in qemu upstream, makes > maintaining the code in libxl__build_device_model_args_new more > difficult and therefore increases the chances of problems caused by > coding errors and typos for users of libxl. So that is another advantage > of the patch for qemu. We would just needs to use a different approach in libxl when generating the command line. We could probably avoid duplications. I was hopping to have patch series for libxl that would change the machine used to start using "pc" instead of "xenfv" for all configurations, but based on the point above (IGD specific change to "xenfv"), then I guess we can't really do anything from libxl to fix IGD passthrough. > OTOH, fixing this in qemu causes newer qemu versions to behave > differently than previous versions of qemu, which the qemu community > does not like, although they seem OK with the other patch since it only > affects qemu "xenfv" machine types, but they do not want the patch to > affect toolstacks like libvirt that do not use qemu upstream's > autoconfiguration options as much as libxl does, and, of course, libvirt > can manage qemu "xenfv" machines so exising "xenfv" guests configured > manually by libvirt could be adversely affected by the patch to qemu, > but only if those same guests are also configured for igd-passthrough, > which is likely a very small number of possibly affected libvirt users > of qemu. > > A year or two ago I tried to configure guests for pci passthrough on xen > using libvirt's tool to convert a libxl xl.cfg file to libvirt xml. It > could not convert an xl.cfg file with a configuration item > pci = [ "PCI_SPEC_STRING", "PCI_SPEC_STRING", ...] for pci passthrough. > So it is unlikely there are any users out there using libvirt to > configure xen hvm guests for igd passthrough on xen, and those are the > only users that could be adversely affected by the simpler patch to qemu > to fix this. FYI, libvirt should be using libxl to create guest, I don't think there is another way for libvirt to create xen guests. So overall, unfortunately the "pc" machine in QEMU isn't suitable to do IGD passthrough as the "xenfv" machine has already some workaround to make IGD work and just need some more. I've seen that the patch for QEMU is now reviewed, so I look at having it merged soonish. Thanks, -- Anthony PERARD
Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE
On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote: > On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote: > > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally > > removes > > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in > > the PC > > machine agnostic to the precise southbridge being used. 2/ will become > > particularily interesting once PIIX4 becomes usable in the PC machine, > > avoiding > > the "Frankenstein" use of PIIX4_ACPI in PIIX3. > > Looks ok to me. > Reviewed-by: Michael S. Tsirkin > > Feel free to merge through Xen tree. Hi Bernhard, The series currently doesn't apply on master. And a quick try at applying the series it is based on also failed. Could you rebase it , or maybe you would prefer to wait until the other series "Consolidate PIIX..." is fully applied? Thanks. > > Testing done: > > None, because I don't know how to conduct this properly :( > > > > Based-on: <20221221170003.2929-1-shen...@gmail.com> > > "[PATCH v4 00/30] Consolidate PIIX south bridges" -- Anthony PERARD
Re: [PATCH v6] xen/pt: reserve PCI slot 2 for Intel igd-passthru
On Fri, Jan 06, 2023 at 09:10:55AM -0500, Chuck Zmudzinski wrote: > Well, our messages almost collided! I just proposed a v7 that adds > a check to prevent the extra processing for cases when machine is > not xenfv and the slot does not need to be cleared because it was > never reserved. The proposed v7 would not change the behavior of the > patch at all but it would avoid some unnecessary processing. Do you > want me to submit that v7? Well, preventing an simple assignment and a message from getting logged isn't going to get us far. On the other end, the message "using slot 2" when we don't even if slot 2 is actually going to be used could be confusing, so I guess preventing that message from been logged could be useful indeed. So your proposed v7 would be fine. Cheers, -- Anthony PERARD
[PATCH] configure: Expand test which disable -Wmissing-braces
From: Anthony PERARD With "clang 6.0.0-1ubuntu2" on Ubuntu Bionic, the test with build fine, but clang still suggest braces around the zero initializer in a few places, where there is a subobject. Expand test to include a sub struct which doesn't build on clang 6.0.0-1ubuntu2, and give: config-temp/qemu-conf.c:7:8: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] } x = {0}; ^ {} These are the error reported by clang on QEMU's code (v7.2.0): hw/pci-bridge/cxl_downstream.c:101:51: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] dvsec = (uint8_t *)&(CXLDVSECPortExtensions){ 0 }; hw/pci-bridge/cxl_root_port.c:62:51: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] dvsec = (uint8_t *)&(CXLDVSECPortExtensions){ 0 }; tests/qtest/virtio-net-test.c:322:34: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] QOSGraphTestOptions opts = { 0 }; Reported-by: Andrew Cooper Signed-off-by: Anthony PERARD --- While Ubuntu Bionic isn't supposed to be supported anymore, clang v6 is still the minimum required as tested by ./configure. --- configure | 4 1 file changed, 4 insertions(+) diff --git a/configure b/configure index 9f0bc57546..3cd9b8bad4 100755 --- a/configure +++ b/configure @@ -1290,7 +1290,11 @@ fi # Disable -Wmissing-braces on older compilers that warn even for # the "universal" C zero initializer {0}. cat > $TMPC << EOF +struct s { + void *a; +}; struct { + struct s s; int a[2]; } x = {0}; EOF -- Anthony PERARD
Re: [PATCH v6] xen/pt: reserve PCI slot 2 for Intel igd-passthru
On Sun, Jan 01, 2023 at 06:52:03PM -0500, Chuck Zmudzinski wrote: > Intel specifies that the Intel IGD must occupy slot 2 on the PCI bus, > as noted in docs/igd-assign.txt in the Qemu source code. > > Currently, when the xl toolstack is used to configure a Xen HVM guest with > Intel IGD passthrough to the guest with the Qemu upstream device model, > a Qemu emulated PCI device will occupy slot 2 and the Intel IGD will occupy > a different slot. This problem often prevents the guest from booting. > > The only available workaround is not good: Configure Xen HVM guests to use > the old and no longer maintained Qemu traditional device model available > from xenbits.xen.org which does reserve slot 2 for the Intel IGD. > > To implement this feature in the Qemu upstream device model for Xen HVM > guests, introduce the following new functions, types, and macros: > > * XEN_PT_DEVICE_CLASS declaration, based on the existing TYPE_XEN_PT_DEVICE > * XEN_PT_DEVICE_GET_CLASS macro helper function for XEN_PT_DEVICE_CLASS > * typedef XenPTQdevRealize function pointer > * XEN_PCI_IGD_SLOT_MASK, the value of slot_reserved_mask to reserve slot 2 > * xen_igd_reserve_slot and xen_igd_clear_slot functions > > The new xen_igd_reserve_slot function uses the existing slot_reserved_mask > member of PCIBus to reserve PCI slot 2 for Xen HVM guests configured using > the xl toolstack with the gfx_passthru option enabled, which sets the > igd-passthru=on option to Qemu for the Xen HVM machine type. > > The new xen_igd_reserve_slot function also needs to be implemented in > hw/xen/xen_pt_stub.c to prevent FTBFS during the link stage for the case > when Qemu is configured with --enable-xen and --disable-xen-pci-passthrough, > in which case it does nothing. > > The new xen_igd_clear_slot function overrides qdev->realize of the parent > PCI device class to enable the Intel IGD to occupy slot 2 on the PCI bus > since slot 2 was reserved by xen_igd_reserve_slot when the PCI bus was > created in hw/i386/pc_piix.c for the case when igd-passthru=on. > > Move the call to xen_host_pci_device_get, and the associated error > handling, from xen_pt_realize to the new xen_igd_clear_slot function to > initialize the device class and vendor values which enables the checks for > the Intel IGD to succeed. The verification that the host device is an > Intel IGD to be passed through is done by checking the domain, bus, slot, > and function values as well as by checking that gfx_passthru is enabled, > the device class is VGA, and the device vendor in Intel. > > Signed-off-by: Chuck Zmudzinski This patch looks good enough. It only changes the "xenfv" machine so it doesn't prevent a proper fix to be done in the toolstack libxl. The change in xen_pci_passthrough_class_init() to try to run some code before pci_qdev_realize() could potentially break in the future due to been uncommon but hopefully that will be ok. So if no work to fix libxl appear soon, I'm ok with this patch: Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH v6] xen/pt: reserve PCI slot 2 for Intel igd-passthru
On Tue, Jan 03, 2023 at 05:58:01PM -0500, Chuck Zmudzinski wrote: > Hello Anthony and Paul, Hi Chuck, > I am requesting your feedback to Alex Williamson's suggestion that this > problem with assigning the correct slot address to the igd on xen should > be fixed in libxl instead of in qemu. > > It seems to me that the xen folks and the kvm folks have two different > philosophies regarding how a tool stack should be designed. kvm/libvirt > provides much greater flexibility in configuring the guest which puts > the burden on the administrator to set all the options correctly for > a given feature set, while xen/xenlight does not provide so much > flexibility and tries to automatically configure the guest based on > a high-level feature option such as the igd-passthrough=on option that > is available for xen guests using qemu but not for kvm guests using > qemu. > > What do you think? Should libxl be patched instead of fixing the problem > with this patch to qemu, which is contrary to Alex's suggestion? I do think that libxl should be able to deal with having to put a graphic card on slot 2. QEMU already provides every API necessary for a toolstack to be able to start a Xen guest with all the PCI card in the right slot. But it would just be a bit more complicated to implement in libxl. At the moment, libxl makes use of the QEMU machine 'xenfv', libxl should instead start to use the 'pc' machine and add the "xen-platform" pci device. (libxl already uses 'pc' when the "xen-platform" pci card isn't needed.) Also probably add the other pci devices to specific slot to be able to add the passthrough graphic card at the right slot. Next is to deal with migration when using the 'pc' machine, as it's just an alias to a specific version of the machine. We need to use the same machine on the receiving end, that is start with e.g. "pc-i440fx-7.1" if 'pc' was an alias for it at guest creation. I wonder if we can already avoid to patch the 'xenfv' machine with some xl config: # avoid 'xenfv' machine and use 'pc' instead xen_platform_pci=0 # add xen-platform pci device back device_model_args_hvm = [ "-device", "xen-platform,addr=3", ] But there's probably another device which is going to be auto-assigned to slot 2. If you feel like dealing with the technical dept in libxl, that is to stop using 'xenfv' and use 'pc' instead, then go for it, I can help with that. Otherwise, if the patch to QEMU only changes the behavior of the 'xenfv' machine then I think I would be ok with it. I'll do a review of that QEMU patch in another email. Cheers, -- Anthony PERARD
Re: [PATCH 2/6] hw/xen: use G_GNUC_PRINTF/SCANF for various functions
On Mon, Dec 19, 2022 at 08:02:01AM -0500, Daniel P. Berrangé wrote: > Signed-off-by: Daniel P. Berrangé Acked-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH 1/2] hw/xen/xen_pt: Call default handler only if no custom one is set
On Mon, Nov 14, 2022 at 08:20:10PM +0100, Marek Marczykowski-Górecki wrote: > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index 0ec7e52183..269bd26109 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -255,6 +255,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, > uint32_t addr, > uint32_t find_addr = addr; > XenPTRegInfo *reg = NULL; > bool wp_flag = false; > +uint32_t emul_mask = 0, write_val; > > if (xen_pt_pci_config_access_check(d, addr, len)) { > return; > @@ -310,7 +311,6 @@ static void xen_pt_pci_write_config(PCIDevice *d, > uint32_t addr, > } > > memory_region_transaction_begin(); > -pci_default_write_config(d, addr, val, len); > > /* adjust the read and write value to appropriate CFC-CFF window */ > read_val <<= (addr & 3) << 3; > @@ -370,6 +370,8 @@ static void xen_pt_pci_write_config(PCIDevice *d, > uint32_t addr, > return; > } > > +emul_mask |= ( (1 << (reg->size * 8) ) - 1 ) << ((find_addr & 3) > * 8); > + > /* calculate next address to find */ > emul_len -= reg->size; > if (emul_len > 0) { > @@ -396,6 +398,24 @@ static void xen_pt_pci_write_config(PCIDevice *d, > uint32_t addr, > /* need to shift back before passing them to xen_host_pci_set_block. */ > val >>= (addr & 3) << 3; > > +/* store emulated registers that didn't have specific hooks */ > +write_val = val; > +for (index = 0; emul_mask; index += emul_len) { `index` isn't used, was it meant to be use for something? > +emul_len = 0; > +while (emul_mask & 0xff) { > +emul_len++; This seems to count the number of byte that have a hook (xen_pt_find_reg() found a `reg_entry`). This loop should count instead the number of bytes for which no `reg_entry` have been found, right? Shouldn't the loop count when a byte in emul_mask is unset? > +emul_mask >>= 8; > +} > +if (emul_len) { > +uint32_t mask = ((1 << (emul_len * 8)) - 1); > +pci_default_write_config(d, addr, write_val & mask, emul_len); `addr` isn't updated in the loop, aren't we going to write bytes to the wrong place? If for example "emul_mask == 0x00ff00ff" ? > +write_val >>= emul_len * 8; > +} else { > +emul_mask >>= 8; > +write_val >>= 8; > +} > +} Thanks, -- Anthony PERARD
[PULL 1/1] hw/xen: set pci Atomic Ops requests for passthrough device
From: Ruili Ji Make guest os access pci device control 2 reg for passthrough device as struct XenPTRegInfo described in the file hw/xen/xen_pt.h. /* reg read only field mask (ON:RO/ROS, OFF:other) */ uint32_t ro_mask; /* reg emulate field mask (ON:emu, OFF:passthrough) */ uint32_t emu_mask; Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1196 Signed-off-by: Aaron Liu Signed-off-by: Ruili Ji Message-ID: Reviewed-by: Paul Durrant Signed-off-by: Anthony PERARD --- hw/xen/xen_pt_config_init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c index 4758514ddf..cde898b744 100644 --- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -985,8 +985,8 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[] = { .offset = 0x28, .size = 2, .init_val = 0x, -.ro_mask= 0xFFE0, -.emu_mask = 0x, +.ro_mask= 0xFFA0, +.emu_mask = 0xFFBF, .init = xen_pt_devctrl2_reg_init, .u.w.read = xen_pt_word_reg_read, .u.w.write = xen_pt_word_reg_write, -- Anthony PERARD
[PULL 0/1] xen queue 2022-09-27
The following changes since commit 99d6b11b5b44d7dd64f4cb1973184e40a4a174f8: Merge tag 'pull-target-arm-20220922' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2022-09-26 13:38:26 -0400) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20220927 for you to fetch changes up to fb36fb9344c284a58f3f3ec5be6408fc51eaf3f1: hw/xen: set pci Atomic Ops requests for passthrough device (2022-09-27 14:23:23 +0100) Xen patch - Xen PCI passthrough fix for Atomic Ops requests Ruili Ji (1): hw/xen: set pci Atomic Ops requests for passthrough device hw/xen/xen_pt_config_init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Re: [PATCH] hw/xen: set pci Atomic Ops requests for passthrough device
On Tue, Sep 06, 2022 at 07:40:25AM +, Ji, Ruili wrote: > From: Ruili Ji ruili...@amd.com<mailto:ruili...@amd.com> > Date: Tue, 6 Sep 2022 14:09:41 +0800 > Subject: [PATCH] hw/xen: set pci Atomic Ops requests for passthrough device > > Make guest os access pci device control 2 reg for passthrough device > as struct XenPTRegInfo described in the file hw/xen/xen_pt.h. > /* reg read only field mask (ON:RO/ROS, OFF:other) */ > uint32_t ro_mask; > /* reg emulate field mask (ON:emu, OFF:passthrough) */ > uint32_t emu_mask; > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1196 > Signed-off-by: aaron@amd.com<mailto:aaron@amd.com> > Signed-off-by: ruili...@amd.com<mailto:ruili...@amd.com> Hi Ruili, I believe both signed-off-by line aren't formatted as expected (I think a name is needed with the email address), may I change them to: Signed-off-by: Aaron Lui Signed-off-by: Ruili Ji The second line would be the same as the author of the patch (From:). Thanks, -- Anthony PERARD