Re: [PATCH 17/45] KVM: Introduce NR/NW/NX memory attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] KVM: wrong macro in #else of kvm_supports_memory_protection_attr()
- [Medium] KVM: x86: selecting KVM_VM_MEMORY_ATTRIBUTES on 32-bit breaks build
--

commit 7813791b6014f78cd1b4dde881abb82a354594d5
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: Introduce NR/NW/NX memory attributes

This commit introduces the KVM_MEMORY_ATTRIBUTE_NR, KVM_MEMORY_ATTRIBUTE_NW,
and KVM_MEMORY_ATTRIBUTE_NX memory attributes to map guest physical memory
regions as non-readable, non-writable, and/or non-executable, and adds basic
support on x86.

Link: https://patch.msgid.link/20260918135030.171564-14-pbonzini@redhat.com

> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index 1719059570fb7..3cde8bc5ff016 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -44,6 +44,7 @@ config KVM_X86
>  	select KVM_GENERIC_PRE_FAULT_MEMORY
>  	select KVM_WERROR if WERROR
>  	select KVM_GUEST_MEMFD if X86_64
> +	select KVM_VM_MEMORY_ATTRIBUTES

[Severity: Medium]
Does selecting KVM_VM_MEMORY_ATTRIBUTES unconditionally in config KVM_X86
break 32-bit x86 builds?

Previously, KVM_VM_MEMORY_ATTRIBUTES was selected by KVM_PRIVATE_MEMATTR_VM,
which only depended on 64-bit options (KVM_SW_PROTECTED_VM, KVM_INTEL_TDX,
and KVM_AMD_SEV). Selecting it unconditionally under KVM_X86 enables
CONFIG_KVM_VM_MEMORY_ATTRIBUTES on 32-bit x86 builds, which triggers the
compile-time assertion in virt/kvm/kvm_main.c:

kvm_vm_ioctl_set_mem_attributes() {
	...
	BUILD_BUG_ON(sizeof(attrs->attributes) != sizeof(unsigned long));
}

Also, if KVM_VM_MEMORY_ATTRIBUTES is gated on X86_64, removing the
CONFIG_KVM_VM_MEMORY_ATTRIBUTES guards in arch/x86/kvm/mmu/mmu.c and
arch/x86/kvm/x86.c (around kvm_mmu_init_memslot_memory_attributes() in
kvm_alloc_memslot_metadata(), and the calls to kvm_get_vm_memory_attributes()
and kvm_range_has_vm_memory_attributes() in mmu.c) will leave those callers
referencing symbols that have no fallback definitions when
CONFIG_KVM_VM_MEMORY_ATTRIBUTES is disabled.

[ ... ]

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 3d0293ebc864a..8442fb29a1ee8 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -726,6 +726,15 @@ static inline int kvm_arch_vcpu_memslots_id(struct kvm_vcpu *vcpu)
>  }
>  #endif
>  
> +#ifdef kvm_arch_has_memory_protection_attr
> +bool kvm_arch_mem_attributes_supported_prot(struct kvm *kvm, unsigned long attrs);
> +#else
> +static inline bool kvm_arch_has_memory_protection_attr(struct kvm *kvm)
> +{
> +	return false;
> +}
> +#endif

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 17341c689d11a..717d7aec9a6c9 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2473,13 +2473,28 @@ static u64 kvm_supports_private_mem(struct kvm *kvm)
>  #define kvm_supports_private_mem(kvm) false
>  #endif
>  
> +
> +#ifdef kvm_arch_has_memory_protection_attr
> +static u64 kvm_supports_memory_protection_attr(struct kvm *kvm)
> +{
> +	return !kvm || kvm_arch_has_memory_protection_attr(kvm);
> +}
> +#else
> +#define kvm_supports_private_mem(kvm) false
> +#endif
> +
>  #ifdef CONFIG_KVM_VM_MEMORY_ATTRIBUTES
>  u64 kvm_supported_vm_mem_attributes(struct kvm *kvm)
>  {
> -	if (gmem_in_place_conversion || !kvm_supports_private_mem(kvm))
> -		return 0;
> +	u64 supported_attrs = 0;
> +
> +	if (kvm_supports_memory_protection_attr(kvm))
> +		supported_attrs |= KVM_MEMORY_ATTRIBUTE_PROT;

[Severity: Medium]
Should the #else branch here define kvm_supports_memory_protection_attr(kvm)
instead of kvm_supports_private_mem(kvm)?

On architectures that do not define kvm_arch_has_memory_protection_attr as a
preprocessor macro, the #else branch defines kvm_supports_private_mem(kvm)
as false. This leaves kvm_supports_memory_protection_attr() undeclared when
called by kvm_supported_vm_mem_attributes(), while also overriding the
preceding definition of kvm_supports_private_mem(kvm) on architectures that
define kvm_arch_has_private_mem.

In addition, when kvm_arch_has_memory_protection_attr is not defined as a
macro, include/linux/kvm_host.h omits the prototype for
kvm_arch_mem_attributes_supported_prot(), which is still compiled
unconditionally as a non-static __weak function in virt/kvm/kvm_main.c when
CONFIG_KVM_VM_MEMORY_ATTRIBUTES is enabled, causing a missing-prototype
warning.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102174130.8779-1-pbonzini@redhat.com?part=17