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