Re: [PATCH v8 17/46] KVM: guest_memfd: Advertise KVM_SET_MEMORY_ATTRIBUTES2 ioctl

[Xiaoyao Li]

On 7/25/2026 4:26 AM, Ackerley Tng wrote:
> Xiaoyao Li <xiaoyao.li@xxxxxxxxx> writes:
>
> > On 7/2/2026 12:09 AM, Sean Christopherson wrote:
> > > On Wed, Jul 01, 2026, Xiaoyao Li wrote:
> > > > On 6/19/2026 8:31 AM, Ackerley Tng via B4 Relay wrote:
> > > > > @@ -4969,6 +4973,11 @@ static int kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
> > > > >     		return 1;
> > > > >     	case KVM_CAP_GUEST_MEMFD_FLAGS:
> > > > >     		return kvm_gmem_get_supported_flags(kvm);
> > > > > +	case KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES:
> > > > > +		if (!gmem_in_place_conversion || !kvm_supports_private_mem(kvm))
> > > > > +			return 0;
> > > > > +
> > > > > +		return KVM_MEMORY_ATTRIBUTE_PRIVATE;
> > > > >     #endif
> > > > >     	default:
> > > > >     		break;
> > > >
> > > > this looks inconsistent with the
> > > >
> > > > 	case KVM_SET_MEMORY_ATTRIBUTES2:
> > > > 		if (!gmem_in_place_conversion)
> > > > 			return -ENOTTY;
> > > >
> > > > Well, the check of
> > > >
> > > > 	if (!kvm_arch_has_private_mem(f->kvm))
> > > > 		return -EINVAL;
> > > >
> > > > is buried in the following kvm_gmem_set_attributes(). How about moving of
> > > > kvm_arch_has_private_mem() check to put it along with
> > > > gmem_in_place_conversion check in kvm_gmem_ioctl() in Patch 13?
> > >
> > > Me confused, patch 13 already adds the kvm_arch_has_private_mem() in
> > > kvm_gmem_set_attributes().
> >
> > I wanted to make the check in KVM_SET_MEMORY_ATTRIBUTES2 in Patch 13
> > like something below:
> >
> > 	case KVM_SET_MEMORY_ATTRIBUTES2:
> > 		if (!gmem_in_place_conversion || !kvm_arch_has_private_mem(f->kvm))
> > 		return -EXXX;
> >
> > and finally, with the introduction of kvm_supports_private_mem() in this
> > patch, it becomes:
> >
> > 	case KVM_SET_MEMORY_ATTRIBUTES2:
> > 		if (!gmem_in_place_conversion || !kvm_supports_private_mem(f->kvm))
> > 		return -EXXX;
>
> Is the request here to make KVM_SET_MEMORY_ATTRIBUTES2 return -ENOTTY
> instead of -EINVAL? kvm_gmem_set_attributes() also checks for
> !kvm_arch_has_private_mem() and returns -EINVAL.

Not really.

I want both "KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES" and 
"KVM_SET_MEMORY_ATTRIBUTES2 on gmem" to check gmem_in_place_conversion 
and kvm_supports_private_mem() in the same if(), or side by side if two 
different return code is desired.

Because the current code somewhat confused me on why 
KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES checks two conditions while 
KVM_SET_MEMORY_ATTRIBUTES2 on gmem only checks gmem_in_place_conversion. 
Only later did I realize that KVM_SET_MEMORY_ATTRIBUTES2 on gmem also 
checks kvm_supports_private_mem(), but buried in the following handling.

> I'm about to post v9, which squashes the patches as Sean
> suggested. Please review v9 instead to see if this concern is already
> addressed :)

sure.