Re: [PATCH v15 01/21] KVM: Rename CONFIG_KVM_PRIVATE_MEM to CONFIG_KVM_GMEM

[Fuad Tabba <tabba@google.com>]

Hi Sean,

On Mon, 21 Jul 2025 at 16:17, Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Jul 17, 2025, Fuad Tabba wrote:
> > Rename the Kconfig option CONFIG_KVM_PRIVATE_MEM to CONFIG_KVM_GMEM.
>
> Please name this CONFIG_KVM_GUEST_MEMFD.  I'm a-ok using gmem as the namespace
> for functions/macros/variables, but there's zero reason to shorten things like
> Kconfigs.

Ack.

> > @@ -719,10 +719,10 @@ static inline int kvm_arch_vcpu_memslots_id(struct kvm_vcpu *vcpu)
> >  #endif
> >
> >  /*
> > - * Arch code must define kvm_arch_has_private_mem if support for private memory
> > - * is enabled.
> > + * Arch code must define kvm_arch_has_private_mem if support for guest_memfd is
> > + * enabled.
>
> This is undesirable, and the comment is flat out wrong.  As evidenced by the lack
> of a #define in arm64, arch does NOT need to #define kvm_arch_has_private_mem if
> CONFIG_KVM_GUEST_MEMFD=y.  It "works" because the sole caller to kvm_arch_has_private_mem()
> is guarded by CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES=y, and that's never selected
> by arm64.
>
> I.e. this needs to key off of CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES=y, not off of
> CONFIG_KVM_GUEST_MEMFD=y.  And I would just drop the comment altogether at that
> point, because it's all quite self-explanatory:
>
> #ifndef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES
> static inline bool kvm_arch_has_private_mem(struct kvm *kvm)
> {
>         return false;
> }
> #endif

Ack.

>
> >   */
> > -#if !defined(kvm_arch_has_private_mem) && !IS_ENABLED(CONFIG_KVM_PRIVATE_MEM)
> > +#if !defined(kvm_arch_has_private_mem) && !IS_ENABLED(CONFIG_KVM_GMEM)
> >  static inline bool kvm_arch_has_private_mem(struct kvm *kvm)
> >  {
> >       return false;
> > @@ -2527,7 +2527,7 @@ bool kvm_arch_post_set_memory_attributes(struct kvm *kvm,
> >
> >  static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
> >  {
> > -     return IS_ENABLED(CONFIG_KVM_PRIVATE_MEM) &&
> > +     return IS_ENABLED(CONFIG_KVM_GMEM) &&
>
> And this is equally wrong.  The existing code checked CONFIG_KVM_PRIVATE_MEM,
> because memory obviously can't be private if private memory is unsupported.
>
> But that logic chain doesn't work as well for guest_memfd.  In a way, this is a
> weird semantic change, e.g. it changes from "select guest_memfd if private memory
> is supported" to "allow private memory if guest_memfd is select".   The former
> existed because compiling in support for guest_memfd when it coulnd't possibly
> be used was wasteful, but even then it was somewhat superfluous.
>
> The latter is an arbitrary requirement that probably shouldn't exist, and if we
> did want to make it a hard requirement, should be expressed in the Kconfig
> dependency, not here.
>
> TL;DR: drop the IS_ENABLED(CONFIG_KVM_GMEM) check.

Ack.

Thanks!
/fuad