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

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?

> +    }
> +
>       if (!kvm_guest_memfd_supported) {
>           error_setg(errp, "KVM does not support guest_memfd");
>           return -1;
> diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
> index 3d34e3b99d..acbd0785e0 100644
> --- a/accel/stubs/kvm-stub.c
> +++ b/accel/stubs/kvm-stub.c
> @@ -141,6 +141,7 @@ bool kvm_hwpoisoned_mem(void)
>   
>   int kvm_create_guest_memfd(uint64_t size, uint64_t flags, Error **errp)
>   {
> +    error_setg(errp, "KVM is not enabled");
>       return -ENOSYS;
>   }
>