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. 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 :) > So that the guard for KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES and > KVM_SET_MEMORY_ATTRIBUTES2 is consistent. > >> That said, the ordering here is wonky and misleading. A cursory read of the >> series >> would make one think that waiting to advertise >> KVM_CAP_GUEST_MEMFD_MEMORY_ATTRIBUTES >> makes it safe/ok for KVM to plumb in support for KVM_SET_MEMORY_ATTRIBUTES2 >> over >> multiple patches. But that's not actually true, because the ioctl becomes >> live >> the instant the code exists, userspace doesn't need to wait for KVM to >> formally >> advertise support. >> >> To further confuse matters, it is actually safe/ok to iteratively add >> support, >> because it's all effectively dead code until "Let userspace disable per-VM >> mem >> attributes, enable per-gmem attributes". > > yeah, before patch 24, gmem_in_place_conversion is always false. So > KVM_SET_MEMORY_ATTRIBUTES2 on gmem fd always return -ENOTTY. > >> So, I think we should go a step further than what I think Xiaoyao is >> suggesting, >> and fully squash patch 17 into patch 13. That way the reader doesn't have >> to jump >> through as many mental hoops to piece together what is happening. It'll >> obviously >> be a bigger patch, but should be easier to review/understand overall. >> >> Oh, and that combined patch should carve out error_offset straightaway, so >> that >> the full uAPI can be reviewed in a single patch. > > It sounds good.
