[PATCH v4 00/17] Maintainer updates for May (testing, plugins, virtio-gpu) - pre-PR

[PATCH v4 00/17] Maintainer updates for May (testing, plugins, virtio-gpu) - pre-PR

[Alex Bennée]

Now the tree is open here is the state of my current maintainer
queues. I've tagged the virtio-gpu changes as Cc: qemu-stable but
given how close to release we were it didn't seem worth rushing them
in for 10.0. I've included a MAINTAINERS patch just to make sure I can
route any more fixes until a more full time maintainer volunteers.

As there is still controversy about a theoretical UAF in virtio-gpu
I've dropped that patch for now. There were a number of other review
comments addressed so I'm re-sending the pre-PR so I can roll the PR
later this week.

For v4
  - more review comments
  - made boot.S error handling more robust
  - dropped virtio-gpu hang fix for TCG
  - dropped test-plugins fix for Makefile

For v3
  - addressed review comments
  - included gdb/next patches

For v2
  - addressed various comments (see bellow --- in commits)
  - added Akihiko/Dmitry as virtio-gpu reviewers

The following still need review:

  tests/tcg: make aarch64 boot.S handle different starting modes

Alex.

Alex Bennée (11):
  tests/docker: expose $HOME/.cache/qemu as docker volume
  gitlab: disable debug info on CI builds
  tests/tcg: make aarch64 boot.S handle different starting modes
  contrib/plugins: add a scaling factor to the ips arg
  contrib/plugins: allow setting of instructions per quantum
  MAINTAINERS: add myself to virtio-gpu for Odd Fixes
  MAINTAINERS: add Akihiko and Dmitry as reviewers
  hw/display: re-arrange memory region tracking
  include/exec: fix assert in size_memop
  include/gdbstub: fix include guard in commands.h
  gdbstub: assert earlier in handle_read_all_regs

Dominik 'Disconnect3d' Czarnota (1):
  gdbstub: Implement qGDBServerVersion packet

Dongwon Kim (1):
  ui/gtk-gl-area: Remove extra draw call in refresh

Manos Pitsidianakis (2):
  virtio-gpu: refactor async blob unmapping
  gdbstub: update aarch64-core.xml

Nabih Estefan (1):
  tests/qtest: Avoid unaligned access in IGB test

Yiwei Zhang (1):
  virtio-gpu: support context init multiple timeline

 MAINTAINERS                               |   5 +-
 docs/about/emulation.rst                  |   4 +
 include/exec/memop.h                      |   4 +-
 include/gdbstub/commands.h                |   2 +-
 include/system/memory.h                   |   1 +
 contrib/plugins/ips.c                     |  49 +++++-
 gdbstub/gdbstub.c                         |  19 ++-
 hw/display/virtio-gpu-virgl.c             | 102 +++++++++----
 tests/qtest/libqos/igb.c                  |   4 +-
 ui/gtk-gl-area.c                          |   1 -
 .gitlab-ci.d/buildtest-template.yml       |   1 +
 gdb-xml/aarch64-core.xml                  |  52 ++++++-
 tests/docker/Makefile.include             |  10 +-
 tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
 tests/tcg/aarch64/system/boot.S           | 172 +++++++++++++++++++++-
 15 files changed, 382 insertions(+), 47 deletions(-)

-- 
2.47.2

[PATCH v4 01/17] tests/docker: expose $HOME/.cache/qemu as docker volume

[Alex Bennée]

If you want to run functional tests we should share .cache/qemu so we
don't force containers to continually re-download images. We also move
ccache to use this shared area.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>

---
v3
  - mention ccache
v2
  - Share a whole .cache/qemu path.
---
 tests/docker/Makefile.include | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index fa1cbb6726..3959d8a028 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -185,8 +185,10 @@ docker:
 
 docker-help: docker
 
+# Where QEMU caches build artefacts
+DOCKER_QEMU_CACHE_DIR := $$HOME/.cache/qemu
 # Use a global constant ccache directory to speed up repetitive builds
-DOCKER_CCACHE_DIR := $$HOME/.cache/qemu-docker-ccache
+DOCKER_QEMU_CCACHE_DIR := DOCKER_QEMU_CACHE_DIR/docker-ccache
 
 # This rule if for directly running against an arbitrary docker target.
 # It is called by the expanded docker targets (e.g. make
@@ -195,7 +197,7 @@ DOCKER_CCACHE_DIR := $$HOME/.cache/qemu-docker-ccache
 # For example: make docker-run TEST="test-quick" IMAGE="debian:arm64" EXECUTABLE=./aarch64-linux-user/qemu-aarch64
 #
 docker-run: docker-qemu-src
-	@mkdir -p "$(DOCKER_CCACHE_DIR)"
+	@mkdir -p "$(DOCKER_QEMU_CCACHE_DIR)"
 	@if test -z "$(IMAGE)" || test -z "$(TEST)"; \
 		then echo "Invalid target $(IMAGE)/$(TEST)"; exit 1; \
 	fi
@@ -222,8 +224,8 @@ docker-run: docker-qemu-src
 			-e V=$V -e J=$J -e DEBUG=$(DEBUG)		\
 			-e SHOW_ENV=$(SHOW_ENV) 			\
 			$(if $(NOUSER),,				\
-				-e CCACHE_DIR=/var/tmp/ccache 		\
-				-v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \
+				-v $(DOCKER_QEMU_CACHE_DIR):$(DOCKER_QEMU_CACHE_DIR) 	\
+				-e CCACHE_DIR=$(DOCKER_QEMU_CCACHE_DIR)	\
 			)						\
 			-v $$(readlink -e $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \
 			$(IMAGE) 					\
-- 
2.47.2

[PATCH v4 02/17] gitlab: disable debug info on CI builds

[Alex Bennée]

Our default build enables debug info which adds hugely to the size of
the builds as well as the size of cached objects. Disable debug info
across the board to save space and reduce pressure on the CI system.
We still have a number of builds which explicitly enable debug and
related extra asserts like --enable-debug-tcg.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 .gitlab-ci.d/buildtest-template.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml
index 118371e377..19663126ca 100644
--- a/.gitlab-ci.d/buildtest-template.yml
+++ b/.gitlab-ci.d/buildtest-template.yml
@@ -24,6 +24,7 @@
     - ccache --zero-stats
     - section_start configure "Running configure"
     - ../configure --enable-werror --disable-docs --enable-fdt=system
+          --disable-debug-info
           ${TARGETS:+--target-list="$TARGETS"}
           $CONFIGURE_ARGS ||
       { cat config.log meson-logs/meson-log.txt && exit 1; }
-- 
2.47.2

[PATCH v4 03/17] tests/tcg: make aarch64 boot.S handle different starting modes

[Alex Bennée]

Currently the boot.S code assumes everything starts at EL1. This will
break things like the memory test which will barf on unaligned memory
access when run at a higher level.

Adapt the boot code to do some basic verification of the starting mode
and the minimal configuration to move to the lower exception levels.
With this we can run the memory test with:

  -M virt,secure=on
  -M virt,secure=on,virtualization=on
  -M virt,virtualisation=on

If a test needs to be at a particular EL it can use the semihosting
command line to indicate the level we should execute in.

Cc: Julian Armistead <julian.armistead@linaro.org>
Cc: Jim MacArthur <jim.macarthur@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v4
  - drop post eret nops
  - proper error string for EL0 error case
  - clamp any invalid target EL value to 1
v3
  - create system stack so we _exit cleanly
  - normalise EL string before compares
  - catch when we start in a lower EL than we asked for
  - default to EL1 when arg unclear
v2
  - allow tests to control the final EL we end up at
  - use tabs consistently
  - validate command line arg is between 1 and 3
---
 tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
 tests/tcg/aarch64/system/boot.S           | 172 +++++++++++++++++++++-
 2 files changed, 169 insertions(+), 6 deletions(-)

diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index 9c52475b7a..f7a7d2b800 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -68,7 +68,8 @@ run-plugin-semiconsole-with-%: semiconsole
 
 # vtimer test needs EL2
 QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
-run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_BASE_ARGS) -kernel
+QEMU_EL2_BASE_ARGS=-semihosting-config enable=on,target=native,chardev=output,arg="2"
+run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_EL2_BASE_ARGS) -kernel
 
 # Simple Record/Replay Test
 .PHONY: memory-record
diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
index a5df9c173d..8bfa4e4efc 100644
--- a/tests/tcg/aarch64/system/boot.S
+++ b/tests/tcg/aarch64/system/boot.S
@@ -16,6 +16,7 @@
 #define semihosting_call hlt 0xf000
 #define SYS_WRITEC	0x03	/* character to debug channel */
 #define SYS_WRITE0	0x04	/* string to debug channel */
+#define SYS_GET_CMDLINE 0x15	/* get command line */
 #define SYS_EXIT	0x18
 
 	.align	12
@@ -70,21 +71,172 @@ lower_a32_sync:
 lower_a32_irq:
 lower_a32_fiq:
 lower_a32_serror:
+	adr	x1, .unexp_excp
+exit_msg:
 	mov	x0, SYS_WRITE0
-	adr	x1, .error
 	semihosting_call
 	mov	x0, 1 /* EXIT_FAILURE */
 	bl 	_exit
 	/* never returns */
 
 	.section .rodata
-.error:
-	.string "Terminated by exception.\n"
+.unexp_excp:
+	.string "Unexpected exception.\n"
+.high_el_msg:
+	.string "Started in lower EL than requested.\n"
+.unexp_el0:
+	.string "Started in invalid EL.\n"
+
+	.align 8
+.get_cmd:
+	.quad	cmdline
+	.quad	128
 
 	.text
 	.align 4
 	.global __start
 __start:
+	/*
+         * Initialise the stack for whatever EL we are in before
+	 * anything else, we need it to be able to _exit cleanly.
+	 * It's smaller than the stack we pass to the C code but we
+	 * don't need much.
+	 */
+	adrp	x0, system_stack_end
+	add	x0, x0, :lo12:system_stack_end
+	mov	sp, x0
+
+	/*
+	 * The test can set the semihosting command line to the target
+	 * EL needed for the test. However if no semihosting args are set we will
+	 * end up with -kernel/-append data (see semihosting_arg_fallback).
+	 * Keep the normalised target in w11.
+	 */
+	mov	x0, SYS_GET_CMDLINE
+	adr	x1, .get_cmd
+	semihosting_call
+	adrp	x10, cmdline
+	add	x10, x10, :lo12:cmdline
+	ldrb	w11, [x10]
+
+	/* sanity check, normalise char to EL, clamp to 1 if outside range */
+	subs w11, w11, #'0'
+	b.lt el_default
+	cmp  w11, #3
+	b.gt el_default
+	b 1f
+
+el_high:
+	adr	x1, .high_el_msg
+	b	exit_msg
+
+el_default:
+	mov	w11, #1
+
+1:
+	/* Determine current Exception Level */
+	mrs	x0, CurrentEL
+	lsr	x0, x0, #2	  /* CurrentEL[3:2] contains the current EL */
+
+	/* Are we already in a lower EL than we want? */
+	cmp	w11, w0
+	bgt	el_high
+
+	/* Branch based on current EL */
+	cmp	x0, #3
+	b.eq	setup_el3
+	cmp	x0, #2
+	b.eq	setup_el2
+	cmp	x0, #1
+	b.eq	at_testel	     /* Already at EL1, skip transition */
+
+	/* Should not be at EL0 - error out */
+	adr 	x1, .unexp_el0
+	b	exit_msg
+
+setup_el3:
+	/* Ensure we trap if we get anything wrong */
+	adr	x0, vector_table
+	msr	vbar_el3, x0
+
+	/* Does the test want to be at EL3? */
+	cmp	w11, #3
+	beq	at_testel
+
+	/* Configure EL3 to for lower states (EL2 or EL1) */
+	mrs	x0, scr_el3
+	orr	x0, x0, #(1 << 10)    /* RW = 1: EL2/EL1 execution state is AArch64 */
+	orr	x0, x0, #(1 << 0)     /* NS = 1: Non-secure state */
+	msr	scr_el3, x0
+
+	/*
+	 * We need to check if EL2 is actually enabled via ID_AA64PFR0_EL1,
+	 * otherwise we should just jump straight to EL1.
+	 */
+	mrs	x0, id_aa64pfr0_el1
+	ubfx	x0, x0, #8, #4	      /* Extract EL2 field (bits 11:8) */
+	cbz	x0, el2_not_present   /* If field is 0 no EL2 */
+
+
+	/* Prepare SPSR for exception return to EL2 */
+	mov	x0, #0x3c9	      /* DAIF bits and EL2h mode (9) */
+	msr	spsr_el3, x0
+
+	/* Set EL2 entry point */
+	adr	x0, setup_el2
+	msr	elr_el3, x0
+
+	/* Return to EL2 */
+	eret
+
+el2_not_present:
+	/* Initialize SCTLR_EL1 with reset value */
+	msr	sctlr_el1, xzr
+
+	/* Set EL1 entry point */
+	adr	x0, at_testel
+	msr	elr_el3, x0
+
+	/* Prepare SPSR for exception return to EL1h with interrupts masked */
+	mov	x0, #0x3c5	      /* DAIF bits and EL1h mode (5) */
+	msr	spsr_el3, x0
+
+	isb			      /* Synchronization barrier */
+	eret			      /* Jump to EL1 */
+
+setup_el2:
+	/* Ensure we trap if we get anything wrong */
+	adr	x0, vector_table
+	msr	vbar_el2, x0
+
+	/* Does the test want to be at EL2? */
+	cmp	w11, #2
+	beq	at_testel
+
+	/* Configure EL2 to allow transition to EL1 */
+	mrs	x0, hcr_el2
+	orr	x0, x0, #(1 << 31)    /* RW = 1: EL1 execution state is AArch64 */
+	msr	hcr_el2, x0
+
+	/* Initialize SCTLR_EL1 with reset value */
+	msr	sctlr_el1, xzr
+
+	/* Set EL1 entry point */
+	adr	x0, at_testel
+	msr	elr_el2, x0
+
+	/* Prepare SPSR for exception return to EL1 */
+	mov	x0, #(0x5 << 0)	      /* EL1h (SPx), with interrupts disabled */
+	msr	spsr_el2, x0
+
+	/* Return to EL1 */
+	eret
+
+	/*
+	 * At the target EL for the test, usually EL1. Note we still
+	 * set everything up as if we were at EL1.
+	 */
+at_testel:
 	/* Installs a table of exception vectors to catch and handle all
 	   exceptions by terminating the process with a diagnostic.  */
 	adr	x0, vector_table
@@ -100,7 +252,7 @@ __start:
 	 * maps RAM to the first Gb. The stage2 tables have two 2mb
 	 * translation block entries covering a series of adjacent
 	 * 4k pages.
-	*/
+	 */
 
 	/* Stage 1 entry: indexed by IA[38:30] */
 	adr	x1, .				/* phys address */
@@ -198,7 +350,8 @@ __start:
 	orr	x0, x0, #(3 << 16)
 	msr	cpacr_el1, x0
 
-	/* Setup some stack space and enter the test code.
+	/*
+	 * Setup some stack space before we enter the test code.
 	 * Assume everything except the return value is garbage when we
 	 * return, we won't need it.
 	 */
@@ -233,6 +386,11 @@ __sys_outc:
 	ret
 
 	.data
+
+	.align 8
+cmdline:
+	.space 128, 0
+
 	.align	12
 
 	/* Translation table
@@ -246,6 +404,10 @@ ttb_stage2:
 	.space	4096, 0
 
 	.align	12
+system_stack:
+	.space 4096, 0
+system_stack_end:
+
 stack:
 	.space 65536, 0
 stack_end:
-- 
2.47.2

Re: [PATCH v4 03/17] tests/tcg: make aarch64 boot.S handle different starting modes

[Akihiko Odaki]

On 2025/06/03 20:01, Alex Bennée wrote:
> Currently the boot.S code assumes everything starts at EL1. This will
> break things like the memory test which will barf on unaligned memory
> access when run at a higher level.
> 
> Adapt the boot code to do some basic verification of the starting mode
> and the minimal configuration to move to the lower exception levels.
> With this we can run the memory test with:
> 
>    -M virt,secure=on
>    -M virt,secure=on,virtualization=on
>    -M virt,virtualisation=on
> 
> If a test needs to be at a particular EL it can use the semihosting
> command line to indicate the level we should execute in.
> 
> Cc: Julian Armistead <julian.armistead@linaro.org>
> Cc: Jim MacArthur <jim.macarthur@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v4
>    - drop post eret nops
>    - proper error string for EL0 error case
>    - clamp any invalid target EL value to 1
> v3
>    - create system stack so we _exit cleanly
>    - normalise EL string before compares
>    - catch when we start in a lower EL than we asked for
>    - default to EL1 when arg unclear
> v2
>    - allow tests to control the final EL we end up at
>    - use tabs consistently
>    - validate command line arg is between 1 and 3
> ---
>   tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
>   tests/tcg/aarch64/system/boot.S           | 172 +++++++++++++++++++++-
>   2 files changed, 169 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
> index 9c52475b7a..f7a7d2b800 100644
> --- a/tests/tcg/aarch64/Makefile.softmmu-target
> +++ b/tests/tcg/aarch64/Makefile.softmmu-target
> @@ -68,7 +68,8 @@ run-plugin-semiconsole-with-%: semiconsole
>   
>   # vtimer test needs EL2
>   QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
> -run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_BASE_ARGS) -kernel
> +QEMU_EL2_BASE_ARGS=-semihosting-config enable=on,target=native,chardev=output,arg="2"
> +run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_EL2_BASE_ARGS) -kernel
>   
>   # Simple Record/Replay Test
>   .PHONY: memory-record
> diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
> index a5df9c173d..8bfa4e4efc 100644
> --- a/tests/tcg/aarch64/system/boot.S
> +++ b/tests/tcg/aarch64/system/boot.S
> @@ -16,6 +16,7 @@
>   #define semihosting_call hlt 0xf000
>   #define SYS_WRITEC	0x03	/* character to debug channel */
>   #define SYS_WRITE0	0x04	/* string to debug channel */
> +#define SYS_GET_CMDLINE 0x15	/* get command line */
>   #define SYS_EXIT	0x18
>   
>   	.align	12
> @@ -70,21 +71,172 @@ lower_a32_sync:
>   lower_a32_irq:
>   lower_a32_fiq:
>   lower_a32_serror:
> +	adr	x1, .unexp_excp
> +exit_msg:
>   	mov	x0, SYS_WRITE0
> -	adr	x1, .error
>   	semihosting_call
>   	mov	x0, 1 /* EXIT_FAILURE */
>   	bl 	_exit
>   	/* never returns */
>   
>   	.section .rodata
> -.error:
> -	.string "Terminated by exception.\n"
> +.unexp_excp:
> +	.string "Unexpected exception.\n"
> +.high_el_msg:
> +	.string "Started in lower EL than requested.\n"
> +.unexp_el0:
> +	.string "Started in invalid EL.\n"
> +
> +	.align 8
> +.get_cmd:

Please do not send a new version without addressing all comments for the 
previous versions or at least noting there are unaddressed comments:
https://lore.kernel.org/qemu-devel/7a76e746-9022-48cf-8216-775071e6d631@daynix.com

Following the best practices in docs/devel/submitting-a-patch.rst will 
ensure a smoother patch review. It is fine for me if you submit a new 
version noting unaddressed comments, but some may disagree.

The same goes "[PATCH v4 11/17] ui/gtk-gl-area: Remove extra draw call 
in refresh".

Regards,
Akihiko Odaki

> +	.quad	cmdline
> +	.quad	128
>   
>   	.text
>   	.align 4
>   	.global __start
>   __start:
> +	/*
> +         * Initialise the stack for whatever EL we are in before
> +	 * anything else, we need it to be able to _exit cleanly.
> +	 * It's smaller than the stack we pass to the C code but we
> +	 * don't need much.
> +	 */
> +	adrp	x0, system_stack_end
> +	add	x0, x0, :lo12:system_stack_end
> +	mov	sp, x0
> +
> +	/*
> +	 * The test can set the semihosting command line to the target
> +	 * EL needed for the test. However if no semihosting args are set we will
> +	 * end up with -kernel/-append data (see semihosting_arg_fallback).
> +	 * Keep the normalised target in w11.
> +	 */
> +	mov	x0, SYS_GET_CMDLINE
> +	adr	x1, .get_cmd
> +	semihosting_call
> +	adrp	x10, cmdline
> +	add	x10, x10, :lo12:cmdline
> +	ldrb	w11, [x10]
> +
> +	/* sanity check, normalise char to EL, clamp to 1 if outside range */
> +	subs w11, w11, #'0'
> +	b.lt el_default
> +	cmp  w11, #3
> +	b.gt el_default
> +	b 1f
> +
> +el_high:
> +	adr	x1, .high_el_msg
> +	b	exit_msg
> +
> +el_default:
> +	mov	w11, #1
> +
> +1:
> +	/* Determine current Exception Level */
> +	mrs	x0, CurrentEL
> +	lsr	x0, x0, #2	  /* CurrentEL[3:2] contains the current EL */
> +
> +	/* Are we already in a lower EL than we want? */
> +	cmp	w11, w0
> +	bgt	el_high
> +
> +	/* Branch based on current EL */
> +	cmp	x0, #3
> +	b.eq	setup_el3
> +	cmp	x0, #2
> +	b.eq	setup_el2
> +	cmp	x0, #1
> +	b.eq	at_testel	     /* Already at EL1, skip transition */
> +
> +	/* Should not be at EL0 - error out */
> +	adr 	x1, .unexp_el0
> +	b	exit_msg
> +
> +setup_el3:
> +	/* Ensure we trap if we get anything wrong */
> +	adr	x0, vector_table
> +	msr	vbar_el3, x0
> +
> +	/* Does the test want to be at EL3? */
> +	cmp	w11, #3
> +	beq	at_testel
> +
> +	/* Configure EL3 to for lower states (EL2 or EL1) */
> +	mrs	x0, scr_el3
> +	orr	x0, x0, #(1 << 10)    /* RW = 1: EL2/EL1 execution state is AArch64 */
> +	orr	x0, x0, #(1 << 0)     /* NS = 1: Non-secure state */
> +	msr	scr_el3, x0
> +
> +	/*
> +	 * We need to check if EL2 is actually enabled via ID_AA64PFR0_EL1,
> +	 * otherwise we should just jump straight to EL1.
> +	 */
> +	mrs	x0, id_aa64pfr0_el1
> +	ubfx	x0, x0, #8, #4	      /* Extract EL2 field (bits 11:8) */
> +	cbz	x0, el2_not_present   /* If field is 0 no EL2 */
> +
> +
> +	/* Prepare SPSR for exception return to EL2 */
> +	mov	x0, #0x3c9	      /* DAIF bits and EL2h mode (9) */
> +	msr	spsr_el3, x0
> +
> +	/* Set EL2 entry point */
> +	adr	x0, setup_el2
> +	msr	elr_el3, x0
> +
> +	/* Return to EL2 */
> +	eret
> +
> +el2_not_present:
> +	/* Initialize SCTLR_EL1 with reset value */
> +	msr	sctlr_el1, xzr
> +
> +	/* Set EL1 entry point */
> +	adr	x0, at_testel
> +	msr	elr_el3, x0
> +
> +	/* Prepare SPSR for exception return to EL1h with interrupts masked */
> +	mov	x0, #0x3c5	      /* DAIF bits and EL1h mode (5) */
> +	msr	spsr_el3, x0
> +
> +	isb			      /* Synchronization barrier */
> +	eret			      /* Jump to EL1 */
> +
> +setup_el2:
> +	/* Ensure we trap if we get anything wrong */
> +	adr	x0, vector_table
> +	msr	vbar_el2, x0
> +
> +	/* Does the test want to be at EL2? */
> +	cmp	w11, #2
> +	beq	at_testel
> +
> +	/* Configure EL2 to allow transition to EL1 */
> +	mrs	x0, hcr_el2
> +	orr	x0, x0, #(1 << 31)    /* RW = 1: EL1 execution state is AArch64 */
> +	msr	hcr_el2, x0
> +
> +	/* Initialize SCTLR_EL1 with reset value */
> +	msr	sctlr_el1, xzr
> +
> +	/* Set EL1 entry point */
> +	adr	x0, at_testel
> +	msr	elr_el2, x0
> +
> +	/* Prepare SPSR for exception return to EL1 */
> +	mov	x0, #(0x5 << 0)	      /* EL1h (SPx), with interrupts disabled */
> +	msr	spsr_el2, x0
> +
> +	/* Return to EL1 */
> +	eret
> +
> +	/*
> +	 * At the target EL for the test, usually EL1. Note we still
> +	 * set everything up as if we were at EL1.
> +	 */
> +at_testel:
>   	/* Installs a table of exception vectors to catch and handle all
>   	   exceptions by terminating the process with a diagnostic.  */
>   	adr	x0, vector_table
> @@ -100,7 +252,7 @@ __start:
>   	 * maps RAM to the first Gb. The stage2 tables have two 2mb
>   	 * translation block entries covering a series of adjacent
>   	 * 4k pages.
> -	*/
> +	 */
>   
>   	/* Stage 1 entry: indexed by IA[38:30] */
>   	adr	x1, .				/* phys address */
> @@ -198,7 +350,8 @@ __start:
>   	orr	x0, x0, #(3 << 16)
>   	msr	cpacr_el1, x0
>   
> -	/* Setup some stack space and enter the test code.
> +	/*
> +	 * Setup some stack space before we enter the test code.
>   	 * Assume everything except the return value is garbage when we
>   	 * return, we won't need it.
>   	 */
> @@ -233,6 +386,11 @@ __sys_outc:
>   	ret
>   
>   	.data
> +
> +	.align 8
> +cmdline:
> +	.space 128, 0
> +
>   	.align	12
>   
>   	/* Translation table
> @@ -246,6 +404,10 @@ ttb_stage2:
>   	.space	4096, 0
>   
>   	.align	12
> +system_stack:
> +	.space 4096, 0
> +system_stack_end:
> +
>   stack:
>   	.space 65536, 0
>   stack_end:

Re: [PATCH v4 03/17] tests/tcg: make aarch64 boot.S handle different starting modes

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/06/03 20:01, Alex Bennée wrote:
>> Currently the boot.S code assumes everything starts at EL1. This will
>> break things like the memory test which will barf on unaligned memory
>> access when run at a higher level.
>> Adapt the boot code to do some basic verification of the starting
>> mode
>> and the minimal configuration to move to the lower exception levels.
>> With this we can run the memory test with:
>>    -M virt,secure=on
>>    -M virt,secure=on,virtualization=on
>>    -M virt,virtualisation=on
>> If a test needs to be at a particular EL it can use the semihosting
>> command line to indicate the level we should execute in.
>> Cc: Julian Armistead <julian.armistead@linaro.org>
>> Cc: Jim MacArthur <jim.macarthur@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>> v4
>>    - drop post eret nops
>>    - proper error string for EL0 error case
>>    - clamp any invalid target EL value to 1
>> v3
>>    - create system stack so we _exit cleanly
>>    - normalise EL string before compares
>>    - catch when we start in a lower EL than we asked for
>>    - default to EL1 when arg unclear
>> v2
>>    - allow tests to control the final EL we end up at
>>    - use tabs consistently
>>    - validate command line arg is between 1 and 3
>> ---
>>   tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
>>   tests/tcg/aarch64/system/boot.S           | 172 +++++++++++++++++++++-
>>   2 files changed, 169 insertions(+), 6 deletions(-)
>> diff --git a/tests/tcg/aarch64/Makefile.softmmu-target
>> b/tests/tcg/aarch64/Makefile.softmmu-target
>> index 9c52475b7a..f7a7d2b800 100644
>> --- a/tests/tcg/aarch64/Makefile.softmmu-target
>> +++ b/tests/tcg/aarch64/Makefile.softmmu-target
>> @@ -68,7 +68,8 @@ run-plugin-semiconsole-with-%: semiconsole
>>     # vtimer test needs EL2
>>   QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
>> -run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_BASE_ARGS) -kernel
>> +QEMU_EL2_BASE_ARGS=-semihosting-config enable=on,target=native,chardev=output,arg="2"
>> +run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_EL2_BASE_ARGS) -kernel
>>     # Simple Record/Replay Test
>>   .PHONY: memory-record
>> diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
>> index a5df9c173d..8bfa4e4efc 100644
>> --- a/tests/tcg/aarch64/system/boot.S
>> +++ b/tests/tcg/aarch64/system/boot.S
>> @@ -16,6 +16,7 @@
>>   #define semihosting_call hlt 0xf000
>>   #define SYS_WRITEC	0x03	/* character to debug channel */
>>   #define SYS_WRITE0	0x04	/* string to debug channel */
>> +#define SYS_GET_CMDLINE 0x15	/* get command line */
>>   #define SYS_EXIT	0x18
>>     	.align	12
>> @@ -70,21 +71,172 @@ lower_a32_sync:
>>   lower_a32_irq:
>>   lower_a32_fiq:
>>   lower_a32_serror:
>> +	adr	x1, .unexp_excp
>> +exit_msg:
>>   	mov	x0, SYS_WRITE0
>> -	adr	x1, .error
>>   	semihosting_call
>>   	mov	x0, 1 /* EXIT_FAILURE */
>>   	bl 	_exit
>>   	/* never returns */
>>     	.section .rodata
>> -.error:
>> -	.string "Terminated by exception.\n"
>> +.unexp_excp:
>> +	.string "Unexpected exception.\n"
>> +.high_el_msg:
>> +	.string "Started in lower EL than requested.\n"
>> +.unexp_el0:
>> +	.string "Started in invalid EL.\n"
>> +
>> +	.align 8
>> +.get_cmd:
>
> Please do not send a new version without addressing all comments for
> the previous versions or at least noting there are unaddressed
> comments:
> https://lore.kernel.org/qemu-devel/7a76e746-9022-48cf-8216-775071e6d631@daynix.com
>
> Following the best practices in docs/devel/submitting-a-patch.rst will
> ensure a smoother patch review. It is fine for me if you submit a new
> version noting unaddressed comments, but some may disagree.

There is no style guide for assembler. I have made the strings
consistently use the . prefix.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v4 03/17] tests/tcg: make aarch64 boot.S handle different starting modes

[Akihiko Odaki]

On 2025/06/05 17:51, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/06/03 20:01, Alex Bennée wrote:
>>> Currently the boot.S code assumes everything starts at EL1. This will
>>> break things like the memory test which will barf on unaligned memory
>>> access when run at a higher level.
>>> Adapt the boot code to do some basic verification of the starting
>>> mode
>>> and the minimal configuration to move to the lower exception levels.
>>> With this we can run the memory test with:
>>>     -M virt,secure=on
>>>     -M virt,secure=on,virtualization=on
>>>     -M virt,virtualisation=on
>>> If a test needs to be at a particular EL it can use the semihosting
>>> command line to indicate the level we should execute in.
>>> Cc: Julian Armistead <julian.armistead@linaro.org>
>>> Cc: Jim MacArthur <jim.macarthur@linaro.org>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>> v4
>>>     - drop post eret nops
>>>     - proper error string for EL0 error case
>>>     - clamp any invalid target EL value to 1
>>> v3
>>>     - create system stack so we _exit cleanly
>>>     - normalise EL string before compares
>>>     - catch when we start in a lower EL than we asked for
>>>     - default to EL1 when arg unclear
>>> v2
>>>     - allow tests to control the final EL we end up at
>>>     - use tabs consistently
>>>     - validate command line arg is between 1 and 3
>>> ---
>>>    tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
>>>    tests/tcg/aarch64/system/boot.S           | 172 +++++++++++++++++++++-
>>>    2 files changed, 169 insertions(+), 6 deletions(-)
>>> diff --git a/tests/tcg/aarch64/Makefile.softmmu-target
>>> b/tests/tcg/aarch64/Makefile.softmmu-target
>>> index 9c52475b7a..f7a7d2b800 100644
>>> --- a/tests/tcg/aarch64/Makefile.softmmu-target
>>> +++ b/tests/tcg/aarch64/Makefile.softmmu-target
>>> @@ -68,7 +68,8 @@ run-plugin-semiconsole-with-%: semiconsole
>>>      # vtimer test needs EL2
>>>    QEMU_EL2_MACHINE=-machine virt,virtualization=on,gic-version=2 -cpu cortex-a57 -smp 4
>>> -run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_BASE_ARGS) -kernel
>>> +QEMU_EL2_BASE_ARGS=-semihosting-config enable=on,target=native,chardev=output,arg="2"
>>> +run-vtimer: QEMU_OPTS=$(QEMU_EL2_MACHINE) $(QEMU_EL2_BASE_ARGS) -kernel
>>>      # Simple Record/Replay Test
>>>    .PHONY: memory-record
>>> diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
>>> index a5df9c173d..8bfa4e4efc 100644
>>> --- a/tests/tcg/aarch64/system/boot.S
>>> +++ b/tests/tcg/aarch64/system/boot.S
>>> @@ -16,6 +16,7 @@
>>>    #define semihosting_call hlt 0xf000
>>>    #define SYS_WRITEC	0x03	/* character to debug channel */
>>>    #define SYS_WRITE0	0x04	/* string to debug channel */
>>> +#define SYS_GET_CMDLINE 0x15	/* get command line */
>>>    #define SYS_EXIT	0x18
>>>      	.align	12
>>> @@ -70,21 +71,172 @@ lower_a32_sync:
>>>    lower_a32_irq:
>>>    lower_a32_fiq:
>>>    lower_a32_serror:
>>> +	adr	x1, .unexp_excp
>>> +exit_msg:
>>>    	mov	x0, SYS_WRITE0
>>> -	adr	x1, .error
>>>    	semihosting_call
>>>    	mov	x0, 1 /* EXIT_FAILURE */
>>>    	bl 	_exit
>>>    	/* never returns */
>>>      	.section .rodata
>>> -.error:
>>> -	.string "Terminated by exception.\n"
>>> +.unexp_excp:
>>> +	.string "Unexpected exception.\n"
>>> +.high_el_msg:
>>> +	.string "Started in lower EL than requested.\n"
>>> +.unexp_el0:
>>> +	.string "Started in invalid EL.\n"
>>> +
>>> +	.align 8
>>> +.get_cmd:
>>
>> Please do not send a new version without addressing all comments for
>> the previous versions or at least noting there are unaddressed
>> comments:
>> https://lore.kernel.org/qemu-devel/7a76e746-9022-48cf-8216-775071e6d631@daynix.com
>>
>> Following the best practices in docs/devel/submitting-a-patch.rst will
>> ensure a smoother patch review. It is fine for me if you submit a new
>> version noting unaddressed comments, but some may disagree.
> 
> There is no style guide for assembler. I have made the strings
> consistently use the . prefix.
> 

Global symbols share the symbol space with C so the naming convention of 
C can be applied to assembly too.

I also pointed out it ".error" was prefixed with a dot probably due to a 
failed attempt to make it local. There is no point of following a mistake.

I don't see a reason to differentiate the string labels from the others 
either.

Regards,
Akihiko Odaki

[PATCH v4 04/17] tests/qtest: Avoid unaligned access in IGB test

[Alex Bennée]

From: Nabih Estefan <nabihestefan@google.com>

  ../tests/qtest/libqos/igb.c:106:5: runtime error: load of misaligned address 0x562040be8e33 for type 'uint32_t', which requires 4 byte alignment

Instead of straight casting the uint8_t array, we can use ldl_le_p and
lduw_l_p to assure the unaligned access works properly against
uint32_t and uint16_t.

Signed-off-by: Nabih Estefan <nabihestefan@google.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Tested-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20250429155621.2028198-1-nabihestefan@google.com>
[AJB: fix commit message, remove unneeded casts]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

---
v3
  - update commit title to Philippe's suggestion
v2
  - change title to reflect test that failed
  - re-phrase ldl functions to assure unaligned works
  - remove excess ()'s
---
 tests/qtest/libqos/igb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqos/igb.c b/tests/qtest/libqos/igb.c
index f40c4ec4cd..ab3ef6f0c3 100644
--- a/tests/qtest/libqos/igb.c
+++ b/tests/qtest/libqos/igb.c
@@ -104,10 +104,10 @@ static void igb_pci_start_hw(QOSGraphObject *obj)
     e1000e_macreg_write(&d->e1000e, E1000_RDT(0), 0);
     e1000e_macreg_write(&d->e1000e, E1000_RDH(0), 0);
     e1000e_macreg_write(&d->e1000e, E1000_RA,
-                        le32_to_cpu(*(uint32_t *)address));
+                        ldl_le_p(address));
     e1000e_macreg_write(&d->e1000e, E1000_RA + 4,
                         E1000_RAH_AV | E1000_RAH_POOL_1 |
-                        le16_to_cpu(*(uint16_t *)(address + 4)));
+                        lduw_le_p(address + 4));
 
     /* Set supported receive descriptor mode */
     e1000e_macreg_write(&d->e1000e,
-- 
2.47.2

[PATCH v4 05/17] contrib/plugins: add a scaling factor to the ips arg

[Alex Bennée]

It's easy to get lost in zeros while setting the numbers of
instructions per second. Add a scaling suffix to make things simpler.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

---
v2
  - normalise the suffix before a full strcmp0
  - check endptr actually set
  - fix checkpatch
  - scale_entry -> ScaleEntry
  - drop hz from suffix
---
 contrib/plugins/ips.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
index e5297dbb01..eb4418c25b 100644
--- a/contrib/plugins/ips.c
+++ b/contrib/plugins/ips.c
@@ -129,6 +129,18 @@ static void plugin_exit(qemu_plugin_id_t id, void *udata)
     qemu_plugin_scoreboard_free(vcpus);
 }
 
+typedef struct {
+    const char *suffix;
+    unsigned long multipler;
+} ScaleEntry;
+
+/* a bit like units.h but not binary */
+static ScaleEntry scales[] = {
+    { "k", 1000 },
+    { "m", 1000 * 1000 },
+    { "g", 1000 * 1000 * 1000 },
+};
+
 QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                                            const qemu_info_t *info, int argc,
                                            char **argv)
@@ -137,12 +149,32 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
         char *opt = argv[i];
         g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
         if (g_strcmp0(tokens[0], "ips") == 0) {
-            max_insn_per_second = g_ascii_strtoull(tokens[1], NULL, 10);
+            char *endptr = NULL;
+            max_insn_per_second = g_ascii_strtoull(tokens[1], &endptr, 10);
             if (!max_insn_per_second && errno) {
                 fprintf(stderr, "%s: couldn't parse %s (%s)\n",
                         __func__, tokens[1], g_strerror(errno));
                 return -1;
             }
+
+            if (endptr && *endptr != 0) {
+                g_autofree gchar *lower = g_utf8_strdown(endptr, -1);
+                unsigned long scale = 0;
+
+                for (int j = 0; j < G_N_ELEMENTS(scales); j++) {
+                    if (g_strcmp0(lower, scales[j].suffix) == 0) {
+                        scale = scales[j].multipler;
+                        break;
+                    }
+                }
+
+                if (scale) {
+                    max_insn_per_second *= scale;
+                } else {
+                    fprintf(stderr, "bad suffix: %s\n", endptr);
+                    return -1;
+                }
+            }
         } else {
             fprintf(stderr, "option parsing failed: %s\n", opt);
             return -1;
-- 
2.47.2

[PATCH v4 06/17] contrib/plugins: allow setting of instructions per quantum

[Alex Bennée]

The default is we update time every 1/10th of a second or so. However
for some cases we might want to update time more frequently. Allow
this to be set via the command line through the ipq argument.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

---
v4
  - simplify error checking, don't bother with errno
v3
  - error checking for ipq
v2
  - checkpatch fixes.
---
 docs/about/emulation.rst |  4 ++++
 contrib/plugins/ips.c    | 15 ++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/docs/about/emulation.rst b/docs/about/emulation.rst
index a72591ee4d..456d01d5b0 100644
--- a/docs/about/emulation.rst
+++ b/docs/about/emulation.rst
@@ -811,6 +811,10 @@ This plugin can limit the number of Instructions Per Second that are executed::
   * - ips=N
     - Maximum number of instructions per cpu that can be executed in one second.
       The plugin will sleep when the given number of instructions is reached.
+  * - ipq=N
+    - Instructions per quantum. How many instructions before we re-calculate time.
+      The lower the number the more accurate time will be, but the less efficient the plugin.
+      Defaults to ips/10
 
 Other emulation features
 ------------------------
diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
index eb4418c25b..c9948976f6 100644
--- a/contrib/plugins/ips.c
+++ b/contrib/plugins/ips.c
@@ -145,6 +145,8 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                                            const qemu_info_t *info, int argc,
                                            char **argv)
 {
+    bool ipq_set = false;
+
     for (int i = 0; i < argc; i++) {
         char *opt = argv[i];
         g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
@@ -175,6 +177,14 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                     return -1;
                 }
             }
+        } else if (g_strcmp0(tokens[0], "ipq") == 0) {
+            max_insn_per_quantum = g_ascii_strtoull(tokens[1], NULL, 10);
+
+            if (!max_insn_per_quantum) {
+                fprintf(stderr, "bad ipq value: %s\n", tokens[0]);
+                return -1;
+            }
+            ipq_set = true;
         } else {
             fprintf(stderr, "option parsing failed: %s\n", opt);
             return -1;
@@ -182,7 +192,10 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
     }
 
     vcpus = qemu_plugin_scoreboard_new(sizeof(vCPUTime));
-    max_insn_per_quantum = max_insn_per_second / NUM_TIME_UPDATE_PER_SEC;
+
+    if (!ipq_set) {
+        max_insn_per_quantum = max_insn_per_second / NUM_TIME_UPDATE_PER_SEC;
+    }
 
     if (max_insn_per_quantum == 0) {
         fprintf(stderr, "minimum of %d instructions per second needed\n",
-- 
2.47.2

[PATCH v4 07/17] MAINTAINERS: add myself to virtio-gpu for Odd Fixes

[Alex Bennée]

Seeing as I've taken a few patches to here now I might as well put
myself forward to maintain virtio-gpu. I've marked it as Odd Fixes as
it is not my core focus. If someone with more GPU experience comes
forward we can always update again.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - s/M:/S:/ for the maintainer entry
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 16af37986a..7718199979 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2668,7 +2668,8 @@ F: hw/display/ramfb*.c
 F: include/hw/display/ramfb.h
 
 virtio-gpu
-S: Orphan
+M: Alex Bennée <alex.bennee@linaro.org>
+S: Odd Fixes
 F: hw/display/virtio-gpu*
 F: hw/display/virtio-vga.*
 F: include/hw/virtio/virtio-gpu.h
-- 
2.47.2

[PATCH v4 08/17] MAINTAINERS: add Akihiko and Dmitry as reviewers

[Alex Bennée]

Thanks for volunteering to help.

Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7718199979..79b1d5c0b3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2669,6 +2669,8 @@ F: include/hw/display/ramfb.h
 
 virtio-gpu
 M: Alex Bennée <alex.bennee@linaro.org>
+R: Akihiko Odaki <akihiko.odaki@daynix.com>
+R: Dmitry Osipenko <dmitry.osipenko@collabora.com>
 S: Odd Fixes
 F: hw/display/virtio-gpu*
 F: hw/display/virtio-vga.*
-- 
2.47.2

Re: [PATCH v4 08/17] MAINTAINERS: add Akihiko and Dmitry as reviewers

[Akihiko Odaki]

On 2025/06/03 20:01, Alex Bennée wrote:
> Thanks for volunteering to help.
> 
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   MAINTAINERS | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7718199979..79b1d5c0b3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2669,6 +2669,8 @@ F: include/hw/display/ramfb.h
>   
>   virtio-gpu
>   M: Alex Bennée <alex.bennee@linaro.org>
> +R: Akihiko Odaki <akihiko.odaki@daynix.com>

Please update my email address according to:
https://lore.kernel.org/qemu-devel/20250531-rsg-v1-1-e0bae1e1d90e@rsg.ci.i.u-tokyo.ac.jp/

> +R: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>   S: Odd Fixes
>   F: hw/display/virtio-gpu*
>   F: hw/display/virtio-vga.*

[PATCH v4 09/17] hw/display: re-arrange memory region tracking

[Alex Bennée]

QOM objects can be embedded in other QOM objects and managed as part
of their lifetime but this isn't the case for
virtio_gpu_virgl_hostmem_region. However before we can split it out we
need some other way of associating the wider data structure with the
memory region.

Fortunately MemoryRegion has an opaque pointer. This is passed down to
MemoryRegionOps for device type regions but is unused in the
memory_region_init_ram_ptr() case. Use the opaque to carry the
reference and allow the final MemoryRegion object to be reaped when
its reference count is cleared.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
Cc: qemu-stable@nongnu.org
---
 include/system/memory.h       |  1 +
 hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/include/system/memory.h b/include/system/memory.h
index fc35a0dcad..90715ff44a 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -784,6 +784,7 @@ struct MemoryRegion {
     DeviceState *dev;
 
     const MemoryRegionOps *ops;
+    /* opaque data, used by backends like @ops */
     void *opaque;
     MemoryRegion *container;
     int mapped_via_alias; /* Mapped via an alias, container might be NULL */
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 145a0b3879..71a7500de9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
 
 #if VIRGL_VERSION_MAJOR >= 1
 struct virtio_gpu_virgl_hostmem_region {
-    MemoryRegion mr;
+    MemoryRegion *mr;
     struct VirtIOGPU *g;
     bool finish_unmapping;
 };
 
-static struct virtio_gpu_virgl_hostmem_region *
-to_hostmem_region(MemoryRegion *mr)
-{
-    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
-}
-
 static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
 {
     VirtIOGPU *g = opaque;
@@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
 static void virtio_gpu_virgl_hostmem_region_free(void *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
-    struct virtio_gpu_virgl_hostmem_region *vmr;
+    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
     VirtIOGPUBase *b;
     VirtIOGPUGL *gl;
 
-    vmr = to_hostmem_region(mr);
-    vmr->finish_unmapping = true;
-
     b = VIRTIO_GPU_BASE(vmr->g);
+    vmr->finish_unmapping = true;
     b->renderer_blocked--;
 
     /*
@@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
 
     vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
     vmr->g = g;
+    mr = g_new0(MemoryRegion, 1);
 
-    mr = &vmr->mr;
     memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
     memory_region_add_subregion(&b->hostmem, offset, mr);
     memory_region_set_enabled(mr, true);
@@ -131,7 +123,9 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
      * command processing until MR is fully unreferenced and freed.
      */
     OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free;
+    mr->opaque = vmr;
 
+    vmr->mr = mr;
     res->mr = mr;
 
     return 0;
@@ -142,16 +136,15 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
                                      struct virtio_gpu_virgl_resource *res,
                                      bool *cmd_suspended)
 {
-    struct virtio_gpu_virgl_hostmem_region *vmr;
     VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
     MemoryRegion *mr = res->mr;
+    struct virtio_gpu_virgl_hostmem_region *vmr;
     int ret;
 
     if (!mr) {
         return 0;
     }
-
-    vmr = to_hostmem_region(res->mr);
+    vmr = mr->opaque;
 
     /*
      * Perform async unmapping in 3 steps:
-- 
2.47.2

Re: [PATCH v4 09/17] hw/display: re-arrange memory region tracking

[Akihiko Odaki]

On 2025/06/03 20:01, Alex Bennée wrote:
> QOM objects can be embedded in other QOM objects and managed as part
> of their lifetime but this isn't the case for
> virtio_gpu_virgl_hostmem_region. However before we can split it out we
> need some other way of associating the wider data structure with the
> memory region.
> 
> Fortunately MemoryRegion has an opaque pointer. This is passed down to
> MemoryRegionOps for device type regions but is unused in the
> memory_region_init_ram_ptr() case. Use the opaque to carry the
> reference and allow the final MemoryRegion object to be reaped when
> its reference count is cleared.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>   include/system/memory.h       |  1 +
>   hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
>   2 files changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/include/system/memory.h b/include/system/memory.h
> index fc35a0dcad..90715ff44a 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -784,6 +784,7 @@ struct MemoryRegion {
>       DeviceState *dev;
>   
>       const MemoryRegionOps *ops;
> +    /* opaque data, used by backends like @ops */
>       void *opaque;
>       MemoryRegion *container;
>       int mapped_via_alias; /* Mapped via an alias, container might be NULL */
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 145a0b3879..71a7500de9 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>   
>   #if VIRGL_VERSION_MAJOR >= 1
>   struct virtio_gpu_virgl_hostmem_region {
> -    MemoryRegion mr;
> +    MemoryRegion *mr;
>       struct VirtIOGPU *g;
>       bool finish_unmapping;
>   };
>   
> -static struct virtio_gpu_virgl_hostmem_region *
> -to_hostmem_region(MemoryRegion *mr)
> -{
> -    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
> -}
> -
>   static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>   {
>       VirtIOGPU *g = opaque;
> @@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>   static void virtio_gpu_virgl_hostmem_region_free(void *obj)
>   {
>       MemoryRegion *mr = MEMORY_REGION(obj);
> -    struct virtio_gpu_virgl_hostmem_region *vmr;
> +    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
>       VirtIOGPUBase *b;
>       VirtIOGPUGL *gl;
>   
> -    vmr = to_hostmem_region(mr);
> -    vmr->finish_unmapping = true;
> -
>       b = VIRTIO_GPU_BASE(vmr->g);
> +    vmr->finish_unmapping = true;
>       b->renderer_blocked--;
>   
>       /*
> @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>   
>       vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>       vmr->g = g;
> +    mr = g_new0(MemoryRegion, 1);

This patch does nothing more than adding a separate allocation for 
MemoryRegion. Besides there is no corresponding g_free(). This patch can 
be simply dropped.

Regards,
Akihiko Odaki

>   
> -    mr = &vmr->mr;
>       memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
>       memory_region_add_subregion(&b->hostmem, offset, mr);
>       memory_region_set_enabled(mr, true);
> @@ -131,7 +123,9 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>        * command processing until MR is fully unreferenced and freed.
>        */
>       OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free;
> +    mr->opaque = vmr;
>   
> +    vmr->mr = mr;
>       res->mr = mr;
>   
>       return 0;
> @@ -142,16 +136,15 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>                                        struct virtio_gpu_virgl_resource *res,
>                                        bool *cmd_suspended)
>   {
> -    struct virtio_gpu_virgl_hostmem_region *vmr;
>       VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>       MemoryRegion *mr = res->mr;
> +    struct virtio_gpu_virgl_hostmem_region *vmr;
>       int ret;
>   
>       if (!mr) {
>           return 0;
>       }
> -
> -    vmr = to_hostmem_region(res->mr);
> +    vmr = mr->opaque;
>   
>       /*
>        * Perform async unmapping in 3 steps:

Re: [PATCH v4 09/17] hw/display: re-arrange memory region tracking

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/06/03 20:01, Alex Bennée wrote:
>> QOM objects can be embedded in other QOM objects and managed as part
>> of their lifetime but this isn't the case for
>> virtio_gpu_virgl_hostmem_region. However before we can split it out we
>> need some other way of associating the wider data structure with the
>> memory region.
>> Fortunately MemoryRegion has an opaque pointer. This is passed down
>> to
>> MemoryRegionOps for device type regions but is unused in the
>> memory_region_init_ram_ptr() case. Use the opaque to carry the
>> reference and allow the final MemoryRegion object to be reaped when
>> its reference count is cleared.
>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> ---
>>   include/system/memory.h       |  1 +
>>   hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
>>   2 files changed, 9 insertions(+), 15 deletions(-)
>> diff --git a/include/system/memory.h b/include/system/memory.h
>> index fc35a0dcad..90715ff44a 100644
>> --- a/include/system/memory.h
>> +++ b/include/system/memory.h
>> @@ -784,6 +784,7 @@ struct MemoryRegion {
>>       DeviceState *dev;
>>         const MemoryRegionOps *ops;
>> +    /* opaque data, used by backends like @ops */
>>       void *opaque;
>>       MemoryRegion *container;
>>       int mapped_via_alias; /* Mapped via an alias, container might be NULL */
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index 145a0b3879..71a7500de9 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>     #if VIRGL_VERSION_MAJOR >= 1
>>   struct virtio_gpu_virgl_hostmem_region {
>> -    MemoryRegion mr;
>> +    MemoryRegion *mr;
>>       struct VirtIOGPU *g;
>>       bool finish_unmapping;
>>   };
>>   -static struct virtio_gpu_virgl_hostmem_region *
>> -to_hostmem_region(MemoryRegion *mr)
>> -{
>> -    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
>> -}
>> -
>>   static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>   {
>>       VirtIOGPU *g = opaque;
>> @@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>   static void virtio_gpu_virgl_hostmem_region_free(void *obj)
>>   {
>>       MemoryRegion *mr = MEMORY_REGION(obj);
>> -    struct virtio_gpu_virgl_hostmem_region *vmr;
>> +    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
>>       VirtIOGPUBase *b;
>>       VirtIOGPUGL *gl;
>>   -    vmr = to_hostmem_region(mr);
>> -    vmr->finish_unmapping = true;
>> -
>>       b = VIRTIO_GPU_BASE(vmr->g);
>> +    vmr->finish_unmapping = true;
>>       b->renderer_blocked--;
>>         /*
>> @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>         vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>       vmr->g = g;
>> +    mr = g_new0(MemoryRegion, 1);
>
> This patch does nothing more than adding a separate allocation for
> MemoryRegion. Besides there is no corresponding g_free(). This patch
> can be simply dropped.

As the patch says the MemoryRegion is now free'd when it is
de-referenced. Do you have a test case showing it leaking?


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v4 09/17] hw/display: re-arrange memory region tracking

[Akihiko Odaki]

On 2025/06/05 20:57, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/06/03 20:01, Alex Bennée wrote:
>>> QOM objects can be embedded in other QOM objects and managed as part
>>> of their lifetime but this isn't the case for
>>> virtio_gpu_virgl_hostmem_region. However before we can split it out we
>>> need some other way of associating the wider data structure with the
>>> memory region.
>>> Fortunately MemoryRegion has an opaque pointer. This is passed down
>>> to
>>> MemoryRegionOps for device type regions but is unused in the
>>> memory_region_init_ram_ptr() case. Use the opaque to carry the
>>> reference and allow the final MemoryRegion object to be reaped when
>>> its reference count is cleared.
>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
>>> Cc: qemu-stable@nongnu.org
>>> ---
>>>    include/system/memory.h       |  1 +
>>>    hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
>>>    2 files changed, 9 insertions(+), 15 deletions(-)
>>> diff --git a/include/system/memory.h b/include/system/memory.h
>>> index fc35a0dcad..90715ff44a 100644
>>> --- a/include/system/memory.h
>>> +++ b/include/system/memory.h
>>> @@ -784,6 +784,7 @@ struct MemoryRegion {
>>>        DeviceState *dev;
>>>          const MemoryRegionOps *ops;
>>> +    /* opaque data, used by backends like @ops */
>>>        void *opaque;
>>>        MemoryRegion *container;
>>>        int mapped_via_alias; /* Mapped via an alias, container might be NULL */
>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>> index 145a0b3879..71a7500de9 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>>      #if VIRGL_VERSION_MAJOR >= 1
>>>    struct virtio_gpu_virgl_hostmem_region {
>>> -    MemoryRegion mr;
>>> +    MemoryRegion *mr;
>>>        struct VirtIOGPU *g;
>>>        bool finish_unmapping;
>>>    };
>>>    -static struct virtio_gpu_virgl_hostmem_region *
>>> -to_hostmem_region(MemoryRegion *mr)
>>> -{
>>> -    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
>>> -}
>>> -
>>>    static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>    {
>>>        VirtIOGPU *g = opaque;
>>> @@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>    static void virtio_gpu_virgl_hostmem_region_free(void *obj)
>>>    {
>>>        MemoryRegion *mr = MEMORY_REGION(obj);
>>> -    struct virtio_gpu_virgl_hostmem_region *vmr;
>>> +    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
>>>        VirtIOGPUBase *b;
>>>        VirtIOGPUGL *gl;
>>>    -    vmr = to_hostmem_region(mr);
>>> -    vmr->finish_unmapping = true;
>>> -
>>>        b = VIRTIO_GPU_BASE(vmr->g);
>>> +    vmr->finish_unmapping = true;
>>>        b->renderer_blocked--;
>>>          /*
>>> @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>          vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>>        vmr->g = g;
>>> +    mr = g_new0(MemoryRegion, 1);
>>
>> This patch does nothing more than adding a separate allocation for
>> MemoryRegion. Besides there is no corresponding g_free(). This patch
>> can be simply dropped.
> 
> As the patch says the MemoryRegion is now free'd when it is
> de-referenced. Do you have a test case showing it leaking?

"De-referenced" is confusing and sounds like pointer dereferencing.

OBJECT(mr)->free, which has virtio_gpu_virgl_hostmem_region_free() as 
its value, will be called to free mr when the references of mr are 
removed. This patch however does not add a corresponding g_free() call 
to virtio_gpu_virgl_hostmem_region_free(), leaking mr.

AddressSanitizer will catch the memory leak.

Regards,
Akihiko Odaki

Re: [PATCH v4 09/17] hw/display: re-arrange memory region tracking

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/06/05 20:57, Alex Bennée wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>> 
>>> On 2025/06/03 20:01, Alex Bennée wrote:
>>>> QOM objects can be embedded in other QOM objects and managed as part
>>>> of their lifetime but this isn't the case for
>>>> virtio_gpu_virgl_hostmem_region. However before we can split it out we
>>>> need some other way of associating the wider data structure with the
>>>> memory region.
>>>> Fortunately MemoryRegion has an opaque pointer. This is passed down
>>>> to
>>>> MemoryRegionOps for device type regions but is unused in the
>>>> memory_region_init_ram_ptr() case. Use the opaque to carry the
>>>> reference and allow the final MemoryRegion object to be reaped when
>>>> its reference count is cleared.
>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
>>>> Cc: qemu-stable@nongnu.org
>>>> ---
>>>>    include/system/memory.h       |  1 +
>>>>    hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
>>>>    2 files changed, 9 insertions(+), 15 deletions(-)
>>>> diff --git a/include/system/memory.h b/include/system/memory.h
>>>> index fc35a0dcad..90715ff44a 100644
>>>> --- a/include/system/memory.h
>>>> +++ b/include/system/memory.h
>>>> @@ -784,6 +784,7 @@ struct MemoryRegion {
>>>>        DeviceState *dev;
>>>>          const MemoryRegionOps *ops;
>>>> +    /* opaque data, used by backends like @ops */
>>>>        void *opaque;
>>>>        MemoryRegion *container;
>>>>        int mapped_via_alias; /* Mapped via an alias, container might be NULL */
>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>>> index 145a0b3879..71a7500de9 100644
>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>> @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>>>      #if VIRGL_VERSION_MAJOR >= 1
>>>>    struct virtio_gpu_virgl_hostmem_region {
>>>> -    MemoryRegion mr;
>>>> +    MemoryRegion *mr;
>>>>        struct VirtIOGPU *g;
>>>>        bool finish_unmapping;
>>>>    };
>>>>    -static struct virtio_gpu_virgl_hostmem_region *
>>>> -to_hostmem_region(MemoryRegion *mr)
>>>> -{
>>>> -    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
>>>> -}
>>>> -
>>>>    static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>>    {
>>>>        VirtIOGPU *g = opaque;
>>>> @@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>>    static void virtio_gpu_virgl_hostmem_region_free(void *obj)
>>>>    {
>>>>        MemoryRegion *mr = MEMORY_REGION(obj);
>>>> -    struct virtio_gpu_virgl_hostmem_region *vmr;
>>>> +    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
>>>>        VirtIOGPUBase *b;
>>>>        VirtIOGPUGL *gl;
>>>>    -    vmr = to_hostmem_region(mr);
>>>> -    vmr->finish_unmapping = true;
>>>> -
>>>>        b = VIRTIO_GPU_BASE(vmr->g);
>>>> +    vmr->finish_unmapping = true;
>>>>        b->renderer_blocked--;
>>>>          /*
>>>> @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>>          vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>>>        vmr->g = g;
>>>> +    mr = g_new0(MemoryRegion, 1);
>>>
>>> This patch does nothing more than adding a separate allocation for
>>> MemoryRegion. Besides there is no corresponding g_free(). This patch
>>> can be simply dropped.
>> As the patch says the MemoryRegion is now free'd when it is
>> de-referenced. Do you have a test case showing it leaking?
>
> "De-referenced" is confusing and sounds like pointer dereferencing.
>
> OBJECT(mr)->free, which has virtio_gpu_virgl_hostmem_region_free() as
> its value, will be called to free mr when the references of mr are
> removed. This patch however does not add a corresponding g_free() call
> to virtio_gpu_virgl_hostmem_region_free(), leaking mr.
>
> AddressSanitizer will catch the memory leak.

Example invocation?

I ran the AddressSantizier against all the virtio-gpu tests yesterday
and it did not complain.

>
> Regards,
> Akihiko Odaki

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v4 09/17] hw/display: re-arrange memory region tracking

[Akihiko Odaki]

On 2025/06/06 19:16, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/06/05 20:57, Alex Bennée wrote:
>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>
>>>> On 2025/06/03 20:01, Alex Bennée wrote:
>>>>> QOM objects can be embedded in other QOM objects and managed as part
>>>>> of their lifetime but this isn't the case for
>>>>> virtio_gpu_virgl_hostmem_region. However before we can split it out we
>>>>> need some other way of associating the wider data structure with the
>>>>> memory region.
>>>>> Fortunately MemoryRegion has an opaque pointer. This is passed down
>>>>> to
>>>>> MemoryRegionOps for device type regions but is unused in the
>>>>> memory_region_init_ram_ptr() case. Use the opaque to carry the
>>>>> reference and allow the final MemoryRegion object to be reaped when
>>>>> its reference count is cleared.
>>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
>>>>> Cc: qemu-stable@nongnu.org
>>>>> ---
>>>>>     include/system/memory.h       |  1 +
>>>>>     hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
>>>>>     2 files changed, 9 insertions(+), 15 deletions(-)
>>>>> diff --git a/include/system/memory.h b/include/system/memory.h
>>>>> index fc35a0dcad..90715ff44a 100644
>>>>> --- a/include/system/memory.h
>>>>> +++ b/include/system/memory.h
>>>>> @@ -784,6 +784,7 @@ struct MemoryRegion {
>>>>>         DeviceState *dev;
>>>>>           const MemoryRegionOps *ops;
>>>>> +    /* opaque data, used by backends like @ops */
>>>>>         void *opaque;
>>>>>         MemoryRegion *container;
>>>>>         int mapped_via_alias; /* Mapped via an alias, container might be NULL */
>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>>>> index 145a0b3879..71a7500de9 100644
>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>> @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>>>>       #if VIRGL_VERSION_MAJOR >= 1
>>>>>     struct virtio_gpu_virgl_hostmem_region {
>>>>> -    MemoryRegion mr;
>>>>> +    MemoryRegion *mr;
>>>>>         struct VirtIOGPU *g;
>>>>>         bool finish_unmapping;
>>>>>     };
>>>>>     -static struct virtio_gpu_virgl_hostmem_region *
>>>>> -to_hostmem_region(MemoryRegion *mr)
>>>>> -{
>>>>> -    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
>>>>> -}
>>>>> -
>>>>>     static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>>>     {
>>>>>         VirtIOGPU *g = opaque;
>>>>> @@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>>>     static void virtio_gpu_virgl_hostmem_region_free(void *obj)
>>>>>     {
>>>>>         MemoryRegion *mr = MEMORY_REGION(obj);
>>>>> -    struct virtio_gpu_virgl_hostmem_region *vmr;
>>>>> +    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
>>>>>         VirtIOGPUBase *b;
>>>>>         VirtIOGPUGL *gl;
>>>>>     -    vmr = to_hostmem_region(mr);
>>>>> -    vmr->finish_unmapping = true;
>>>>> -
>>>>>         b = VIRTIO_GPU_BASE(vmr->g);
>>>>> +    vmr->finish_unmapping = true;
>>>>>         b->renderer_blocked--;
>>>>>           /*
>>>>> @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>>>           vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>>>>         vmr->g = g;
>>>>> +    mr = g_new0(MemoryRegion, 1);
>>>>
>>>> This patch does nothing more than adding a separate allocation for
>>>> MemoryRegion. Besides there is no corresponding g_free(). This patch
>>>> can be simply dropped.
>>> As the patch says the MemoryRegion is now free'd when it is
>>> de-referenced. Do you have a test case showing it leaking?
>>
>> "De-referenced" is confusing and sounds like pointer dereferencing.
>>
>> OBJECT(mr)->free, which has virtio_gpu_virgl_hostmem_region_free() as
>> its value, will be called to free mr when the references of mr are
>> removed. This patch however does not add a corresponding g_free() call
>> to virtio_gpu_virgl_hostmem_region_free(), leaking mr.
>>
>> AddressSanitizer will catch the memory leak.
> 
> Example invocation?
> 
> I ran the AddressSantizier against all the virtio-gpu tests yesterday
> and it did not complain.

The following command line triggered the memory leak. The image is a 
clean Debian 12 installation. I booted the installation, and shut down 
it by pressing the button on the booted GDM:

build/qemu-system-x86_64 -drive file=debian12.qcow2 -m 8G -smp 8 -device 
virtio-vga-gl,blob=on,hostmem=1G -display egl-headless,gl=on -vnc :0 -M 
q35,accel=kvm
==361968==WARNING: ASan doesn't fully support makecontext/swapcontext 
functions and may produce false positives in some cases!
==361968==WARNING: ASan is ignoring requested __asan_handle_no_return: 
stack type: default top: 0x7bf41d2b8380; bottom 0x7bf2e0f4c000; size: 
0x00013c36c380 (5305189248)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189

=================================================================
==361968==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 816 byte(s) in 3 object(s) allocated from:
     #0 0x7ff640f50a43 in calloc (/lib64/libasan.so.8+0xe6a43) (BuildId: 
6a82bb83b1f19d3f3a2118085acf79daa3b52371)
     #1 0x7ff64077c901 in g_malloc0 (/lib64/libglib-2.0.so.0+0x48901) 
(BuildId: 6827394d759bc44f207f57e7ab5f8e6b17e82c1c)
     #2 0x557fa8080dc5 in virtio_gpu_virgl_map_resource_blob 
../hw/display/virtio-gpu-virgl.c:113
     #3 0x557fa8080dc5 in virgl_cmd_resource_map_blob 
../hw/display/virtio-gpu-virgl.c:772
     #4 0x557fa8080dc5 in virtio_gpu_virgl_process_cmd 
../hw/display/virtio-gpu-virgl.c:952

SUMMARY: AddressSanitizer: 816 byte(s) leaked in 3 allocation(s).

Regards,
Akihiko Odaki

Re: [PATCH v4 09/17] hw/display: re-arrange memory region tracking

[Akihiko Odaki]

On 2025/06/07 0:02, Akihiko Odaki wrote:
> 
> 
> On 2025/06/06 19:16, Alex Bennée wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>
>>> On 2025/06/05 20:57, Alex Bennée wrote:
>>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>>
>>>>> On 2025/06/03 20:01, Alex Bennée wrote:
>>>>>> QOM objects can be embedded in other QOM objects and managed as part
>>>>>> of their lifetime but this isn't the case for
>>>>>> virtio_gpu_virgl_hostmem_region. However before we can split it 
>>>>>> out we
>>>>>> need some other way of associating the wider data structure with the
>>>>>> memory region.
>>>>>> Fortunately MemoryRegion has an opaque pointer. This is passed down
>>>>>> to
>>>>>> MemoryRegionOps for device type regions but is unused in the
>>>>>> memory_region_init_ram_ptr() case. Use the opaque to carry the
>>>>>> reference and allow the final MemoryRegion object to be reaped when
>>>>>> its reference count is cleared.
>>>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
>>>>>> Cc: qemu-stable@nongnu.org
>>>>>> ---
>>>>>>     include/system/memory.h       |  1 +
>>>>>>     hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
>>>>>>     2 files changed, 9 insertions(+), 15 deletions(-)
>>>>>> diff --git a/include/system/memory.h b/include/system/memory.h
>>>>>> index fc35a0dcad..90715ff44a 100644
>>>>>> --- a/include/system/memory.h
>>>>>> +++ b/include/system/memory.h
>>>>>> @@ -784,6 +784,7 @@ struct MemoryRegion {
>>>>>>         DeviceState *dev;
>>>>>>           const MemoryRegionOps *ops;
>>>>>> +    /* opaque data, used by backends like @ops */
>>>>>>         void *opaque;
>>>>>>         MemoryRegion *container;
>>>>>>         int mapped_via_alias; /* Mapped via an alias, container 
>>>>>> might be NULL */
>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio- 
>>>>>> gpu-virgl.c
>>>>>> index 145a0b3879..71a7500de9 100644
>>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>>> @@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
>>>>>>       #if VIRGL_VERSION_MAJOR >= 1
>>>>>>     struct virtio_gpu_virgl_hostmem_region {
>>>>>> -    MemoryRegion mr;
>>>>>> +    MemoryRegion *mr;
>>>>>>         struct VirtIOGPU *g;
>>>>>>         bool finish_unmapping;
>>>>>>     };
>>>>>>     -static struct virtio_gpu_virgl_hostmem_region *
>>>>>> -to_hostmem_region(MemoryRegion *mr)
>>>>>> -{
>>>>>> -    return container_of(mr, struct 
>>>>>> virtio_gpu_virgl_hostmem_region, mr);
>>>>>> -}
>>>>>> -
>>>>>>     static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>>>>     {
>>>>>>         VirtIOGPU *g = opaque;
>>>>>> @@ -73,14 +67,12 @@ static void 
>>>>>> virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
>>>>>>     static void virtio_gpu_virgl_hostmem_region_free(void *obj)
>>>>>>     {
>>>>>>         MemoryRegion *mr = MEMORY_REGION(obj);
>>>>>> -    struct virtio_gpu_virgl_hostmem_region *vmr;
>>>>>> +    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
>>>>>>         VirtIOGPUBase *b;
>>>>>>         VirtIOGPUGL *gl;
>>>>>>     -    vmr = to_hostmem_region(mr);
>>>>>> -    vmr->finish_unmapping = true;
>>>>>> -
>>>>>>         b = VIRTIO_GPU_BASE(vmr->g);
>>>>>> +    vmr->finish_unmapping = true;
>>>>>>         b->renderer_blocked--;
>>>>>>           /*
>>>>>> @@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>>>>>>           vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
>>>>>>         vmr->g = g;
>>>>>> +    mr = g_new0(MemoryRegion, 1);
>>>>>
>>>>> This patch does nothing more than adding a separate allocation for
>>>>> MemoryRegion. Besides there is no corresponding g_free(). This patch
>>>>> can be simply dropped.
>>>> As the patch says the MemoryRegion is now free'd when it is
>>>> de-referenced. Do you have a test case showing it leaking?
>>>
>>> "De-referenced" is confusing and sounds like pointer dereferencing.
>>>
>>> OBJECT(mr)->free, which has virtio_gpu_virgl_hostmem_region_free() as
>>> its value, will be called to free mr when the references of mr are
>>> removed. This patch however does not add a corresponding g_free() call
>>> to virtio_gpu_virgl_hostmem_region_free(), leaking mr.
>>>
>>> AddressSanitizer will catch the memory leak.
>>
>> Example invocation?
>>
>> I ran the AddressSantizier against all the virtio-gpu tests yesterday
>> and it did not complain.
> 
> The following command line triggered the memory leak. The image is a 
> clean Debian 12 installation. I booted the installation, and shut down 
> it by pressing the button on the booted GDM:
> 
> build/qemu-system-x86_64 -drive file=debian12.qcow2 -m 8G -smp 8 -device 
> virtio-vga-gl,blob=on,hostmem=1G -display egl-headless,gl=on -vnc :0 -M 
> q35,accel=kvm
> ==361968==WARNING: ASan doesn't fully support makecontext/swapcontext 
> functions and may produce false positives in some cases!
> ==361968==WARNING: ASan is ignoring requested __asan_handle_no_return: 
> stack type: default top: 0x7bf41d2b8380; bottom 0x7bf2e0f4c000; size: 
> 0x00013c36c380 (5305189248)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> 
> =================================================================
> ==361968==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 816 byte(s) in 3 object(s) allocated from:
>      #0 0x7ff640f50a43 in calloc (/lib64/libasan.so.8+0xe6a43) (BuildId: 
> 6a82bb83b1f19d3f3a2118085acf79daa3b52371)
>      #1 0x7ff64077c901 in g_malloc0 (/lib64/libglib-2.0.so.0+0x48901) 
> (BuildId: 6827394d759bc44f207f57e7ab5f8e6b17e82c1c)
>      #2 0x557fa8080dc5 in virtio_gpu_virgl_map_resource_blob ../hw/ 
> display/virtio-gpu-virgl.c:113
>      #3 0x557fa8080dc5 in virgl_cmd_resource_map_blob ../hw/display/ 
> virtio-gpu-virgl.c:772
>      #4 0x557fa8080dc5 in virtio_gpu_virgl_process_cmd ../hw/display/ 
> virtio-gpu-virgl.c:952
> 
> SUMMARY: AddressSanitizer: 816 byte(s) leaked in 3 allocation(s).

The following command line also reproduced the issue:

LIBGL_ALWAYS_SOFTWARE=1 \
LSAN_OPTIONS=suppressions=<(echo leak:fontconfig) \
build/qemu-system-aarch64 -M virt,accel=kvm -cpu host -smp 8 -m 8G \
-device virtio-gpu-gl,blob=on,hostmem=256M -display gtk,gl=on \
-drive file=Fedora-Workstation-Live-42-1.1.aarch64.iso,format=raw \
-bios /usr/share/edk2/aarch64/QEMU_EFI-silent-pflash.raw \
-serial mon:stdio

Fedora's Live disk allows you to test without installation.

By the way, I tried TCG to reproduce the hang, but I couldn't. I'd 
appreciate if you tell:

- how to reproduce the issue
- whether the CPU usage saturates with 100 %
   (i.e., if the hang is a busy-loop or not).
- the stack traces of all the threads when the hang happens.

Hopefully they will provide more insights into the problem and your fix.

Regards,
Akihiko Odaki

[PATCH v4 10/17] virtio-gpu: refactor async blob unmapping

[Alex Bennée]

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Change the 3 part async cleanup of a blob memory mapping to check if the
unmapping has finished already after deleting the subregion; this
condition allows us to skip suspending the command and responding to the
guest right away.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20250410122643.1747913-4-manos.pitsidianakis@linaro.org>
Cc: qemu-stable@nongnu.org
---
 hw/display/virtio-gpu-virgl.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 71a7500de9..b4aa8abb96 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -155,7 +155,32 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
      *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
      * 3. Finish the unmapping with final virgl_renderer_resource_unmap().
      */
+
+    /* 1. Check if we should start unmapping now */
+    if (!vmr->finish_unmapping) {
+        /* begin async unmapping. render will be unblocked once MR is freed */
+        b->renderer_blocked++;
+
+        memory_region_set_enabled(mr, false);
+        memory_region_del_subregion(&b->hostmem, mr);
+        object_unparent(OBJECT(mr));
+        /*
+         * The unmapping might have already finished at this point if no one
+         * else held a reference to the MR; if yes, we can skip suspending the
+         * command and unmap the resource right away.
+         */
+        *cmd_suspended = !vmr->finish_unmapping;
+    }
+
+    /*
+     * 2. if virtio_gpu_virgl_hostmem_region_free hasn't been executed yet, we
+     * have marked the command to be re-processed later by setting
+     * cmd_suspended to true. The freeing callback will be called from RCU
+     * context later.
+     */
+
     if (vmr->finish_unmapping) {
+        /* 3. MemoryRegion has been freed, so finish unmapping */
         res->mr = NULL;
         g_free(vmr);
 
@@ -166,16 +191,6 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
                           __func__, strerror(-ret));
             return ret;
         }
-    } else {
-        *cmd_suspended = true;
-
-        /* render will be unblocked once MR is freed */
-        b->renderer_blocked++;
-
-        /* memory region owns self res->mr object and frees it by itself */
-        memory_region_set_enabled(mr, false);
-        memory_region_del_subregion(&b->hostmem, mr);
-        object_unparent(OBJECT(mr));
     }
 
     return 0;
-- 
2.47.2

[PATCH v4 11/17] ui/gtk-gl-area: Remove extra draw call in refresh

[Alex Bennée]

From: Dongwon Kim <dongwon.kim@intel.com>

This partially reverts commit 77bf310084dad38b3a2badf01766c659056f1cf2
which causes some guest display corruption when gtk-gl-area
is used for GTK rendering (e.g. Wayland Compositor) possibly due to
simulataneous accesses on the guest frame buffer by host compositor
and the guest.

Fixes: 77bf310084 ("ui/gtk: Draw guest frame at refresh cycle")
Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Reported-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
Message-Id: <20250214170813.2234754-1-dongwon.kim@intel.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 ui/gtk-gl-area.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 8151cc413c..429ba914e0 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -168,7 +168,6 @@ void gd_gl_area_refresh(DisplayChangeListener *dcl)
 
     if (vc->gfx.guest_fb.dmabuf &&
         qemu_dmabuf_get_draw_submitted(vc->gfx.guest_fb.dmabuf)) {
-        gd_gl_area_draw(vc);
         return;
     }
 
-- 
2.47.2

[PATCH v4 12/17] virtio-gpu: support context init multiple timeline

[Alex Bennée]

From: Yiwei Zhang <zzyiwei@gmail.com>

Venus and later native contexts have their own fence context along with
multiple timelines within. Fences wtih VIRTIO_GPU_FLAG_INFO_RING_IDX in
the flags must be dispatched to be created on the target context. Fence
signaling also has to be handled on the specific timeline within that
target context.

Before this change, venus fencing is completely broken if the host
driver doesn't support implicit fencing with external memory objects.
Frames can go backwards along with random artifacts on screen if the
host driver doesn't attach an implicit fence to the render target. The
symptom could be hidden by certain guest wsi backend that waits on a
venus native VkFence object for the actual payload with limited present
modes or under special configs. e.g. x11 mailbox or xwayland.

After this change, everything related to venus fencing starts making
sense. Confirmed this via guest and host side perfetto tracing.

Cc:  <qemu-stable@nongnu.org>
Fixes: 94d0ea1c1928 ("virtio-gpu: Support Venus context")
Signed-off-by: Yiwei Zhang <zzyiwei@gmail.com>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Message-Id: <20250518152651.334115-1-zzyiwei@gmail.com>
[AJB: remove version history from commit message]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-virgl.c | 44 +++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index b4aa8abb96..cea2e12eb9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -978,6 +978,15 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
     }
 
     trace_virtio_gpu_fence_ctrl(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
+#if VIRGL_VERSION_MAJOR >= 1
+    if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
+        virgl_renderer_context_create_fence(cmd->cmd_hdr.ctx_id,
+                                            VIRGL_RENDERER_FENCE_FLAG_MERGEABLE,
+                                            cmd->cmd_hdr.ring_idx,
+                                            cmd->cmd_hdr.fence_id);
+        return;
+    }
+#endif
     virgl_renderer_create_fence(cmd->cmd_hdr.fence_id, cmd->cmd_hdr.type);
 }
 
@@ -991,6 +1000,11 @@ static void virgl_write_fence(void *opaque, uint32_t fence)
          * the guest can end up emitting fences out of order
          * so we should check all fenced cmds not just the first one.
          */
+#if VIRGL_VERSION_MAJOR >= 1
+        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX) {
+            continue;
+        }
+#endif
         if (cmd->cmd_hdr.fence_id > fence) {
             continue;
         }
@@ -1005,6 +1019,29 @@ static void virgl_write_fence(void *opaque, uint32_t fence)
     }
 }
 
+#if VIRGL_VERSION_MAJOR >= 1
+static void virgl_write_context_fence(void *opaque, uint32_t ctx_id,
+                                      uint32_t ring_idx, uint64_t fence_id) {
+    VirtIOGPU *g = opaque;
+    struct virtio_gpu_ctrl_command *cmd, *tmp;
+
+    QTAILQ_FOREACH_SAFE(cmd, &g->fenceq, next, tmp) {
+        if (cmd->cmd_hdr.flags & VIRTIO_GPU_FLAG_INFO_RING_IDX &&
+            cmd->cmd_hdr.ctx_id == ctx_id && cmd->cmd_hdr.ring_idx == ring_idx &&
+            cmd->cmd_hdr.fence_id <= fence_id) {
+            trace_virtio_gpu_fence_resp(cmd->cmd_hdr.fence_id);
+            virtio_gpu_ctrl_response_nodata(g, cmd, VIRTIO_GPU_RESP_OK_NODATA);
+            QTAILQ_REMOVE(&g->fenceq, cmd, next);
+            g_free(cmd);
+            g->inflight--;
+            if (virtio_gpu_stats_enabled(g->parent_obj.conf)) {
+                trace_virtio_gpu_dec_inflight_fences(g->inflight);
+            }
+        }
+    }
+}
+#endif
+
 static virgl_renderer_gl_context
 virgl_create_context(void *opaque, int scanout_idx,
                      struct virgl_renderer_gl_ctx_param *params)
@@ -1039,11 +1076,18 @@ static int virgl_make_context_current(void *opaque, int scanout_idx,
 }
 
 static struct virgl_renderer_callbacks virtio_gpu_3d_cbs = {
+#if VIRGL_VERSION_MAJOR >= 1
+    .version             = 3,
+#else
     .version             = 1,
+#endif
     .write_fence         = virgl_write_fence,
     .create_gl_context   = virgl_create_context,
     .destroy_gl_context  = virgl_destroy_context,
     .make_current        = virgl_make_context_current,
+#if VIRGL_VERSION_MAJOR >= 1
+    .write_context_fence = virgl_write_context_fence,
+#endif
 };
 
 static void virtio_gpu_print_stats(void *opaque)
-- 
2.47.2

[PATCH v4 13/17] include/exec: fix assert in size_memop

[Alex Bennée]

We can handle larger sized memops now, expand the range of the assert.

Fixes: 4b473e0c60 (tcg: Expand MO_SIZE to 3 bits)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

---
v2
  - instead of 128 use 1 << MO_SIZE for future proofing
v3
  - fix comment, 1 << MO_SIZE goes to 1024
v4
  - assert is_power_of_2()
---
 include/exec/memop.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/exec/memop.h b/include/exec/memop.h
index 407a47d82c..cf7da3362e 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -162,8 +162,8 @@ static inline unsigned memop_size(MemOp op)
 static inline MemOp size_memop(unsigned size)
 {
 #ifdef CONFIG_DEBUG_TCG
-    /* Power of 2 up to 8.  */
-    assert((size & (size - 1)) == 0 && size >= 1 && size <= 8);
+    /* Power of 2 up to 1024 */
+    assert(is_power_of_2(size) && size >= 1 && size <= (1 << MO_SIZE));
 #endif
     return (MemOp)ctz32(size);
 }
-- 
2.47.2

[PATCH v4 14/17] include/gdbstub: fix include guard in commands.h

[Alex Bennée]

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/gdbstub/commands.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
index 40f0514fe9..bff3674872 100644
--- a/include/gdbstub/commands.h
+++ b/include/gdbstub/commands.h
@@ -1,5 +1,5 @@
 #ifndef GDBSTUB_COMMANDS_H
-#define GDBSTUB
+#define GDBSTUB_COMMANDS_H
 
 typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
 
-- 
2.47.2

[PATCH v4 15/17] gdbstub: assert earlier in handle_read_all_regs

[Alex Bennée]

When things go wrong we want to assert on the register that failed to
be able to figure out what went wrong.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub/gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 565f6b33a9..6023c80d25 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1343,8 +1343,8 @@ static void handle_read_all_regs(GArray *params, void *user_ctx)
         len += gdb_read_register(gdbserver_state.g_cpu,
                                  gdbserver_state.mem_buf,
                                  reg_id);
+        g_assert(len == gdbserver_state.mem_buf->len);
     }
-    g_assert(len == gdbserver_state.mem_buf->len);
 
     gdb_memtohex(gdbserver_state.str_buf, gdbserver_state.mem_buf->data, len);
     gdb_put_strbuf();
-- 
2.47.2

[PATCH v4 16/17] gdbstub: Implement qGDBServerVersion packet

[Alex Bennée]

From: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>

This commit adds support for the `qGDBServerVersion` packet to the qemu
gdbstub  which could be used by clients to detect the QEMU version
(and, e.g., use a workaround for known bugs).

This packet is not documented/standarized by GDB but it was implemented
by LLDB gdbstub [0] and is helpful for projects like Pwndbg [1].

This has been implemented by Patryk, who I included in Co-authored-by
and who asked me to send the patch.

[0] https://lldb.llvm.org/resources/lldbgdbremote.html#qgdbserverversion
[1] https://github.com/pwndbg/pwndbg/issues/2648

Co-authored-by: Patryk 'patryk4815' Sondej <patryk.sondej@gmail.com>
Signed-off-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
Message-Id: <20250403191340.53343-1-dominik.b.czarnota@gmail.com>
[AJB: fix include, checkpatch linewrap]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub/gdbstub.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 6023c80d25..def0b7e877 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -28,6 +28,7 @@
 #include "qemu/cutils.h"
 #include "qemu/module.h"
 #include "qemu/error-report.h"
+#include "qemu/target-info.h"
 #include "trace.h"
 #include "exec/gdbstub.h"
 #include "gdbstub/commands.h"
@@ -1597,6 +1598,18 @@ static void handle_query_threads(GArray *params, void *user_ctx)
     gdbserver_state.query_cpu = gdb_next_attached_cpu(gdbserver_state.query_cpu);
 }
 
+static void handle_query_gdb_server_version(GArray *params, void *user_ctx)
+{
+#if defined(CONFIG_USER_ONLY)
+    g_string_printf(gdbserver_state.str_buf, "name:qemu-%s;version:%s;",
+                    target_name(), QEMU_VERSION);
+#else
+    g_string_printf(gdbserver_state.str_buf, "name:qemu-system-%s;version:%s;",
+                    target_name(), QEMU_VERSION);
+#endif
+    gdb_put_strbuf();
+}
+
 static void handle_query_first_threads(GArray *params, void *user_ctx)
 {
     gdbserver_state.query_cpu = gdb_first_attached_cpu();
@@ -1842,6 +1855,10 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
         .handler = handle_query_threads,
         .cmd = "sThreadInfo",
     },
+    {
+        .handler = handle_query_gdb_server_version,
+        .cmd = "GDBServerVersion",
+    },
     {
         .handler = handle_query_first_threads,
         .cmd = "fThreadInfo",
-- 
2.47.2

[PATCH v4 17/17] gdbstub: update aarch64-core.xml

[Alex Bennée]

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Update aarch64-core.xml to include field definitions for PSTATE, which
in gdb is modelled in the cpsr (current program status register)
pseudo-register, named after the actual cpsr register in armv7.

Defining the fields layout of the register allows easy inspection of for
example, the current exception level (EL):

For example. Before booting a Linux guest, EL=2, but after booting and
Ctrl-C'ing in gdb, we get EL=0:

  (gdb) info registers $cpsr
  cpsr           0x20402009          [ SP EL=2 BTYPE=0 PAN C ]
  (gdb) cont
  Continuing.
  ^C
  Thread 2 received signal SIGINT, Interrupt.
  0x0000ffffaaff286c in ?? ()
  (gdb) info registers $cpsr
  cpsr           0x20001000          [ EL=0 BTYPE=0 SSBS C ]

The aarch64-core.xml has been updated to match exactly the version
retrieved from upstream gdb, retrieved in 2025-05-19 from HEAD commit
9f4dc0b137c86f6ff2098cb1ab69442c69d6023d.

Link: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/features/aarch64-core.xml;h=b8046510b9a085d30463d37b3ecc8d435f5fb7a4;hb=HEAD
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Message-Id: <20250519-gdbstub-aarch64-pstate-xml-v1-1-b4dbe87fe7c6@linaro.org>
[AJB: expanded upstream link]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdb-xml/aarch64-core.xml | 52 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/gdb-xml/aarch64-core.xml b/gdb-xml/aarch64-core.xml
index e1e9dc3f91..b8046510b9 100644
--- a/gdb-xml/aarch64-core.xml
+++ b/gdb-xml/aarch64-core.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0"?>
-<!-- Copyright (C) 2009-2012 Free Software Foundation, Inc.
+<!-- Copyright (C) 2009-2025 Free Software Foundation, Inc.
      Contributed by ARM Ltd.
 
      Copying and distribution of this file, with or without modification,
@@ -42,5 +42,53 @@
   <reg name="sp" bitsize="64" type="data_ptr"/>
 
   <reg name="pc" bitsize="64" type="code_ptr"/>
-  <reg name="cpsr" bitsize="32"/>
+
+  <flags id="cpsr_flags" size="4">
+    <!-- Stack Pointer.  -->
+    <field name="SP" start="0" end="0"/>
+
+    <!-- Exception Level.  -->
+    <field name="EL" start="2" end="3"/>
+    <!-- Execution state.  -->
+    <field name="nRW" start="4" end="4"/>
+
+    <!-- FIQ interrupt mask.  -->
+    <field name="F" start="6" end="6"/>
+    <!-- IRQ interrupt mask.  -->
+    <field name="I" start="7" end="7"/>
+    <!-- SError interrupt mask.  -->
+    <field name="A" start="8" end="8"/>
+    <!-- Debug exception mask.  -->
+    <field name="D" start="9" end="9"/>
+
+    <!-- ARMv8.5-A: Branch Target Identification BTYPE.  -->
+    <field name="BTYPE" start="10" end="11"/>
+
+    <!-- ARMv8.0-A: Speculative Store Bypass.  -->
+    <field name="SSBS" start="12" end="12"/>
+
+    <!-- Illegal Execution state.  -->
+    <field name="IL" start="20" end="20"/>
+    <!-- Software Step.  -->
+    <field name="SS" start="21" end="21"/>
+    <!-- ARMv8.1-A: Privileged Access Never.  -->
+    <field name="PAN" start="22" end="22"/>
+    <!-- ARMv8.2-A: User Access Override.  -->
+    <field name="UAO" start="23" end="23"/>
+    <!-- ARMv8.4-A: Data Independent Timing.  -->
+    <field name="DIT" start="24" end="24"/>
+    <!-- ARMv8.5-A: Tag Check Override.  -->
+    <field name="TCO" start="25" end="25"/>
+
+    <!-- Overflow Condition flag.  -->
+    <field name="V" start="28" end="28"/>
+    <!-- Carry Condition flag.  -->
+    <field name="C" start="29" end="29"/>
+    <!-- Zero Condition flag.  -->
+    <field name="Z" start="30" end="30"/>
+    <!-- Negative Condition flag.  -->
+    <field name="N" start="31" end="31"/>
+  </flags>
+  <reg name="cpsr" bitsize="32" type="cpsr_flags"/>
+
 </feature>
-- 
2.47.2