Re: [PATCH] physmem: replace assertion with error

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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