Re: [PATCH v3 4/9] KVM: guest_memfd: add GUEST_MEMFD_FLAG_MIGRATABLE

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Global capability check for KVM_CAP_GUEST_MEMFD_FLAGS falsely advertises GUEST_MEMFD_FLAG_MIGRATABLE on unsupported architectures.
- [Low] Typo in API documentation for GUEST_MEMFD_FLAG_MIGRATABLE.
--

--- Patch [4]: [PATCH v3 4/9] KVM: guest_memfd: add GUEST_MEMFD_FLAG_MIGRATABLE ---
commit f12765f58cf3ad7c38abcf69c609570a66578e93
Author: Shivank Garg <[email protected]>

KVM: guest_memfd: add GUEST_MEMFD_FLAG_MIGRATABLE
    
This patch adds architecture support checks and a new flag,
GUEST_MEMFD_FLAG_MIGRATABLE, to allow the MM to migrate guest_memfd folios.
It updates the mapping GFP mask and removes a redundant WARN_ON_ONCE.

> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index e3003a241d5b0..976921527ef9a 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6461,6 +6461,9 @@ specified via KVM_CREATE_GUEST_MEMFD.  Currently defined flags:
>                                 without INIT_SHARED will be marked private).
>                                 Shared memory can be faulted into host userspace
>                                 page tables. Private memory cannot.
> + GUEST_MEMFD_FLAG_MIGRATABLE   Allow MM to migrate guest_memfd folios.
> +                               Availability is architecture sepecific. Without

[Severity: Low]
This isn't a bug, but there is a typo here (sepecific instead of specific).

> +                               this flag, folios remain unmovable.
>    ============================ ================================================

[ ... ]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 47cb9eba113b1..7087b0b8f973f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -14130,6 +14130,15 @@ bool kvm_arch_supports_gmem_init_shared(struct kvm *kvm)
>  	return !kvm_arch_has_private_mem(kvm);
>  }
>  
> +/*
> + * Migration of guest_memfd with private memory is not supported yet
> + * as this may require architecture-specific handling.
> + */
> +bool kvm_arch_supports_gmem_migration(struct kvm *kvm)
> +{
> +	return !kvm_arch_has_private_mem(kvm);
> +}

[ ... ]

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index ab8cfaec82d31..7b2f15b219705 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -731,6 +731,7 @@ static inline bool kvm_arch_has_private_mem(struct kvm *kvm)
>  
>  #ifdef CONFIG_KVM_GUEST_MEMFD
>  bool kvm_arch_supports_gmem_init_shared(struct kvm *kvm);
> +bool kvm_arch_supports_gmem_migration(struct kvm *kvm);
>  
>  static inline u64 kvm_gmem_get_supported_flags(struct kvm *kvm)
>  {
> @@ -738,6 +739,9 @@ static inline u64 kvm_gmem_get_supported_flags(struct kvm *kvm)
>  
>  	if (!kvm || kvm_arch_supports_gmem_init_shared(kvm))
>  		flags |= GUEST_MEMFD_FLAG_INIT_SHARED;
> +	if (IS_ENABLED(CONFIG_MIGRATION) &&
> +	    (!kvm || kvm_arch_supports_gmem_migration(kvm)))
> +		flags |= GUEST_MEMFD_FLAG_MIGRATABLE;

[Severity: Medium]
When userspace queries KVM_CAP_GUEST_MEMFD_FLAGS on the global /dev/kvm
file descriptor, kvm is NULL. Does the !kvm check here cause the API to
unconditionally advertise GUEST_MEMFD_FLAG_MIGRATABLE for all architectures
where CONFIG_MIGRATION is enabled?

If so, architectures that do not support migration (relying on the weak
default returning false) would advertise support globally, but then return
-EINVAL when userspace actually attempts to create a guest_memfd with this
flag.

If we drop the !kvm bypass to fix the capability advertisement, would the
x86 implementation of kvm_arch_supports_gmem_migration() shown above need an
explicit check for a NULL kvm pointer before passing it to
kvm_arch_has_private_mem()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4