[PATCH v2 00/14] Maintainer updates for May (testing, plugins, virtio-gpu)

[PATCH v2 00/14] Maintainer updates for May (testing, plugins, virtio-gpu)

[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.

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

The following still need review:

  MAINTAINERS: add Akihiko and Dmitry as reviewers
  MAINTAINERS: add myself to virtio-gpu for Odd Fixes
  tests/tcg: make aarch64 boot.S handle different starting modes
  tests/docker: expose $HOME/.cache/qemu as docker volume

Alex.

Alex Bennée (8):
  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

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

Manos Pitsidianakis (2):
  virtio-gpu: fix hang under TCG when unmapping blob
  virtio-gpu: refactor async blob unmapping

Nabih Estefan (1):
  Running with --enable-ubsan leads to a qtest failure

Peter Maydell (2):
  docs: Create a uniquelabel Sphinx extension
  docs: Use uniquelabel in qemu-block-drivers.rst.inc

 MAINTAINERS                               |   5 +-
 docs/about/emulation.rst                  |   4 +
 docs/conf.py                              |   1 +
 docs/devel/codebase.rst                   |   2 +-
 docs/sphinx/uniquelabel.py                |  74 ++++++++++++
 docs/system/qemu-block-drivers.rst.inc    |   2 +-
 include/system/memory.h                   |   1 +
 contrib/plugins/ips.c                     |  46 +++++++-
 hw/display/virtio-gpu-virgl.c             |  60 +++++-----
 tests/qtest/libqos/igb.c                  |   4 +-
 ui/gtk-gl-area.c                          |   1 -
 .gitlab-ci.d/buildtest-template.yml       |   1 +
 tests/docker/Makefile.include             |  10 +-
 tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
 tests/tcg/aarch64/system/boot.S           | 135 +++++++++++++++++++++-
 15 files changed, 309 insertions(+), 40 deletions(-)
 create mode 100644 docs/sphinx/uniquelabel.py

-- 
2.39.5

[PATCH v2 01/14] 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.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
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.39.5

[PATCH v2 02/14] 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 d4f145fdb5..d9e69c3237 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.39.5

[PATCH v2 03/14] 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>

---
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           | 135 +++++++++++++++++++++-
 2 files changed, 136 insertions(+), 2 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..a52d28c881 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
@@ -81,10 +82,137 @@ lower_a32_serror:
 .error:
 	.string "Terminated by exception.\n"
 
+	.align 8
+.get_cmd:
+	.quad	cmdline
+	.quad	128
+
 	.text
 	.align 4
 	.global __start
 __start:
+	/*
+	 * The test can set the semihosting command line to the target
+	 * EL needed for the test. Keep that in w11.
+	 */
+	mov	x0, SYS_GET_CMDLINE
+	adr	x1, .get_cmd
+	semihosting_call
+	adrp	x10, cmdline
+	add	x10, x10, :lo12:cmdline
+	ldrb	w11, [x10]
+	cbz	w11, 2f
+
+	/* sanity check, clamp to 1 if invalid */
+	cmp w11, #'0'
+	b.lt 1f
+	cmp w11, #'4'
+	b.ge 1f
+	sub	w11, w11, #'0'
+	b 2f
+1:	mov w11, #1
+
+2:
+	/* Determine current Exception Level */
+	mrs	x0, CurrentEL
+	lsr	x0, x0, #2	  /* CurrentEL[3:2] contains the current EL */
+
+	/* 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 */
+	b	curr_sp0_sync
+
+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
+	nop
+
+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
+
+	nop
+
+	/*
+	 * 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 +228,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 */
@@ -233,6 +361,11 @@ __sys_outc:
 	ret
 
 	.data
+
+	.align 8
+cmdline:
+	.space 128, 0
+
 	.align	12
 
 	/* Translation table
-- 
2.39.5

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

[Akihiko Odaki]

On 2025/05/06 21:57, 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>
> 
> ---
> 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           | 135 +++++++++++++++++++++-
>   2 files changed, 136 insertions(+), 2 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..a52d28c881 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
> @@ -81,10 +82,137 @@ lower_a32_serror:
>   .error:
>   	.string "Terminated by exception.\n"
>   
> +	.align 8
> +.get_cmd:

This label is prefixed with a dot, which is inconsistent with the other 
labels except ".error".

I guess ".error" is prefixed with a dot to make it local, but a local 
symbol needs to be prefixed with ".L" instead according to:
https://sourceware.org/binutils/docs-2.41/as/Symbol-Names.html#Local-Symbol-Names

 > A local symbol is any symbol beginning with certain local label
 > prefixes. By default, the local label prefix is ‘.L’ for ELF systems
 > or ‘L’ for traditional a.out systems, but each target may have its own
 > set of local label prefixes. On the HPPA local symbols begin with
 > ‘L$’.

> +	.quad	cmdline
> +	.quad	128
> +
>   	.text
>   	.align 4
>   	.global __start
>   __start:
> +	/*
> +	 * The test can set the semihosting command line to the target
> +	 * EL needed for the test. Keep that in w11.
> +	 */
> +	mov	x0, SYS_GET_CMDLINE
> +	adr	x1, .get_cmd
> +	semihosting_call
> +	adrp	x10, cmdline
> +	add	x10, x10, :lo12:cmdline
 > +	ldrb	w11, [x10]> +	cbz	w11, 2f
> +
> +	/* sanity check, clamp to 1 if invalid */
> +	cmp w11, #'0'
> +	b.lt 1f
> +	cmp w11, #'4'
> +	b.ge 1f
> +	sub	w11, w11, #'0'
> +	b 2f
> +1:	mov w11, #1
> +
> +2:

This "sanity check" made me wonder what it is for. This code is simply 
unnecessary if any unknown values are to be ignored and is a bit 
misleading to have this code as it looks like it adds an additional 
behavior. "sub w11, w11, #'0'" is also unnecessary; we can compare w11 
with '2' and '3' later instead.

The patch message says this sanity check was added with v2 so I looked 
for a previous review and found this:
https://lore.kernel.org/qemu-devel/svmkvs.2mf5q4qhsfz83@linaro.org/

 > cmp w11, #'0'
 > b.lt curr_sp0_sync
 > cmp w11, #'4'
 > b.ge curr_sp0_sync

Now I get the original intent; it was to raise an error instead of 
simply ignoring unknown values. However I also see a few problems with 
this original code:
- It still ignores unknown strings that are longer than one character.
- curr_sp0_sync leads to the code that writes a message saying 
"Terminated by exception.", which is incorrect.
- "cmp w11, #'0'" is unnecessary if you check the value after "sub	w11, 
w11, #'0'".

> +	/* Determine current Exception Level */
> +	mrs	x0, CurrentEL
> +	lsr	x0, x0, #2	  /* CurrentEL[3:2] contains the current EL */
> +
> +	/* 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 */
> +	b	curr_sp0_sync
> +
> +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
> +	nop

Why is a nop placed here?

> +
> +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
> +
> +	nop
> +
> +	/*
> +	 * 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 +228,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 */
> @@ -233,6 +361,11 @@ __sys_outc:
>   	ret
>   
>   	.data
> +
> +	.align 8
> +cmdline:
> +	.space 128, 0
> +
>   	.align	12
>   
>   	/* Translation table

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

[Alex Bennée]

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

> On 2025/05/06 21:57, 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>
<snip>
>> +
>> +	/* sanity check, clamp to 1 if invalid */
>> +	cmp w11, #'0'
>> +	b.lt 1f
>> +	cmp w11, #'4'
>> +	b.ge 1f
>> +	sub	w11, w11, #'0'
>> +	b 2f
>> +1:	mov w11, #1
>> +
>> +2:
>
> This "sanity check" made me wonder what it is for. This code is simply
> unnecessary if any unknown values are to be ignored and is a bit
> misleading to have this code as it looks like it adds an additional
> behavior. "sub w11, w11, #'0'" is also unnecessary; we can compare w11
> with '2' and '3' later instead.
>
> The patch message says this sanity check was added with v2 so I looked
> for a previous review and found this:
> https://lore.kernel.org/qemu-devel/svmkvs.2mf5q4qhsfz83@linaro.org/
>
>> cmp w11, #'0'
>> b.lt curr_sp0_sync
>> cmp w11, #'4'
>> b.ge curr_sp0_sync
>
> Now I get the original intent; it was to raise an error instead of
> simply ignoring unknown values. However I also see a few problems with
> this original code:
> - It still ignores unknown strings that are longer than one character.

I'm not intending to handle longer strings in assembly - I think we can
live with accepting "1junk" to "3foo".

> - curr_sp0_sync leads to the code that writes a message saying
>   "Terminated by exception.", which is incorrect.
> - "cmp w11, #'0'" is unnecessary if you check the value after "sub
>   w11, w11, #'0'".

I'll clean that up.

>
>> +	/* Determine current Exception Level */
>> +	mrs	x0, CurrentEL
>> +	lsr	x0, x0, #2	  /* CurrentEL[3:2] contains the current EL */
>> +
>> +	/* 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 */
>> +	b	curr_sp0_sync
>> +
>> +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
>> +	nop
>
> Why is a nop placed here?

Left over debug. I'll drop it.
>
<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 04/14] Running with --enable-ubsan leads to a qtest failure

[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 use use ldl_le_p and
lduw_l_p to assure alignment is correct 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>
---
 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..7df9272069 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.39.5

Re: [PATCH v2 04/14] Running with --enable-ubsan leads to a qtest failure

[Akihiko Odaki]

I think the subject should tell what this patch does and perhaps the 
underlying problem it solves instead of the symptom. It may be nice to 
have a prefix "tests/qtest/libqos/igb:" to comply the convention.

On 2025/05/06 21:57, Alex Bennée wrote:
> 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 use use ldl_le_p and
> lduw_l_p to assure alignment is correct against uint32_t and uint16_t.

It does not assure an alignment is correct but instead performs an 
unaligned access.

> 
> 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>
> ---
>   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..7df9272069 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)));

This line has are extra parentheses.

>   
>       /* Set supported receive descriptor mode */
>       e1000e_macreg_write(&d->e1000e,

[PATCH v2 05/14] 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
---
 contrib/plugins/ips.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
index e5297dbb01..9b166a7d6c 100644
--- a/contrib/plugins/ips.c
+++ b/contrib/plugins/ips.c
@@ -20,6 +20,8 @@
 
 QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
 /* how many times do we update time per sec */
 #define NUM_TIME_UPDATE_PER_SEC 10
 #define NSEC_IN_ONE_SEC (1000 * 1000 * 1000)
@@ -129,6 +131,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;
+} scale_entry;
+
+/* a bit like units.h but not binary */
+static scale_entry scales[] = {
+    { "khz", 1000 },
+    { "mhz", 1000 * 1000 },
+    { "ghz", 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 +151,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 < ARRAY_SIZE(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.39.5

Re: [PATCH v2 05/14] contrib/plugins: add a scaling factor to the ips arg

[Akihiko Odaki]

On 2025/05/06 21:57, Alex Bennée wrote:
> 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
> ---
>   contrib/plugins/ips.c | 36 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
> index e5297dbb01..9b166a7d6c 100644
> --- a/contrib/plugins/ips.c
> +++ b/contrib/plugins/ips.c
> @@ -20,6 +20,8 @@
>   
>   QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>   
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +

G_N_ELEMENTS() is already available.

>   /* how many times do we update time per sec */
>   #define NUM_TIME_UPDATE_PER_SEC 10
>   #define NSEC_IN_ONE_SEC (1000 * 1000 * 1000)
> @@ -129,6 +131,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;

I prefer to have an explicitly-sized type: uint32_t in this case. It 
also saves typing several characters as a bonus.

> +} scale_entry;

docs/devel/style.rst says
 > Structured type names are in CamelCase; harder to type but standing
 > out.

> +
> +/* a bit like units.h but not binary */
> +static scale_entry scales[] = {
> +    { "khz", 1000 },
> +    { "mhz", 1000 * 1000 },
> +    { "ghz", 1000 * 1000 * 1000 },

Having "hz" as suffixes look a bit awkard. "1 giga instructions per 
second" sounds natural, but "1 gigahertz instructions per second" 
doesn't to me. Practically, it would be easier to just type "g" instead 
of "ghz".

util/cutils.c has similar code though I guess a plugin cannot be linked 
to it.

> +};
> +
>   QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>                                              const qemu_info_t *info, int argc,
>                                              char **argv)
> @@ -137,12 +151,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 < ARRAY_SIZE(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;

Re: [PATCH v2 05/14] contrib/plugins: add a scaling factor to the ips arg

[Alex Bennée]

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

> On 2025/05/06 21:57, Alex Bennée wrote:
>> 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
>> ---
>>   contrib/plugins/ips.c | 36 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 35 insertions(+), 1 deletion(-)
>> diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
>> index e5297dbb01..9b166a7d6c 100644
>> --- a/contrib/plugins/ips.c
>> +++ b/contrib/plugins/ips.c
>> @@ -20,6 +20,8 @@
>>     QEMU_PLUGIN_EXPORT int qemu_plugin_version =
>> QEMU_PLUGIN_VERSION;
>>   +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>> +
>
> G_N_ELEMENTS() is already available.
>
>>   /* how many times do we update time per sec */
>>   #define NUM_TIME_UPDATE_PER_SEC 10
>>   #define NSEC_IN_ONE_SEC (1000 * 1000 * 1000)
>> @@ -129,6 +131,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;
>
> I prefer to have an explicitly-sized type: uint32_t in this case. It
> also saves typing several characters as a bonus.

4Ghz would be a reasonable size and that would overflow a simple
uint32_t unless we start casting.

>
>> +} scale_entry;
>
> docs/devel/style.rst says
>> Structured type names are in CamelCase; harder to type but standing
>> out.
>
>> +
>> +/* a bit like units.h but not binary */
>> +static scale_entry scales[] = {
>> +    { "khz", 1000 },
>> +    { "mhz", 1000 * 1000 },
>> +    { "ghz", 1000 * 1000 * 1000 },
>
> Having "hz" as suffixes look a bit awkard. "1 giga instructions per
> second" sounds natural, but "1 gigahertz instructions per second"
> doesn't to me. Practically, it would be easier to just type "g"
> instead of "ghz".
>
> util/cutils.c has similar code though I guess a plugin cannot be
> linked to it.
>
>> +};
>> +
>>   QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
>>                                              const qemu_info_t *info, int argc,
>>                                              char **argv)
>> @@ -137,12 +151,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 < ARRAY_SIZE(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;

I've fixed the other cases.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 05/14] contrib/plugins: add a scaling factor to the ips arg

[Akihiko Odaki]

On 2025/05/19 23:54, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/05/06 21:57, Alex Bennée wrote:
>>> 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
>>> ---
>>>    contrib/plugins/ips.c | 36 +++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 35 insertions(+), 1 deletion(-)
>>> diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
>>> index e5297dbb01..9b166a7d6c 100644
>>> --- a/contrib/plugins/ips.c
>>> +++ b/contrib/plugins/ips.c
>>> @@ -20,6 +20,8 @@
>>>      QEMU_PLUGIN_EXPORT int qemu_plugin_version =
>>> QEMU_PLUGIN_VERSION;
>>>    +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>> +
>>
>> G_N_ELEMENTS() is already available.
>>
>>>    /* how many times do we update time per sec */
>>>    #define NUM_TIME_UPDATE_PER_SEC 10
>>>    #define NSEC_IN_ONE_SEC (1000 * 1000 * 1000)
>>> @@ -129,6 +131,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;
>>
>> I prefer to have an explicitly-sized type: uint32_t in this case. It
>> also saves typing several characters as a bonus.
> 
> 4Ghz would be a reasonable size and that would overflow a simple
> uint32_t unless we start casting.

unsigned long can be 32-bit so it should be uint64_t if you want to 
avoid overflow without casting.

Regards,
Akihiko Odaki

[PATCH v2 06/14] 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>

---
v2
  - checkpatch fixes.
---
 docs/about/emulation.rst |  4 ++++
 contrib/plugins/ips.c    | 10 +++++++++-
 2 files changed, 13 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 9b166a7d6c..62ed8ddd08 100644
--- a/contrib/plugins/ips.c
+++ b/contrib/plugins/ips.c
@@ -147,6 +147,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);
@@ -177,6 +179,9 @@ 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);
+            ipq_set = true;
         } else {
             fprintf(stderr, "option parsing failed: %s\n", opt);
             return -1;
@@ -184,7 +189,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.39.5

Re: [PATCH v2 06/14] contrib/plugins: allow setting of instructions per quantum

[Akihiko Odaki]

On 2025/05/06 21:57, Alex Bennée wrote:
> 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>
> 
> ---
> v2
>    - checkpatch fixes.
> ---
>   docs/about/emulation.rst |  4 ++++
>   contrib/plugins/ips.c    | 10 +++++++++-
>   2 files changed, 13 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 9b166a7d6c..62ed8ddd08 100644
> --- a/contrib/plugins/ips.c
> +++ b/contrib/plugins/ips.c
> @@ -147,6 +147,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);
> @@ -177,6 +179,9 @@ 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);

It may be nice to have an error check performed for ips.

> +            ipq_set = true;
>           } else {
>               fprintf(stderr, "option parsing failed: %s\n", opt);
>               return -1;
> @@ -184,7 +189,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",

Re: [PATCH v2 06/14] contrib/plugins: allow setting of instructions per quantum

[Alex Bennée]

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

> On 2025/05/06 21:57, Alex Bennée wrote:
>> 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>
>> ---
>> v2
>>    - checkpatch fixes.
>> ---
>>   docs/about/emulation.rst |  4 ++++
>>   contrib/plugins/ips.c    | 10 +++++++++-
>>   2 files changed, 13 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 9b166a7d6c..62ed8ddd08 100644
>> --- a/contrib/plugins/ips.c
>> +++ b/contrib/plugins/ips.c
>> @@ -147,6 +147,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);
>> @@ -177,6 +179,9 @@ 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);
>
> It may be nice to have an error check performed for ips.

We do catch most errors bellow on max_insn_per_quantum = 0, but I can
check up here as well.

>
>> +            ipq_set = true;
>>           } else {
>>               fprintf(stderr, "option parsing failed: %s\n", opt);
>>               return -1;
>> @@ -184,7 +189,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",

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 07/14] 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.

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 b3f9f2680b..d11f8ac18c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2644,7 +2644,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.39.5

[PATCH v2 08/14] 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>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d11f8ac18c..2df6c09894 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2645,6 +2645,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.39.5

[PATCH v2 09/14] 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 fbbf4cf911..b3cef1acb5 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -783,6 +783,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.39.5

[PATCH v2 10/14] virtio-gpu: fix hang under TCG when unmapping blob

[Alex Bennée]

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

This commit fixes an indefinite hang when using VIRTIO GPU blob objects
under TCG in certain conditions.

The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
MemoryRegion and attaches it to an offset on a PCI BAR of the
VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
it.

Because virglrenderer commands are not thread-safe they are only
called on the main context and QEMU performs the cleanup in three steps
to prevent a use-after-free scenario where the guest can access the
region after it’s unmapped:

1. From the main context, the region’s field finish_unmapping is false
   by default, so it sets a variable cmd_suspended, increases the
   renderer_blocked variable, deletes the blob subregion, and unparents
   the blob subregion causing its reference count to decrement.

2. From an RCU context, the MemoryView gets freed, the FlatView gets
   recalculated, the free callback of the blob region
   virtio_gpu_virgl_hostmem_region_free is called which sets the
   region’s field finish_unmapping to true, allowing the main thread
   context to finish replying to the command

3. From the main context, the command is processed again, but this time
   finish_unmapping is true, so virgl_renderer_resource_unmap can be
   called and a response is sent to the guest.

It happens so that under TCG, if the guest has no timers configured (and
thus no interrupt will cause the CPU to exit), the RCU thread does not
have enough time to grab the locks and recalculate the FlatView.

That’s not a big problem in practice since most guests will assume a
response will happen later in time and go on to do different things,
potentially triggering interrupts and allowing the RCU context to run.
If the guest waits for the unmap command to complete though, it blocks
indefinitely. Attaching to the QEMU monitor and force quitting the guest
allows the cleanup to continue.

There's no reason why the FlatView recalculation can't occur right away
when we delete the blob subregion, however. It does not, because when we
create the subregion we set the object as its own parent:

    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);

The extra reference is what prevents freeing the memory region object in
the memory transaction of deleting the subregion.

This commit changes the owner object to the device, which removes the
extra owner reference in the memory region and causes the MR to be
freed right away in the main context.

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
Cc: qemu-stable@nongnu.org
---
 hw/display/virtio-gpu-virgl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 71a7500de9..8fbe4e70cc 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
     vmr->g = g;
     mr = g_new0(MemoryRegion, 1);
 
-    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
+    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
     memory_region_add_subregion(&b->hostmem, offset, mr);
     memory_region_set_enabled(mr, true);
 
-- 
2.39.5

[PATCH v2 11/14] 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 8fbe4e70cc..32a32879f7 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.39.5

[PATCH v2 12/14] 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 2c9a0db425..9f7dc697f2 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -129,7 +129,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.39.5

Re: [PATCH v2 12/14] ui/gtk-gl-area: Remove extra draw call in refresh

[Akihiko Odaki]

On 2025/05/06 21:57, Alex Bennée wrote:
> 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.

Simply reverting the part of the commit may re-introduce the problem the 
commit tried to solve, which will be a regression as the commit is 
already included in releases.

I guess the problem is that the gl_block callback of GraphicHwOps is not 
properly implemented and it is what should be fixed.

> 
> 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 2c9a0db425..9f7dc697f2 100644
> --- a/ui/gtk-gl-area.c
> +++ b/ui/gtk-gl-area.c
> @@ -129,7 +129,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;
>       }
>   

Re: [PATCH v2 12/14] ui/gtk-gl-area: Remove extra draw call in refresh

[Dmitry Osipenko]

On 5/10/25 07:52, Akihiko Odaki wrote:
> On 2025/05/06 21:57, Alex Bennée wrote:
>> 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.
> 
> Simply reverting the part of the commit may re-introduce the problem the
> commit tried to solve, which will be a regression as the commit is
> already included in releases.
> 
> I guess the problem is that the gl_block callback of GraphicHwOps is not
> properly implemented and it is what should be fixed.

The reverted commit made QEMU GTK GUI unusable under Wayland. It was
fixing problem which requires very specific QEMU setup, while breaking
generic setups. The offending change should be reverted as it introduced
a bigger problem. A proper solution should be found, meanwhile QEMU GTK
under Wayland should be restored, IMO.

For the reference see [1]. First bug reports about a mirrored display
problem were made to me on IRC a year ago and the root of the problem
was identified only couple months ago.

[1]
https://lore.kernel.org/qemu-devel/5aedf1ad-d9b0-4edb-a050-f3d9bee9bccb@collabora.com/

As of today, the GTK problem isn't understood.

-- 
Best regards,
Dmitry

Re: [PATCH v2 12/14] ui/gtk-gl-area: Remove extra draw call in refresh

[Akihiko Odaki]

On 2025/05/10 21:12, Dmitry Osipenko wrote:
> On 5/10/25 07:52, Akihiko Odaki wrote:
>> On 2025/05/06 21:57, Alex Bennée wrote:
>>> 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.
>>
>> Simply reverting the part of the commit may re-introduce the problem the
>> commit tried to solve, which will be a regression as the commit is
>> already included in releases.
>>
>> I guess the problem is that the gl_block callback of GraphicHwOps is not
>> properly implemented and it is what should be fixed.
> 
> The reverted commit made QEMU GTK GUI unusable under Wayland. It was
> fixing problem which requires very specific QEMU setup, while breaking
> generic setups. The offending change should be reverted as it introduced
> a bigger problem. A proper solution should be found, meanwhile QEMU GTK
> under Wayland should be restored, IMO.
> 
> For the reference see [1]. First bug reports about a mirrored display
> problem were made to me on IRC a year ago and the root of the problem
> was identified only couple months ago.
> 
> [1]
> https://lore.kernel.org/qemu-devel/5aedf1ad-d9b0-4edb-a050-f3d9bee9bccb@collabora.com/

That describes the context well. It should also be described with an 
inline comment so that we will not lose track of the known problem.

> 
> As of today, the GTK problem isn't understood.
> 

If your guess about simultaneous accesses on the guest frame buffer is 
correct, it's a bug of the emulated device, not GTK.

Regards,
Akihiko Odaki

RE: [PATCH v2 12/14] ui/gtk-gl-area: Remove extra draw call in refresh

[Kim, Dongwon]

> Subject: Re: [PATCH v2 12/14] ui/gtk-gl-area: Remove extra draw call in refresh
> 
> On 2025/05/10 21:12, Dmitry Osipenko wrote:
> > On 5/10/25 07:52, Akihiko Odaki wrote:
> >> On 2025/05/06 21:57, Alex Bennée wrote:
> >>> 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.
> >>
> >> Simply reverting the part of the commit may re-introduce the problem
> >> the commit tried to solve, which will be a regression as the commit
> >> is already included in releases.
> >>
> >> I guess the problem is that the gl_block callback of GraphicHwOps is
> >> not properly implemented and it is what should be fixed.
> >
> > The reverted commit made QEMU GTK GUI unusable under Wayland. It was
> > fixing problem which requires very specific QEMU setup, while breaking
> > generic setups. The offending change should be reverted as it
> > introduced a bigger problem. A proper solution should be found,
> > meanwhile QEMU GTK under Wayland should be restored, IMO.
> >
> > For the reference see [1]. First bug reports about a mirrored display
> > problem were made to me on IRC a year ago and the root of the problem
> > was identified only couple months ago.
> >
> > [1]
> > https://lore.kernel.org/qemu-devel/5aedf1ad-d9b0-4edb-a050-f3d9bee9bcc
> > b@collabora.com/
> 
> That describes the context well. It should also be described with an inline
> comment so that we will not lose track of the known problem.

[Kim, Dongwon]  Do you mean we should have this info in the commit message?

> 
> >
> > As of today, the GTK problem isn't understood.
> >
> 
> If your guess about simultaneous accesses on the guest frame buffer is correct,
> it's a bug of the emulated device, not GTK.

[Kim, Dongwon]  "Simultaneous accesses" was just a guess. This problem needs to be debugged further.
BTW, the problem the original commit was trying to fix is some lockup due to suspended rendering on
minimized or hidden GTK window. But according to Dmitry, this case wasn't hit in his setup with wayland
compositor so I assume the risk of not having an extra draw in gtk-gl-area would be low.

> 
> Regards,
> Akihiko Odaki

Re: [PATCH v2 12/14] ui/gtk-gl-area: Remove extra draw call in refresh

[Akihiko Odaki]

On 2025/05/13 2:01, Kim, Dongwon wrote:
>> Subject: Re: [PATCH v2 12/14] ui/gtk-gl-area: Remove extra draw call in refresh
>>
>> On 2025/05/10 21:12, Dmitry Osipenko wrote:
>>> On 5/10/25 07:52, Akihiko Odaki wrote:
>>>> On 2025/05/06 21:57, Alex Bennée wrote:
>>>>> 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.
>>>>
>>>> Simply reverting the part of the commit may re-introduce the problem
>>>> the commit tried to solve, which will be a regression as the commit
>>>> is already included in releases.
>>>>
>>>> I guess the problem is that the gl_block callback of GraphicHwOps is
>>>> not properly implemented and it is what should be fixed.
>>>
>>> The reverted commit made QEMU GTK GUI unusable under Wayland. It was
>>> fixing problem which requires very specific QEMU setup, while breaking
>>> generic setups. The offending change should be reverted as it
>>> introduced a bigger problem. A proper solution should be found,
>>> meanwhile QEMU GTK under Wayland should be restored, IMO.
>>>
>>> For the reference see [1]. First bug reports about a mirrored display
>>> problem were made to me on IRC a year ago and the root of the problem
>>> was identified only couple months ago.
>>>
>>> [1]
>>> https://lore.kernel.org/qemu-devel/5aedf1ad-d9b0-4edb-a050-f3d9bee9bcc
>>> b@collabora.com/
>>
>> That describes the context well. It should also be described with an inline
>> comment so that we will not lose track of the known problem.
> 
> [Kim, Dongwon]  Do you mean we should have this info in the commit message?

A comment in the code is better.

A description of the current version of the code should be left as a 
comment so that readers can find it without git blame.

The commit message is suited for a description of the old version of 
code (e.g., a bug fixed by the commit) or a comparison between the old 
and new version; leaving them as comments will infinitely increase the 
amount of the code.

In this case, it is about a problem with the current version so it 
should be written as a comment.

> 
>>
>>>
>>> As of today, the GTK problem isn't understood.
>>>
>>
>> If your guess about simultaneous accesses on the guest frame buffer is correct,
>> it's a bug of the emulated device, not GTK.
> 
> [Kim, Dongwon]  "Simultaneous accesses" was just a guess. This problem needs to be debugged further.
> BTW, the problem the original commit was trying to fix is some lockup due to suspended rendering on
> minimized or hidden GTK window. But according to Dmitry, this case wasn't hit in his setup with wayland
> compositor so I assume the risk of not having an extra draw in gtk-gl-area would be low.

It is better to describe the estimation of the risk in the comment too.

Regards,
Akihiko Odaki

[PATCH v2 13/14] docs: Create a uniquelabel Sphinx extension

[Alex Bennée]

From: Peter Maydell <peter.maydell@linaro.org>

Sphinx requires that labels within documents are unique across the
whole manual.  This is because the "create a hyperlink" directive
specifies only the name of the label, not a filename+label.  Some
Sphinx versions will warn about duplicate labels, but even if there
is no warning there is still an ambiguity and no guarantee that the
hyperlink will be created to the right target.

For QEMU this is awkward, because we have various .rst.inc fragments
which we include into multiple .rst files.  If you define a label in
the .rst.inc file then it will be a duplicate label.  We have mostly
worked around this by not putting labels into those .rst.inc files,
or by adding "insert a label" functionality into the hxtool extension
(see commit 1eeb432a953b0 "doc/sphinx/hxtool.py: add optional label
argument to SRST directive").

This Sphinx extension adds a "uniquelabel" directive, which creates a
label which is made unique by adding the name of the document to the
label.  The name of the generated label is "dir/file-labelname"; this
is patterned on the generated label names that the hxtool SRST(label)
directive creates.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20250429163212.618953-2-peter.maydell@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/conf.py               |  1 +
 docs/sphinx/uniquelabel.py | 74 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 docs/sphinx/uniquelabel.py

diff --git a/docs/conf.py b/docs/conf.py
index 7b5712e122..562db95bbf 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -67,6 +67,7 @@
     'qapi_domain',
     'qapidoc',
     'qmp_lexer',
+    'uniquelabel',
 ]
 
 if sphinx.version_info[:3] > (4, 0, 0):
diff --git a/docs/sphinx/uniquelabel.py b/docs/sphinx/uniquelabel.py
new file mode 100644
index 0000000000..cc4ab7f31c
--- /dev/null
+++ b/docs/sphinx/uniquelabel.py
@@ -0,0 +1,74 @@
+# coding=utf-8
+#
+# Copyright (c) 2025 Linaro
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Sphinx extension to create a unique label by concatenating
+# the name of the origin document with the label text.
+#
+# Sphinx requires that labels within documents are unique across
+# the whole manual. This is because the "create a hyperlink" directive
+# specifies only the name of the label, not a filename+label.
+# Some Sphinx versions will warn about duplicate labels, but
+# even if there is no warning there is still an ambiguity and no
+# guarantee that the hyperlink will be created to the right target.
+#
+# For QEMU this is awkward, because we have various .rst.inc fragments
+# which we include into multiple .rst files. If you define a label in
+# the .rst.inc file then it will be a duplicate label.
+#
+# The uniquelabel directive is our fix for this: it creates a label
+# whose name includes the name of the top level .rst file. This is then
+# unique even if the .rst.inc file is included in multiple places, and
+# when we create a hyperlink we can explicitly specify which label we
+# are targeting.
+#
+# Concretely, if you have a foo/bar.rst and a foo/baz.rst that
+# both include wat.rst.inc, then in wat.rst.inc you can write
+# .. uniquelabel:: mylabel
+# and it will be as if you had written a reference label:
+# .. _foo/bar-mylabel
+# or
+# .. _foo/baz-mylabel
+# depending on which file included wat.rst.inc, and you can link to
+# whichever one you intend via any of the usual markup, e.g.
+# `documentation of the thing in bar <foo/bar-mylabel>`.
+
+"""uniquelabel is a Sphinx extension that implements the uniquelabel directive"""
+
+from docutils import nodes
+from docutils.statemachine import ViewList
+from docutils.parsers.rst import directives, Directive
+import sphinx
+
+__version__ = '1.0'
+
+class UniqueLabelDocDirective(Directive):
+    """Create a unique label by including the docname"""
+    required_arguments = 1
+    optional_arguments = 0
+    has_content = False
+
+    def run(self):
+        env = self.state.document.settings.env
+        label = self.arguments[0]
+
+        refline = ".. _" + env.docname + "-" + label + ":"
+
+        rstlist = ViewList()
+        rstlist.append(refline, "generated text", 0)
+
+        node = nodes.paragraph()
+        self.state.nested_parse(rstlist, 0, node)
+        return node.children
+
+def setup(app):
+    """ Register uniquelabel directive with Sphinx"""
+    app.add_directive('uniquelabel', UniqueLabelDocDirective)
+
+    return dict(
+        version = __version__,
+        parallel_read_safe = True,
+        parallel_write_safe = True
+    )
-- 
2.39.5

[PATCH v2 14/14] docs: Use uniquelabel in qemu-block-drivers.rst.inc

[Alex Bennée]

From: Peter Maydell <peter.maydell@linaro.org>

Use the uniquelabel directive for the 'nbd' label in
qemu-block-drivers.rst.inc.  This avoids a complaint from some Sphinx
versions about it being a duplicate label, and means that we can
reliably ensure that the link in codebase.rst goes to the document
file that we intend (i.e.  the section of the HTML manual on disk
images, not the HTML copy of the qemu-block-drivers manpage).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20250429163212.618953-3-peter.maydell@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/codebase.rst                | 2 +-
 docs/system/qemu-block-drivers.rst.inc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/devel/codebase.rst b/docs/devel/codebase.rst
index 40273e7d31..f3f133e9ad 100644
--- a/docs/devel/codebase.rst
+++ b/docs/devel/codebase.rst
@@ -116,7 +116,7 @@ yet, so sometimes the source code is all you have.
 * `monitor <https://gitlab.com/qemu-project/qemu/-/tree/master/monitor>`_:
   `Monitor <QEMU monitor>` implementation (HMP & QMP).
 * `nbd <https://gitlab.com/qemu-project/qemu/-/tree/master/nbd>`_:
-  QEMU `NBD (Network Block Device) <nbd>` server.
+  QEMU `NBD (Network Block Device) <system/images-nbd>` server.
 * `net <https://gitlab.com/qemu-project/qemu/-/tree/master/net>`_:
   Network (host) support.
 * `pc-bios <https://gitlab.com/qemu-project/qemu/-/tree/master/pc-bios>`_:
diff --git a/docs/system/qemu-block-drivers.rst.inc b/docs/system/qemu-block-drivers.rst.inc
index cfe1acb78a..1a405e4782 100644
--- a/docs/system/qemu-block-drivers.rst.inc
+++ b/docs/system/qemu-block-drivers.rst.inc
@@ -500,7 +500,7 @@ What you should *never* do:
 - expect it to work when loadvm'ing
 - write to the FAT directory on the host system while accessing it with the guest system
 
-.. _nbd:
+.. uniquelabel:: nbd
 
 NBD access
 ~~~~~~~~~~
-- 
2.39.5