Re: [PATCH v2 36/58] memory: Introduce memory_region_init_ram_gmem()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Aug 18, 2023 at 05:50:19AM -0400, Xiaoyao Li wrote:
> Introduce memory_region_init_ram_gmem() to allocate private gmem on the
> MemoryRegion initialization. It's for the usercase of TDVF, which must
> be private on TDX case.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  include/exec/memory.h |  6 +++++
>  softmmu/memory.c      | 52 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)

> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index af6aa3c1e3c9..ded44dcef1aa 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -25,6 +25,7 @@
>  #include "qom/object.h"
>  #include "trace.h"
>  
> +#include <linux/kvm.h>
>  #include "exec/memory-internal.h"
>  #include "exec/ram_addr.h"
>  #include "sysemu/kvm.h"
> @@ -3602,6 +3603,57 @@ void memory_region_init_ram(MemoryRegion *mr,
>      vmstate_register_ram(mr, owner_dev);
>  }
>  
> +#ifdef CONFIG_KVM
> +void memory_region_init_ram_gmem(MemoryRegion *mr,
> +                                 Object *owner,
> +                                 const char *name,
> +                                 uint64_t size,
> +                                 Error **errp)

Since you have an 'errp' parameter here....

> +{
> +    DeviceState *owner_dev;
> +    Error *err = NULL;
> +    int priv_fd;
> +
> +    memory_region_init_ram_nomigrate(mr, owner, name, size, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    if (object_dynamic_cast(OBJECT(current_accel()), TYPE_KVM_ACCEL)) {
> +        KVMState *s = KVM_STATE(current_accel());
> +        struct kvm_create_guest_memfd gmem = {
> +            .size = size,
> +            /* TODO: add property to hostmem backend for huge pmd */
> +            .flags = KVM_GUEST_MEMFD_ALLOW_HUGEPAGE,
> +        };
> +
> +        priv_fd = kvm_vm_ioctl(s, KVM_CREATE_GUEST_MEMFD, &gmem);
> +        if (priv_fd < 0) {
> +            fprintf(stderr, "%s: error creating gmem: %s\n", __func__,
> +                    strerror(-priv_fd));
> +            abort();

It should be using error_setg_errno() here and return not abort

> +        }
> +    } else {
> +        fprintf(stderr, "%s: gmem unsupported accel: %s\n", __func__,
> +                current_accel_name());

and error_setg() here and return.

> +        abort();
> +    }
> +
> +    memory_region_set_gmem_fd(mr, priv_fd);
> +    memory_region_set_default_private(mr);
> +
> +    /* This will assert if owner is neither NULL nor a DeviceState.
> +     * We only want the owner here for the purposes of defining a
> +     * unique name for migration. TODO: Ideally we should implement
> +     * a naming scheme for Objects which are not DeviceStates, in
> +     * which case we can relax this restriction.
> +     */
> +    owner_dev = DEVICE(owner);
> +    vmstate_register_ram(mr, owner_dev);
> +}
> +#endif
> +
>  void memory_region_init_rom(MemoryRegion *mr,
>                              Object *owner,
>                              const char *name,
> -- 
> 2.34.1
> 

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 :|