* Re: [PATCH] physmem: replace assertion with error
2025-02-17 12:08 [PATCH] physmem: replace assertion with error Paolo Bonzini
@ 2025-02-17 12:19 ` Daniel P. Berrangé
2025-02-17 12:28 ` Philippe Mathieu-Daudé
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2025-02-17 12:19 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Xiaoyao Li, qemu-stable
On Mon, Feb 17, 2025 at 01:08:12PM +0100, Paolo Bonzini wrote:
> It is possible to start QEMU with a confidential-guest-support object
> even in TCG mode. While there is already a check in qemu_machine_creation_done:
>
> if (machine->cgs && !machine->cgs->ready) {
> error_setg(errp, "accelerator does not support confidential guest %s",
> object_get_typename(OBJECT(machine->cgs)));
> exit(1);
> }
>
> the creation of RAMBlocks happens earlier, in qemu_init_board(), if
> the command line does not override the default memory backend with
> -M memdev. Then the RAMBlock will try to use guest_memfd (because
> machine_require_guest_memfd correctly returns true; at least correctly
> according to the current implementation) and trigger the assertion
> failure for kvm_enabled(). This happend with a command line as
> simple as the following:
>
> qemu-system-x86_64 -m 512 -nographic -object sev-snp-guest,reduced-phys-bits=48,id=sev0 \
> -M q35,kernel-irqchip=split,confidential-guest-support=sev0
> qemu-system-x86_64: ../system/physmem.c:1871: ram_block_add: Assertion `kvm_enabled()' failed.
>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> system/physmem.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] physmem: replace assertion with error
2025-02-17 12:08 [PATCH] physmem: replace assertion with error Paolo Bonzini
2025-02-17 12:19 ` Daniel P. Berrangé
@ 2025-02-17 12:28 ` Philippe Mathieu-Daudé
2025-02-17 13:15 ` Paolo Bonzini
2025-02-18 1:30 ` Xiaoyao Li
` (2 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-17 12:28 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Cc: Xiaoyao Li, qemu-stable, Daniel P. Berrangé, Marcelo Tosatti,
Brijesh Singh, Pankaj Gupta
Hi Paolo,
On 17/2/25 13:08, Paolo Bonzini wrote:
> It is possible to start QEMU with a confidential-guest-support object
> even in TCG mode. While there is already a check in qemu_machine_creation_done:
>
> if (machine->cgs && !machine->cgs->ready) {
> error_setg(errp, "accelerator does not support confidential guest %s",
> object_get_typename(OBJECT(machine->cgs)));
> exit(1);
> }
>
> the creation of RAMBlocks happens earlier, in qemu_init_board(), if
> the command line does not override the default memory backend with
> -M memdev. Then the RAMBlock will try to use guest_memfd (because
> machine_require_guest_memfd correctly returns true; at least correctly
> according to the current implementation) and trigger the assertion
> failure for kvm_enabled(). This happend with a command line as
> simple as the following:
>
> qemu-system-x86_64 -m 512 -nographic -object sev-snp-guest,reduced-phys-bits=48,id=sev0 \
> -M q35,kernel-irqchip=split,confidential-guest-support=sev0
> qemu-system-x86_64: ../system/physmem.c:1871: ram_block_add: Assertion `kvm_enabled()' failed.
I'd expect sev-snp-guest to bail out early enough.
Is a KVM-enabled check in sev_snp_guest_instance_init() missing?
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> system/physmem.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 67c9db9daad..1ddf9fb10d0 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1882,7 +1882,11 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> if (new_block->flags & RAM_GUEST_MEMFD) {
> int ret;
>
> - assert(kvm_enabled());
> + if (!kvm_enabled()) {
> + error_setg(errp, "cannot set up private guest memory for %s: KVM required",
> + object_get_typename(OBJECT(current_machine->cgs)));
Common code should be SW vs HW accel, so IMHO your check
should go within sev_snp_guest_instance_init(), removing
the assertion here. That said I have no clue about SEV.
> + goto out_free;
> + }
> assert(new_block->guest_memfd < 0);
>
> ret = ram_block_discard_require(true);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] physmem: replace assertion with error
2025-02-17 12:28 ` Philippe Mathieu-Daudé
@ 2025-02-17 13:15 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2025-02-17 13:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Xiaoyao Li, qemu-stable, Daniel P. Berrangé,
Marcelo Tosatti, Brijesh Singh, Pankaj Gupta
On Mon, Feb 17, 2025 at 1:28 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > qemu-system-x86_64 -m 512 -nographic -object sev-snp-guest,reduced-phys-bits=48,id=sev0 \
> > -M q35,kernel-irqchip=split,confidential-guest-support=sev0
> > qemu-system-x86_64: ../system/physmem.c:1871: ram_block_add: Assertion `kvm_enabled()' failed.
>
> I'd expect sev-snp-guest to bail out early enough.
>
> Is a KVM-enabled check in sev_snp_guest_instance_init() missing?
instance_init cannot do any check (it cannot fail). There is a check for
whether sev_common_kvm_init succeeded:
if (machine->cgs && !machine->cgs->ready) {
error_setg(errp, "accelerator does not support confidential guest %s",
object_get_typename(OBJECT(machine->cgs)));
exit(1);
}
but that doesn't help if the system/physmem.c code is reached before
qemu_machine_creation_done(), for example in qemu_init_board().
Likewise, you cannot fal in host_memory_backend_init() because that's
also an instance_init callback.
> > - assert(kvm_enabled());
> > + if (!kvm_enabled()) {
> > + error_setg(errp, "cannot set up private guest memory for %s: KVM required",
> > + object_get_typename(OBJECT(current_machine->cgs)));
>
> Common code should be SW vs HW accel, so IMHO your check
> should go within sev_snp_guest_instance_init(), removing
> the assertion here. That said I have no clue about SEV.
This code is outside accel/kvm or target/*/kvn, but it is not quite
common code. A few lines below you have:
new_block->guest_memfd =
kvm_create_guest_memfd(new_block->max_length, 0, errp);
Paolo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] physmem: replace assertion with error
2025-02-17 12:08 [PATCH] physmem: replace assertion with error Paolo Bonzini
2025-02-17 12:19 ` Daniel P. Berrangé
2025-02-17 12:28 ` Philippe Mathieu-Daudé
@ 2025-02-18 1:30 ` Xiaoyao Li
2025-02-18 15:45 ` David Hildenbrand
2025-02-19 6:39 ` Gupta, Pankaj
4 siblings, 0 replies; 7+ messages in thread
From: Xiaoyao Li @ 2025-02-18 1:30 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable
On 2/17/2025 8:08 PM, Paolo Bonzini wrote:
> It is possible to start QEMU with a confidential-guest-support object
> even in TCG mode. While there is already a check in qemu_machine_creation_done:
>
> if (machine->cgs && !machine->cgs->ready) {
> error_setg(errp, "accelerator does not support confidential guest %s",
> object_get_typename(OBJECT(machine->cgs)));
> exit(1);
> }
>
> the creation of RAMBlocks happens earlier, in qemu_init_board(), if
> the command line does not override the default memory backend with
> -M memdev. Then the RAMBlock will try to use guest_memfd (because
> machine_require_guest_memfd correctly returns true; at least correctly
> according to the current implementation) and trigger the assertion
> failure for kvm_enabled(). This happend with a command line as
> simple as the following:
>
> qemu-system-x86_64 -m 512 -nographic -object sev-snp-guest,reduced-phys-bits=48,id=sev0 \
> -M q35,kernel-irqchip=split,confidential-guest-support=sev0
> qemu-system-x86_64: ../system/physmem.c:1871: ram_block_add: Assertion `kvm_enabled()' failed.
>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> system/physmem.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 67c9db9daad..1ddf9fb10d0 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1882,7 +1882,11 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> if (new_block->flags & RAM_GUEST_MEMFD) {
> int ret;
>
> - assert(kvm_enabled());
> + if (!kvm_enabled()) {
> + error_setg(errp, "cannot set up private guest memory for %s: KVM required",
> + object_get_typename(OBJECT(current_machine->cgs)));
> + goto out_free;
> + }
> assert(new_block->guest_memfd < 0);
>
> ret = ram_block_discard_require(true);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] physmem: replace assertion with error
2025-02-17 12:08 [PATCH] physmem: replace assertion with error Paolo Bonzini
` (2 preceding siblings ...)
2025-02-18 1:30 ` Xiaoyao Li
@ 2025-02-18 15:45 ` David Hildenbrand
2025-02-19 6:39 ` Gupta, Pankaj
4 siblings, 0 replies; 7+ messages in thread
From: David Hildenbrand @ 2025-02-18 15:45 UTC (permalink / raw)
To: qemu-devel
On 17.02.25 13:08, Paolo Bonzini wrote:
> It is possible to start QEMU with a confidential-guest-support object
> even in TCG mode. While there is already a check in qemu_machine_creation_done:
>
> if (machine->cgs && !machine->cgs->ready) {
> error_setg(errp, "accelerator does not support confidential guest %s",
> object_get_typename(OBJECT(machine->cgs)));
> exit(1);
> }
>
> the creation of RAMBlocks happens earlier, in qemu_init_board(), if
> the command line does not override the default memory backend with
> -M memdev. Then the RAMBlock will try to use guest_memfd (because
> machine_require_guest_memfd correctly returns true; at least correctly
> according to the current implementation) and trigger the assertion
> failure for kvm_enabled(). This happend with a command line as
> simple as the following:
>
> qemu-system-x86_64 -m 512 -nographic -object sev-snp-guest,reduced-phys-bits=48,id=sev0 \
> -M q35,kernel-irqchip=split,confidential-guest-support=sev0
> qemu-system-x86_64: ../system/physmem.c:1871: ram_block_add: Assertion `kvm_enabled()' failed.
>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> system/physmem.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 67c9db9daad..1ddf9fb10d0 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1882,7 +1882,11 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> if (new_block->flags & RAM_GUEST_MEMFD) {
> int ret;
>
> - assert(kvm_enabled());
> + if (!kvm_enabled()) {
> + error_setg(errp, "cannot set up private guest memory for %s: KVM required",
> + object_get_typename(OBJECT(current_machine->cgs)));
> + goto out_free;
> + }
> assert(new_block->guest_memfd < 0);
>
> ret = ram_block_discard_require(true);
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] physmem: replace assertion with error
2025-02-17 12:08 [PATCH] physmem: replace assertion with error Paolo Bonzini
` (3 preceding siblings ...)
2025-02-18 15:45 ` David Hildenbrand
@ 2025-02-19 6:39 ` Gupta, Pankaj
4 siblings, 0 replies; 7+ messages in thread
From: Gupta, Pankaj @ 2025-02-19 6:39 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Xiaoyao Li, qemu-stable
On 2/17/2025 1:08 PM, Paolo Bonzini wrote:
> It is possible to start QEMU with a confidential-guest-support object
> even in TCG mode. While there is already a check in qemu_machine_creation_done:
>
> if (machine->cgs && !machine->cgs->ready) {
> error_setg(errp, "accelerator does not support confidential guest %s",
> object_get_typename(OBJECT(machine->cgs)));
> exit(1);
> }
>
> the creation of RAMBlocks happens earlier, in qemu_init_board(), if
> the command line does not override the default memory backend with
> -M memdev. Then the RAMBlock will try to use guest_memfd (because
> machine_require_guest_memfd correctly returns true; at least correctly
> according to the current implementation) and trigger the assertion
> failure for kvm_enabled(). This happend with a command line as
> simple as the following:
>
> qemu-system-x86_64 -m 512 -nographic -object sev-snp-guest,reduced-phys-bits=48,id=sev0 \
> -M q35,kernel-irqchip=split,confidential-guest-support=sev0
> qemu-system-x86_64: ../system/physmem.c:1871: ram_block_add: Assertion `kvm_enabled()' failed.
>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
> ---
> system/physmem.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 67c9db9daad..1ddf9fb10d0 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1882,7 +1882,11 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
> if (new_block->flags & RAM_GUEST_MEMFD) {
> int ret;
>
> - assert(kvm_enabled());
> + if (!kvm_enabled()) {
> + error_setg(errp, "cannot set up private guest memory for %s: KVM required",
> + object_get_typename(OBJECT(current_machine->cgs)));
> + goto out_free;
> + }
> assert(new_block->guest_memfd < 0);
>
> ret = ram_block_discard_require(true);
^ permalink raw reply [flat|nested] 7+ messages in thread