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

On 7/25/2026 4:26 AM, Ackerley Tng wrote:
> Xiaoyao Li <[email protected]> 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.