[PATCH 0/9] Maintainer updates for May (testing, plugins, virtio-gpu)

[PATCH 0/9] 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.

The following need review:

  MAINTAINERS: add myself to virtio-gpu for Odd Fixes
  contrib/plugins: allow setting of instructions per quantum
  contrib/plugins: add a scaling factor to the ips arg
  tests/tcg: make aarch64 boot.S handle different starting modes
  gitlab: disable debug info on CI builds
  tests/docker: expose $HOME/.cache as docker volume

Alex.

Alex Bennée (7):
  tests/docker: expose $HOME/.cache 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
  hw/display: re-arrange memory region tracking

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

 MAINTAINERS                               |   3 +-
 docs/about/emulation.rst                  |   4 +
 include/system/memory.h                   |   1 +
 contrib/plugins/ips.c                     |  33 +++++-
 hw/display/virtio-gpu-virgl.c             |  60 +++++-----
 .gitlab-ci.d/buildtest-template.yml       |   1 +
 tests/docker/Makefile.include             |   1 +
 tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
 tests/tcg/aarch64/system/boot.S           | 127 +++++++++++++++++++++-
 9 files changed, 202 insertions(+), 31 deletions(-)

-- 
2.39.5

[PATCH 1/9] tests/docker: expose $HOME/.cache as docker volume

[Alex Bennée]

If you want to run functional tests we should share .cache so we don't
force containers to continually re-download images.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/Makefile.include | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index fa1cbb6726..56a8d9f8ff 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -224,6 +224,7 @@ docker-run: docker-qemu-src
 			$(if $(NOUSER),,				\
 				-e CCACHE_DIR=/var/tmp/ccache 		\
 				-v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \
+				-v $(HOME)/.cache:$(HOME)/.cache 	\
 			)						\
 			-v $$(readlink -e $(DOCKER_SRC_COPY)):/var/tmp/qemu:z$(COMMA)ro \
 			$(IMAGE) 					\
-- 
2.39.5

Re: [PATCH 1/9] tests/docker: expose $HOME/.cache as docker volume

[Thomas Huth]

On 28/04/2025 14.59, Alex Bennée wrote:
> If you want to run functional tests we should share .cache so we don't
> force containers to continually re-download images.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/docker/Makefile.include | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index fa1cbb6726..56a8d9f8ff 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -224,6 +224,7 @@ docker-run: docker-qemu-src
>   			$(if $(NOUSER),,				\
>   				-e CCACHE_DIR=/var/tmp/ccache 		\
>   				-v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \
> +				-v $(HOME)/.cache:$(HOME)/.cache 	\

Maybe better restrict it to ~/.cache/qemu ? ... to avoid sharing other 
subfolders there to the container?

  Thomas

[PATCH 2/9] 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>
---
 .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

Re: [PATCH 2/9] gitlab: disable debug info on CI builds

[Thomas Huth]

On 28/04/2025 14.59, Alex Bennée wrote:
> 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>
> ---
>   .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

Do we have any jobs that might show stack traces in the console output ? 
build-oss-fuzz comes to my mind, but that uses a separate script, so we 
should be fine there?

So assuming there are no other jobs around that might need it:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/9] gitlab: disable debug info on CI builds

[Peter Maydell]

On Mon, 28 Apr 2025 at 15:02, Thomas Huth <thuth@redhat.com> wrote:
>
> On 28/04/2025 14.59, Alex Bennée wrote:
> > 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>
> > ---
> >   .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
>
> Do we have any jobs that might show stack traces in the console output ?
> build-oss-fuzz comes to my mind, but that uses a separate script, so we
> should be fine there?

If you build with Rust enabled, and the Rust code panics,
then you get a Rust backtrace. But I don't know if that
cares about debug info to get its backtraces: quite
possibly it's an entirely different mechanism.

-- PMM

Re: [PATCH 2/9] gitlab: disable debug info on CI builds

[Manos Pitsidianakis]

On Mon, 28 Apr 2025 17:20, Peter Maydell <peter.maydell@linaro.org> wrote:
>On Mon, 28 Apr 2025 at 15:02, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 28/04/2025 14.59, Alex Bennée wrote:
>> > 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>
>> > ---
>> >   .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
>>
>> Do we have any jobs that might show stack traces in the console output ?
>> build-oss-fuzz comes to my mind, but that uses a separate script, so we
>> should be fine there?
>
>If you build with Rust enabled, and the Rust code panics,
>then you get a Rust backtrace. But I don't know if that
>cares about debug info to get its backtraces: quite
>possibly it's an entirely different mechanism.

It will show addresses and <unknown> instead of symbols if they are 
stripped.

[PATCH 3/9] 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
---
 tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
 tests/tcg/aarch64/system/boot.S           | 127 +++++++++++++++++++++-
 2 files changed, 128 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..19b18bdeec 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,129 @@ 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, 1f
+	sub 	w11, w11, #'0'
+1:
+
+	/* 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 +220,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 +353,11 @@ __sys_outc:
 	ret
 
 	.data
+
+	.align 8
+cmdline:
+	.space 128, 0
+
 	.align	12
 
 	/* Translation table
-- 
2.39.5

Re: [PATCH 3/9] tests/tcg: make aarch64 boot.S handle different starting modes

[Manos Pitsidianakis]

On Mon, 28 Apr 2025 15:59, Alex Bennée <alex.bennee@linaro.org> 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
>---
> tests/tcg/aarch64/Makefile.softmmu-target |   3 +-
> tests/tcg/aarch64/system/boot.S           | 127 +++++++++++++++++++++-
> 2 files changed, 128 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


The insertions in boot.S have mixed spaces/tabs


>diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
>index a5df9c173d..19b18bdeec 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,129 @@ 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, 1f

Suggested sanity check:

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

>+	sub 	w11, w11, #'0'
>+1:
>+
>+	/* 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 +220,7 @@ __start:
> 	 * maps RAM to the first Gb. The stage2 tables have two 2mb
> 	 * translation block entries covering a series of adjacent
> 	 * 4k pages.
>-	*/
>+ 	 */

Accidental space

> 
> 	/* Stage 1 entry: indexed by IA[38:30] */
> 	adr	x1, .				/* phys address */
>@@ -233,6 +353,11 @@ __sys_outc:
> 	ret
> 
> 	.data
>+
>+	.align 8
>+cmdline:
>+	.space 128, 0
>+
> 	.align	12
> 
> 	/* Translation table
>-- 
>2.39.5

Otherwise,

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

[PATCH 4/9] 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>
---
 contrib/plugins/ips.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
index e5297dbb01..1952e0866d 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,19 @@ 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;
             }
+            for (int j = 0; j < ARRAY_SIZE(scales); j++) {
+                if (g_str_has_suffix(endptr, scales[j].suffix)) {
+                        max_insn_per_second *= scales[j].multipler;
+                        break;
+                }
+            }
         } else {
             fprintf(stderr, "option parsing failed: %s\n", opt);
             return -1;
-- 
2.39.5

Re: [PATCH 4/9] contrib/plugins: add a scaling factor to the ips arg

[Pierrick Bouvier]

On 4/28/25 5:59 AM, 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>
> ---
>   contrib/plugins/ips.c | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
> index e5297dbb01..1952e0866d 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,19 @@ 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;
>               }
> +            for (int j = 0; j < ARRAY_SIZE(scales); j++) {
> +                if (g_str_has_suffix(endptr, scales[j].suffix)) {
> +                        max_insn_per_second *= scales[j].multipler;
> +                        break;
> +                }

Could we add an error in case the present suffix is not recognized, with 
the list of available ones?
As well, can lower the suffix string, so people are free to use any case.

> +            }
>           } else {
>               fprintf(stderr, "option parsing failed: %s\n", opt);
>               return -1;

With the remark above,

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH 5/9] 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>
---
 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..b607183d13 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 1952e0866d..d83124d71c 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);
@@ -164,6 +166,9 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
                         break;
                 }
             }
+        } 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;
@@ -171,7 +176,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 5/9] contrib/plugins: allow setting of instructions per quantum

[Pierrick Bouvier]

On 4/28/25 5:59 AM, 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>
> ---
>   docs/about/emulation.rst |  4 ++++
>   contrib/plugins/ips.c    | 10 +++++++++-
>   2 files changed, 13 insertions(+), 1 deletion(-)
>

Good idea!

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

[PATCH 6/9] 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's 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>
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

Re: [PATCH 6/9] MAINTAINERS: add myself to virtio-gpu for Odd Fixes

[Thomas Huth]

On 28/04/2025 14.59, Alex Bennée wrote:
> 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's 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>
> ---
>   MAINTAINERS | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 661a47db5a..f67c8edcf6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2636,7 +2636,8 @@ F: hw/display/ramfb*.c
>   F: include/hw/display/ramfb.h
>   
>   virtio-gpu
> -S: Orphan
> +M: Alex Bennée <alex.bennee@linaro.org>
> +M: Odd Fixes
>   F: hw/display/virtio-gpu*
>   F: hw/display/virtio-vga.*
>   F: include/hw/virtio/virtio-gpu.h

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 6/9] MAINTAINERS: add myself to virtio-gpu for Odd Fixes

[Dmitry Osipenko]

On 4/28/25 15:59, Alex Bennée wrote:
> 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's 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>
> ---
>  MAINTAINERS | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 661a47db5a..f67c8edcf6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2636,7 +2636,8 @@ F: hw/display/ramfb*.c
>  F: include/hw/display/ramfb.h
>  
>  virtio-gpu
> -S: Orphan
> +M: Alex Bennée <alex.bennee@linaro.org>
> +M: Odd Fixes
>  F: hw/display/virtio-gpu*
>  F: hw/display/virtio-vga.*
>  F: include/hw/virtio/virtio-gpu.h

Thanks a lot for stepping up!

This reminded me that I wanted to propose myself as reviewer for the
virtio-gpu patches. Will do it soon.

Akihiko Odaki is also good at reviewing virtio-gpu patches. Wondering if
Akihiko would want to be added as reviewer or co-maintainer of virtio-gpu?

-- 
Best regards,
Dmitry

Re: [PATCH 6/9] MAINTAINERS: add myself to virtio-gpu for Odd Fixes

[Akihiko Odaki]

On 2025/04/30 3:56, Dmitry Osipenko wrote:
> On 4/28/25 15:59, Alex Bennée wrote:
>> 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's 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>
>> ---
>>   MAINTAINERS | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 661a47db5a..f67c8edcf6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2636,7 +2636,8 @@ F: hw/display/ramfb*.c
>>   F: include/hw/display/ramfb.h
>>   
>>   virtio-gpu
>> -S: Orphan
>> +M: Alex Bennée <alex.bennee@linaro.org>
>> +M: Odd Fixes
>>   F: hw/display/virtio-gpu*
>>   F: hw/display/virtio-vga.*
>>   F: include/hw/virtio/virtio-gpu.h
> 
> Thanks a lot for stepping up!
> 
> This reminded me that I wanted to propose myself as reviewer for the
> virtio-gpu patches. Will do it soon.

Thank both of you for stepping up.

> 
> Akihiko Odaki is also good at reviewing virtio-gpu patches. Wondering if
> Akihiko would want to be added as reviewer or co-maintainer of virtio-gpu?
> 

Yes, please add me as a reviewer.

I guess it would be better if Alex add you and me with the next version 
of this patch or add follow-up patches to this to avoid change conflicts.

Re: [PATCH 6/9] MAINTAINERS: add myself to virtio-gpu for Odd Fixes

[Dmitry Osipenko]

On 5/4/25 10:20, Akihiko Odaki wrote:
> On 2025/04/30 3:56, Dmitry Osipenko wrote:
>> On 4/28/25 15:59, Alex Bennée wrote:
>>> 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's 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>
>>> ---
>>>   MAINTAINERS | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 661a47db5a..f67c8edcf6 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2636,7 +2636,8 @@ F: hw/display/ramfb*.c
>>>   F: include/hw/display/ramfb.h
>>>     virtio-gpu
>>> -S: Orphan
>>> +M: Alex Bennée <alex.bennee@linaro.org>
>>> +M: Odd Fixes
>>>   F: hw/display/virtio-gpu*
>>>   F: hw/display/virtio-vga.*
>>>   F: include/hw/virtio/virtio-gpu.h
>>
>> Thanks a lot for stepping up!
>>
>> This reminded me that I wanted to propose myself as reviewer for the
>> virtio-gpu patches. Will do it soon.
> 
> Thank both of you for stepping up.
> 
>>
>> Akihiko Odaki is also good at reviewing virtio-gpu patches. Wondering if
>> Akihiko would want to be added as reviewer or co-maintainer of virtio-
>> gpu?
>>
> 
> Yes, please add me as a reviewer.
> 
> I guess it would be better if Alex add you and me with the next version
> of this patch or add follow-up patches to this to avoid change conflicts.

+1 Feel free to add us as reviewers in v2 of this patch. Otherwise I'll
send the separate maintainers-update patch along with nctx v12 patches.

-- 
Best regards,
Dmitry

Re: [PATCH 6/9] MAINTAINERS: add myself to virtio-gpu for Odd Fixes

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 5/4/25 10:20, Akihiko Odaki wrote:
>> On 2025/04/30 3:56, Dmitry Osipenko wrote:
>>> On 4/28/25 15:59, Alex Bennée wrote:
>>>> 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's 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>
>>>> ---
>>>>   MAINTAINERS | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 661a47db5a..f67c8edcf6 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -2636,7 +2636,8 @@ F: hw/display/ramfb*.c
>>>>   F: include/hw/display/ramfb.h
>>>>     virtio-gpu
>>>> -S: Orphan
>>>> +M: Alex Bennée <alex.bennee@linaro.org>
>>>> +M: Odd Fixes
>>>>   F: hw/display/virtio-gpu*
>>>>   F: hw/display/virtio-vga.*
>>>>   F: include/hw/virtio/virtio-gpu.h
>>>
>>> Thanks a lot for stepping up!
>>>
>>> This reminded me that I wanted to propose myself as reviewer for the
>>> virtio-gpu patches. Will do it soon.
>> 
>> Thank both of you for stepping up.
>> 
>>>
>>> Akihiko Odaki is also good at reviewing virtio-gpu patches. Wondering if
>>> Akihiko would want to be added as reviewer or co-maintainer of virtio-
>>> gpu?
>>>
>> 
>> Yes, please add me as a reviewer.
>> 
>> I guess it would be better if Alex add you and me with the next version
>> of this patch or add follow-up patches to this to avoid change conflicts.
>
> +1 Feel free to add us as reviewers in v2 of this patch. Otherwise I'll
> send the separate maintainers-update patch along with nctx v12
> patches.

I shall add you both ;-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 7/9] 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 8/9] 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

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Dmitry Osipenko]

On 4/28/25 15:59, Alex Bennée wrote:
> 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);
>  

This change makes QEMU to crash.

AFAICT, it effectively reverts code to old bugged version [1] that was
rejected in the past.

+Akihiko Odaki

[1]
https://lore.kernel.org/qemu-devel/20230915111130.24064-10-ray.huang@amd.com/


-- 
Best regards,
Dmitry

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 4/28/25 15:59, Alex Bennée wrote:
>> 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);
>>  
>
> This change makes QEMU to crash.

What is your command line to cause the crash?

>
> AFAICT, it effectively reverts code to old bugged version [1] that was
> rejected in the past.
>
> +Akihiko Odaki
>
> [1]
> https://lore.kernel.org/qemu-devel/20230915111130.24064-10-ray.huang@amd.com/

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Dmitry Osipenko]

On 4/30/25 00:19, Alex Bennée wrote:
>> This change makes QEMU to crash.
> What is your command line to cause the crash?

I applied this patch on top of native context v11, ran AMD nctx and got a crash on SDDM startup.

(gdb) bt
#0  0x00007ffff5411b54 in __pthread_kill_implementation () at /lib64/libc.so.6
#1  0x00007ffff53b8f9e in raise () at /lib64/libc.so.6
#2  0x00007ffff53a0942 in abort () at /lib64/libc.so.6
#3  0x00007ffff6cbf18c in g_assertion_message[cold] () at /lib64/libglib-2.0.so.0
#4  0x00007ffff6d2ea07 in g_assertion_message_expr () at /lib64/libglib-2.0.so.0
#5  0x0000555555a42820 in object_finalize (data=0x555557c9d290) at ../qom/object.c:732
#6  object_unref (objptr=0x555557c9d290) at ../qom/object.c:1231
#7  0x00005555559f3df3 in memory_region_unref (mr=<optimized out>) at ../system/memory.c:1854
#8  0x0000555555a003a7 in phys_section_destroy (mr=0x555559ef5b60) at ../system/physmem.c:1035
#9  phys_sections_free (map=0x555559c2dd80) at ../system/physmem.c:1048
#10 address_space_dispatch_free (d=0x555559c2dd70) at ../system/physmem.c:2692
#11 0x00005555559f1d33 in flatview_destroy (view=0x55555a54a720) at ../system/memory.c:295
#12 0x0000555555c278cf in call_rcu_thread (opaque=opaque@entry=0x0) at ../util/rcu.c:301
#13 0x0000555555c1cc68 in qemu_thread_start (args=0x555557993d30) at ../util/qemu-thread-posix.c:541
#14 0x00007ffff540fba8 in start_thread () at /lib64/libc.so.6
#15 0x00007ffff5493b8c in __clone3 () at /lib64/libc.so.6

-- 
Best regards,
Dmitry

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Alex Bennée]

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> On 4/30/25 00:19, Alex Bennée wrote:
>>> This change makes QEMU to crash.
>> What is your command line to cause the crash?
>
> I applied this patch on top of native context v11, ran AMD nctx and
> got a crash on SDDM startup.

Did you also include the pre-cursor patch which splits MemoryRegion out
of the container struct. The aim here is to allow MemoryRegion counting
to be handled without worrying about other structure lifetimes.

>
> (gdb) bt
> #0  0x00007ffff5411b54 in __pthread_kill_implementation () at /lib64/libc.so.6
> #1  0x00007ffff53b8f9e in raise () at /lib64/libc.so.6
> #2  0x00007ffff53a0942 in abort () at /lib64/libc.so.6
> #3  0x00007ffff6cbf18c in g_assertion_message[cold] () at /lib64/libglib-2.0.so.0
> #4  0x00007ffff6d2ea07 in g_assertion_message_expr () at /lib64/libglib-2.0.so.0
> #5  0x0000555555a42820 in object_finalize (data=0x555557c9d290) at ../qom/object.c:732
> #6  object_unref (objptr=0x555557c9d290) at ../qom/object.c:1231
> #7  0x00005555559f3df3 in memory_region_unref (mr=<optimized out>) at ../system/memory.c:1854
> #8  0x0000555555a003a7 in phys_section_destroy (mr=0x555559ef5b60) at ../system/physmem.c:1035
> #9  phys_sections_free (map=0x555559c2dd80) at ../system/physmem.c:1048
> #10 address_space_dispatch_free (d=0x555559c2dd70) at ../system/physmem.c:2692
> #11 0x00005555559f1d33 in flatview_destroy (view=0x55555a54a720) at ../system/memory.c:295
> #12 0x0000555555c278cf in call_rcu_thread (opaque=opaque@entry=0x0) at ../util/rcu.c:301
> #13 0x0000555555c1cc68 in qemu_thread_start (args=0x555557993d30) at ../util/qemu-thread-posix.c:541
> #14 0x00007ffff540fba8 in start_thread () at /lib64/libc.so.6
> #15 0x00007ffff5493b8c in __clone3 () at /lib64/libc.so.6

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Dmitry Osipenko]

On 4/30/25 13:24, Alex Bennée wrote:
>> On 4/30/25 00:19, Alex Bennée wrote:
>>>> This change makes QEMU to crash.
>>> What is your command line to cause the crash?
>> I applied this patch on top of native context v11, ran AMD nctx and
>> got a crash on SDDM startup.
> Did you also include the pre-cursor patch which splits MemoryRegion out
> of the container struct. The aim here is to allow MemoryRegion counting
> to be handled without worrying about other structure lifetimes.

Very good catch, I indeed missed that other patch. Reapplied all the
patches and QEMU doesn't crash anymore. Now the code changes look sane
to me.

Will be great if Akihiko Odaki could comment too.

-- 
Best regards,
Dmitry

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Akihiko Odaki]

On 2025/04/30 3:48, Dmitry Osipenko wrote:
> On 4/28/25 15:59, Alex Bennée wrote:
>> 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.

The RCU thread should be free to kick in if the guest is waiting and 
idle. That may be a problem that should be analyzed and fixed.

>>
>> 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);
>>   
> 
> This change makes QEMU to crash.
> 
> AFAICT, it effectively reverts code to old bugged version [1] that was
> rejected in the past.
> 
> +Akihiko Odaki
> 
> [1]
> https://lore.kernel.org/qemu-devel/20230915111130.24064-10-ray.huang@amd.com/

I think you are right.

Changing the owner indeed makes the MR to be freed right away, but this 
is because it makes a reference by the FlatView to the MR dangling and 
can lead to use-after-free.

Setting the owner to the device implies that the device keeps the 
storage of the MR and. The references to the MR must be counted by the 
device to keep the storage available in such a case.

The MR itself doesn't count the references. It is fine as long as the MR 
is kept alive by being parented by the device, but 
virtio_gpu_virgl_unmap_resource_blob() unparents the MR and breaks the 
assumption. That's why the device shouldn't be the the owner of the MR.

Regards,
Akihiko Odaki

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Alex Bennée]

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

> On 2025/04/30 3:48, Dmitry Osipenko wrote:
>> On 4/28/25 15:59, Alex Bennée wrote:
>>> 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.
>
> The RCU thread should be free to kick in if the guest is waiting and
> idle. That may be a problem that should be analyzed and fixed.
>
>>>
>>> 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);
>>>   
>> This change makes QEMU to crash.
>> AFAICT, it effectively reverts code to old bugged version [1] that
>> was
>> rejected in the past.
>> +Akihiko Odaki
>> [1]
>> https://lore.kernel.org/qemu-devel/20230915111130.24064-10-ray.huang@amd.com/
>
> I think you are right.
>
> Changing the owner indeed makes the MR to be freed right away, but
> this is because it makes a reference by the FlatView to the MR
> dangling and can lead to use-after-free.
>
> Setting the owner to the device implies that the device keeps the
> storage of the MR and. The references to the MR must be counted by the
> device to keep the storage available in such a case.

Well the lifetime of the region is effectively controlled by the virgl
resource manager. There is a disconnect between when we can finally free
the MR and when the virglrenderer is done with it.

> The MR itself doesn't count the references. It is fine as long as the
> MR is kept alive by being parented by the device, but
> virtio_gpu_virgl_unmap_resource_blob() unparents the MR and breaks the
> assumption. That's why the device shouldn't be the the owner of the
> MR.

Then who should? Ultimately some object needs to "own" the MR until we
know it can be freed (so no longer mapped into the guest and no longer
referenced by virglrenderer). The normal MR refcounting isn't enough
because the lifetime extends beyond when it is "visible" in the FlatView.

>
> Regards,
> Akihiko Odaki

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 8/9] virtio-gpu: fix hang under TCG when unmapping blob

[Akihiko Odaki]

On 2025/05/06 19:12, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
> 
>> On 2025/04/30 3:48, Dmitry Osipenko wrote:
>>> On 4/28/25 15:59, Alex Bennée wrote:
>>>> 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.
>>
>> The RCU thread should be free to kick in if the guest is waiting and
>> idle. That may be a problem that should be analyzed and fixed.
>>
>>>>
>>>> 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);
>>>>    
>>> This change makes QEMU to crash.
>>> AFAICT, it effectively reverts code to old bugged version [1] that
>>> was
>>> rejected in the past.
>>> +Akihiko Odaki
>>> [1]
>>> https://lore.kernel.org/qemu-devel/20230915111130.24064-10-ray.huang@amd.com/
>>
>> I think you are right.
>>
>> Changing the owner indeed makes the MR to be freed right away, but
>> this is because it makes a reference by the FlatView to the MR
>> dangling and can lead to use-after-free.
>>
>> Setting the owner to the device implies that the device keeps the
>> storage of the MR and. The references to the MR must be counted by the
>> device to keep the storage available in such a case.
> 
> Well the lifetime of the region is effectively controlled by the virgl
> resource manager. There is a disconnect between when we can finally free
> the MR and when the virglrenderer is done with it.
> 
>> The MR itself doesn't count the references. It is fine as long as the
>> MR is kept alive by being parented by the device, but
>> virtio_gpu_virgl_unmap_resource_blob() unparents the MR and breaks the
>> assumption. That's why the device shouldn't be the the owner of the
>> MR.
> 
> Then who should? Ultimately some object needs to "own" the MR until we
> know it can be freed (so no longer mapped into the guest and no longer
> referenced by virglrenderer). The normal MR refcounting isn't enough
> because the lifetime extends beyond when it is "visible" in the FlatView.

The current code makes the MR "own" itself. This works because the 
references are kept counted by the MR's own reference counter just like 
a normal Object.

Regards,
Akihiko Odaki

[PATCH 9/9] 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