Re: [PATCH v4 03/12] kvm: Provide explicit error for kvm_create_guest_memfd()

On Fri, Aug 14, 2026 at 07:56:42AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Peter, Michael,
> 
> On 12/8/26 22:16, Michael Roth wrote:
> > From: Peter Xu <[email protected]>
> > 
> > So that there will be a verbal string returned when kvm not enabled, or
> > kvm not compiled.
> > 
> > Signed-off-by: Peter Xu <[email protected]>
> > Reviewed-by: Xiaoyao Li <[email protected]>
> > Reviewed-by: Fabiano Rosas <[email protected]>
> > Reviewed-by: Michael Roth <[email protected]>
> > Signed-off-by: Michael Roth <[email protected]>
> > ---
> >   accel/kvm/kvm-all.c    | 5 +++++
> >   accel/stubs/kvm-stub.c | 1 +
> >   2 files changed, 6 insertions(+)
> > 
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 120cab1e22..bda2e25a66 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -4758,6 +4758,11 @@ int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp)
> >           .flags = flags,
> >       };
> > +    if (!kvm_enabled()) {
> > +        error_setg(errp, "guest-memfd requires KVM accelerator");
> > +        return -1;
> 
> This doesn't sound right withing a KVM-specific method. We want to
> assert() here.
> 
> The call in ram_block_add() is already protected:
> 
> 2147 static void ram_block_add(RAMBlock *new_block, Error **errp)
> 2148 {
> ...
> 2185     if (new_block->flags & RAM_GUEST_MEMFD) {
> 2188         if (!kvm_enabled()) {
> 2189             error_setg(errp, "cannot set up private guest memory for
> %s: KVM required",
> 2190 object_get_typename(OBJECT(current_machine->cgs)));
> 2191             goto out_free;
> 2192         }
> ...
> 2203         new_block->guest_memfd =
> kvm_create_guest_memfd(new_block->max_length,
> 2204                                                         0, errp);
> 
> The other one is:
> 
> 2823 int ram_block_rebind(Error **errp)
> 2824 {
> ...
> 2829     RAMBLOCK_FOREACH(block) {
> ...
> 2834             block->guest_memfd =
> kvm_create_guest_memfd(block->max_length,
> 2835                                                         0, errp);
> 
> which is only called from KVM:
> 
> 2776 static int kvm_reset_vmfd(MachineState *ms)
> 2777 {
> ...
> 2827     /* rebind memory to new vm fd */
> 2828     ret = ram_block_rebind(&err);
> 
> So maybe what we want is:
> 
> -- >8 --
> diff --git a/system/physmem.c b/system/physmem.c
> index b97016b1303..66ff74541aa 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2824,6 +2824,8 @@ int ram_block_rebind(Error **errp)
>  {
>      RAMBlock *block;
> 
> +    assert(kvm_enabled()); /* Only supported by KVM so far */
> +
>      qemu_mutex_lock_ramlist();
> 
>      RAMBLOCK_FOREACH(block) {
> ---
> 
> Or less aggressive:
> 
> -- >8 --
> diff --git a/system/physmem.c b/system/physmem.c
> index b97016b1303..2988d1dd6c9 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2824,6 +2824,11 @@ int ram_block_rebind(Error **errp)
>  {
>      RAMBlock *block;
> 
> +    if (!kvm_enabled()) {
> +        error_setg(errp, "guest-memfd requires KVM accelerator");
> +        return -1;
> +    }
> +
>      qemu_mutex_lock_ramlist();
> 
>      RAMBLOCK_FOREACH(block) {
> ---
> 
> WDYT?

Fine by me.

IMHO it's normally more of an issue the other way round, if we used an
assert() where we should use error_setg() (hence, user triggerable
assert()s).  Here we expect it to never happen, so either way should not
happen..

If so, we could also assert() in ram_block_rebind(), as it's only used in
kvm_reset_vmfd() only, so I don't see how it can be reached if KVM is not
enabled first..

Thanks,

-- 
Peter Xu