Messages in this thread | | | Date | Mon, 13 Nov 2023 11:37:05 +0800 | Subject | Re: [PATCH 15/34] KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory | From | Xiaoyao Li <> |
| |
On 11/11/2023 2:22 AM, Sean Christopherson wrote: > On Fri, Nov 10, 2023, Xiaoyao Li wrote: >> On 11/6/2023 12:30 AM, Paolo Bonzini wrote: >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index 68a144cb7dbc..a6de526c0426 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -589,8 +589,20 @@ struct kvm_memory_slot { >>> u32 flags; >>> short id; >>> u16 as_id; >>> + >>> +#ifdef CONFIG_KVM_PRIVATE_MEM >>> + struct { >>> + struct file __rcu *file; >>> + pgoff_t pgoff; >>> + } gmem; >>> +#endif >>> }; >>> +static inline bool kvm_slot_can_be_private(const struct kvm_memory_slot *slot) >>> +{ >>> + return slot && (slot->flags & KVM_MEM_GUEST_MEMFD); >>> +} >>> + >> >> maybe we can move this block and ... >> >> <snip> >> >>> @@ -2355,6 +2379,30 @@ bool kvm_arch_pre_set_memory_attributes(struct kvm *kvm, >>> struct kvm_gfn_range *range); >>> bool kvm_arch_post_set_memory_attributes(struct kvm *kvm, >>> struct kvm_gfn_range *range); >>> + >>> +static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn) >>> +{ >>> + return IS_ENABLED(CONFIG_KVM_PRIVATE_MEM) && >>> + kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE; >>> +} >>> +#else >>> +static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn) >>> +{ >>> + return false; >>> +} >>> #endif /* CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES */ >> >> this block to Patch 18? > > It would work, but my vote is to keep them here to minimize the changes to common > KVM code in the x86 enabling. It's not a strong preference though. Of course, > at this point, fiddling with this sort of thing is probably a bad idea in terms > of landing guest_memfd.
Indeed. It's OK then.
>>> @@ -4844,6 +4875,10 @@ static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg) >>> #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES >>> case KVM_CAP_MEMORY_ATTRIBUTES: >>> return kvm_supported_mem_attributes(kvm); >>> +#endif >>> +#ifdef CONFIG_KVM_PRIVATE_MEM >>> + case KVM_CAP_GUEST_MEMFD: >>> + return !kvm || kvm_arch_has_private_mem(kvm); >>> #endif >>> default: >>> break; >>> @@ -5277,6 +5312,18 @@ static long kvm_vm_ioctl(struct file *filp, >>> case KVM_GET_STATS_FD: >>> r = kvm_vm_ioctl_get_stats_fd(kvm); >>> break; >>> +#ifdef CONFIG_KVM_PRIVATE_MEM >>> + case KVM_CREATE_GUEST_MEMFD: { >>> + struct kvm_create_guest_memfd guest_memfd; >> >> Do we need a guard of below? >> >> r = -EINVAL; >> if (!kvm_arch_has_private_mem(kvm)) >> goto out; > > Argh, yeah, that's weird since KVM_CAP_GUEST_MEMFD says "not supported" if the > VM doesn't support private memory. > > Enforcing that would break guest_memfd_test.c though. And having to create a > "special" VM just to test basic guest_memfd functionality would be quite > annoying. > > So my vote is to do: > > case KVM_CAP_GUEST_MEMFD: > return IS_ENABLED(CONFIG_KVM_PRIVATE_MEM);
I'm fine with it.
> There's no harm to KVM if userspace creates a file it can't use, and at some > point KVM will hopefully support guest_memfd irrespective of private memory.
| |