[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", _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, );
+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, +_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 >F when available
On Mon, Jan 29, 2024 at 05:44:43PM +0100, Philippe Mathieu-Daudé wrote:
> When a variable is initialized to >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 = >m;
> <+...
> ->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(>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(>io_listener);
> s->listener_set = false;
> }
> -if (!xen_host_pci_device_closed(>real_device)) {
> -xen_host_pci_device_put(>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, > > +_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 _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 _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 _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 _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, , XBT_NULL, tx->dom_id, "/", );
+ret = init_walk_op(s, , XBT_NULL, tx->dom_id, "/", );
+/*
+ * 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, );
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(>worker_stopped);
-qemu_mutex_unlock(>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(>request_cond);
+qemu_mutex_unlock(>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, _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, _memory_listener);
QLIST_INIT(_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(>memory_listener, _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(>worker_stopped);
-qemu_mutex_unlock(>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(>request_cond);
+qemu_mutex_unlock(>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, );
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(_pci_type_info);
type_register_static(_info);
-type_register_static(_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= _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(>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), >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), >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 = _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(_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], level);
if (level) {
/* Ensure the vCPU polls for
[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_set_ioreq_server_state
[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, _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", );
> +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, );
> +if (!msg) {
> +goto err_out;
> +}
> +
> +if (sscanf(msg, "%u", _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(>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
[PULL 2/2] xen/pass-through: don't create needless register group
From: Chuck Zmudzinski
Currently we are creating a register group for the Intel IGD OpRegion
for every device we pass through, but the XEN_PCI_INTEL_OPREGION
register group is only valid for an Intel IGD. Add a check to make
sure the device is an Intel IGD and a check that the administrator has
enabled gfx_passthru in the xl domain configuration. Require both checks
to be true before creating the register group. Use the existing
is_igd_vga_passthrough() function to check for a graphics device from
any vendor and that the administrator enabled gfx_passthru in the xl
domain configuration, but further require that the vendor be Intel,
because only Intel IGD devices have an Intel OpRegion. These are the
same checks hvmloader and libxl do to determine if the Intel OpRegion
needs to be mapped into the guest's memory. Also, move the comment
about trapping 0xfc for the Intel OpRegion where it belongs after
applying this patch.
Signed-off-by: Chuck Zmudzinski
Reviewed-by: Anthony PERARD
Message-Id:
Signed-off-by: Anthony PERARD
---
hw/xen/xen_pt_config_init.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index bff0962795..4758514ddf 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -2032,12 +2032,16 @@ void xen_pt_config_init(XenPCIPassthroughState *s,
Error **errp)
}
}
-/*
- * By default we will trap up to 0x40 in the cfg space.
- * If an intel device is pass through we need to trap 0xfc,
- * therefore the size should be 0xff.
- */
if (xen_pt_emu_reg_grps[i].grp_id == XEN_PCI_INTEL_OPREGION) {
+if (!is_igd_vga_passthrough(>real_device) ||
+s->real_device.vendor_id != PCI_VENDOR_ID_INTEL) {
+continue;
+}
+/*
+ * By default we will trap up to 0x40 in the cfg space.
+ * If an intel device is pass through we need to trap 0xfc,
+ * therefore the size should be 0xff.
+ */
reg_grp_offset = XEN_PCI_INTEL_OPREGION;
}
--
Anthony PERARD
[PULL 1/2] xen/pass-through: merge emulated bits correctly
From: Chuck Zmudzinski
In xen_pt_config_reg_init(), there is an error in the merging of the
emulated data with the host value. With the current Qemu, instead of
merging the emulated bits with the host bits as defined by emu_mask,
the emulated bits are merged with the host bits as defined by the
inverse of emu_mask. In some cases, depending on the data in the
registers on the host, the way the registers are setup, and the
initial values of the emulated bits, the end result will be that
the register is initialized with the wrong value.
To correct this error, use the XEN_PT_MERGE_VALUE macro to help ensure
the merge is done correctly.
This correction is needed to resolve Qemu project issue #1061, which
describes the failure of Xen HVM Linux guests to boot in certain
configurations with passed through PCI devices, that is, when this error
disables instead of enables the PCI_STATUS_CAP_LIST bit of the
PCI_STATUS register of a passed through PCI device, which in turn
disables the MSI-X capability of the device in Linux guests with the end
result being that the Linux guest never completes the boot process.
Fixes: 2e87512eccf3 ("xen/pt: Sync up the dev.config and data values")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1061
Buglink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=988333
Signed-off-by: Chuck Zmudzinski
Reviewed-by: Anthony PERARD
Message-Id:
Signed-off-by: Anthony PERARD
---
hw/xen/xen_pt_config_init.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index c5c4e943a8..bff0962795 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1965,11 +1965,12 @@ static void
xen_pt_config_reg_init(XenPCIPassthroughState *s,
if ((data & host_mask) != (val & host_mask)) {
uint32_t new_val;
-
-/* Mask out host (including past size). */
-new_val = val & host_mask;
-/* Merge emulated ones (excluding the non-emulated ones). */
-new_val |= data & host_mask;
+/*
+ * Merge the emulated bits (data) with the host bits (val)
+ * and mask out the bits past size to enable restoration
+ * of the proper value for logging below.
+ */
+new_val = XEN_PT_MERGE_VALUE(val, data, host_mask) & size_mask;
/* Leave intact host and emulated values past the size - even
though
* we do not care as we write per reg->size granularity, but for
the
* logging below lets have the proper value. */
--
Anthony PERARD
[PULL 0/2] xen queue
The following changes since commit 19361471b59441cd6f2aa22d4fbee7a6e9e76586: Merge tag 'pull-la-20220705' of https://gitlab.com/rth7680/qemu into staging (2022-07-05 16:30:52 +0530) are available in the Git repository at: https://xenbits.xen.org/git-http/people/aperard/qemu-dm.git tags/pull-xen-20220705 for you to fetch changes up to c0e86b7624cb9d6db03e0d48cf82659e5b89a6a6: xen/pass-through: don't create needless register group (2022-07-05 14:19:48 +0100) Xen patches - Xen PCI passthrough fixes Chuck Zmudzinski (2): xen/pass-through: merge emulated bits correctly xen/pass-through: don't create needless register group hw/xen/xen_pt_config_init.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-)
Re: [PATCH v2] xen/pass-through: don't create needless register group
On Fri, Jun 17, 2022 at 03:13:33PM -0400, Chuck Zmudzinski wrote: > Currently we are creating a register group for the Intel IGD OpRegion > for every device we pass through, but the XEN_PCI_INTEL_OPREGION > register group is only valid for an Intel IGD. Add a check to make > sure the device is an Intel IGD and a check that the administrator has > enabled gfx_passthru in the xl domain configuration. Require both checks > to be true before creating the register group. Use the existing > is_igd_vga_passthrough() function to check for a graphics device from > any vendor and that the administrator enabled gfx_passthru in the xl > domain configuration, but further require that the vendor be Intel, > because only Intel IGD devices have an Intel OpRegion. These are the > same checks hvmloader and libxl do to determine if the Intel OpRegion > needs to be mapped into the guest's memory. Also, move the comment > about trapping 0xfc for the Intel OpRegion where it belongs after > applying this patch. > > Signed-off-by: Chuck Zmudzinski Reviewed-by: Anthony PERARD Thanks, -- Anthony PERARD
Re: [PATCH] xen/pass-through: don't create needless register group
On Fri, Jun 10, 2022 at 12:23:35PM -0400, Chuck Zmudzinski wrote:
> Currently we are creating a register group for the Intel IGD OpRegion
> for every device we pass through, but the XEN_PCI_INTEL_OPREGION
> register group is only valid for an Intel IGD. Add a check to make
> sure the device is an Intel IGD and a check that the administrator has
> enabled gfx_passthru in the xl domain configuration. Require both checks
> to be true before creating the register group. Use the existing
> is_igd_vga_passthrough() function to check for a graphics device from
> any vendor and that the administrator enabled gfx_passthru in the xl
> domain configuration, but further require that the vendor be Intel,
> because only Intel IGD devices have an Intel OpRegion. These are the
> same checks hvmloader and libxl do to determine if the Intel OpRegion
> needs to be mapped into the guest's memory.
>
> Signed-off-by: Chuck Zmudzinski
> ---
> hw/xen/xen_pt_config_init.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index c5c4e943a8..ffd915654c 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -2037,6 +2037,10 @@ void xen_pt_config_init(XenPCIPassthroughState *s,
> Error **errp)
> * therefore the size should be 0xff.
> */
Could you move that comment? I think it would make more sense to comment
the "reg_grp_offset=XEN_PCI_INTEL_OPREGION" line now that the `if` block
also skip setting up the group on non-intel devices.
> if (xen_pt_emu_reg_grps[i].grp_id == XEN_PCI_INTEL_OPREGION) {
> +if (!is_igd_vga_passthrough(>real_device) ||
> +s->real_device.vendor_id != PCI_VENDOR_ID_INTEL) {
> +continue;
> + }
> reg_grp_offset = XEN_PCI_INTEL_OPREGION;
> }
Thanks,
--
Anthony PERARD
Re: [PATCH v2] xen/pass-through: merge emulated bits correctly
On Sat, Jun 11, 2022 at 12:43:29PM -0400, Chuck Zmudzinski wrote: > In xen_pt_config_reg_init(), there is an error in the merging of the > emulated data with the host value. With the current Qemu, instead of > merging the emulated bits with the host bits as defined by emu_mask, > the emulated bits are merged with the host bits as defined by the > inverse of emu_mask. In some cases, depending on the data in the > registers on the host, the way the registers are setup, and the > initial values of the emulated bits, the end result will be that > the register is initialized with the wrong value. > > To correct this error, use the XEN_PT_MERGE_VALUE macro to help ensure > the merge is done correctly. > > This correction is needed to resolve Qemu project issue #1061, which > describes the failure of Xen HVM Linux guests to boot in certain > configurations with passed through PCI devices, that is, when this error > disables instead of enables the PCI_STATUS_CAP_LIST bit of the > PCI_STATUS register of a passed through PCI device, which in turn > disables the MSI-X capability of the device in Linux guests with the end > result being that the Linux guest never completes the boot process. > > Fixes: 2e87512eccf3 > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1061 > Buglink: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=988333 > > Signed-off-by: Chuck Zmudzinski Reviewed-by: Anthony PERARD Thank you, looks like it's been a long quest to figure this one out. -- Anthony PERARD
[PULL 1/3] hw/ide/piix: Remove redundant "piix3-ide-xen" device class
From: Bernhard Beschow
Commit 0f8445820f11a69154309863960328dda3dc1ad4 'xen: piix reuse pci
generic class init function' already resolved redundant code which in
turn rendered piix3-ide-xen redundant.
Signed-off-by: Bernhard Beschow
Reviewed-by: Anthony PERARD
Message-Id: <20220513180957.90514-2-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/i386/pc_piix.c | 3 +--
hw/ide/piix.c | 7 ---
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 578e537b35..0e45521e74 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -246,8 +246,7 @@ static void pc_init1(MachineState *machine,
if (pcmc->pci_enabled) {
PCIDevice *dev;
-dev = pci_create_simple(pci_bus, piix3_devfn + 1,
-xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+dev = pci_create_simple(pci_bus, piix3_devfn + 1, "piix3-ide");
pci_ide_create_devs(dev);
idebus[0] = qdev_get_child_bus(>qdev, "ide.0");
idebus[1] = qdev_get_child_bus(>qdev, "ide.1");
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..2345fe9e1d 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -241,12 +241,6 @@ static const TypeInfo piix3_ide_info = {
.class_init= piix3_ide_class_init,
};
-static const TypeInfo piix3_ide_xen_info = {
-.name = "piix3-ide-xen",
-.parent= TYPE_PCI_IDE,
-.class_init= piix3_ide_class_init,
-};
-
/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
static void piix4_ide_class_init(ObjectClass *klass, void *data)
{
@@ -272,7 +266,6 @@ static const TypeInfo piix4_ide_info = {
static void piix_ide_register_types(void)
{
type_register_static(_ide_info);
-type_register_static(_ide_xen_info);
type_register_static(_ide_info);
}
--
Anthony PERARD
[PULL 2/3] hw/ide/piix: Add some documentation to pci_piix3_xen_ide_unplug()
From: Bernhard Beschow
The comment is based on commit message
ae4d2eb273b167dad748ea4249720319240b1ac2 'xen-platform: add missing disk
unplug option'. Since it seems to describe design decisions and
limitations that still apply it seems worth having.
Signed-off-by: Bernhard Beschow
Reviewed-by: Anthony PERARD
Message-Id: <20220513180957.90514-3-shen...@gmail.com>
Signed-off-by: Anthony PERARD
---
hw/ide/piix.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 2345fe9e1d..bc1b37512a 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -173,6 +173,17 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
**errp)
}
}
+/*
+ * The Xen HVM unplug protocol [1] specifies a mechanism to allow guests to
+ * request unplug of 'aux' disks (which is stated to mean all IDE disks,
+ * except the primary master).
+ *
+ * NOTE: The semantics of what happens if unplug of all disks and 'aux' disks
+ * is simultaneously requested is not clear. The implementation assumes
+ * that an 'all' request overrides an 'aux' request.
+ *
+ * [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
+ */
int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
{
PCIIDEState *pci_ide;
--
Anthony PERARD
