On Thu, Feb 08, 2024, David Woodhouse wrote:
Using anything except NULL as the "no value" value doesn't make sense to me. It violates the principle of least surprise and would be a really bad API.
I'm a-ok with using '0'. My only request is to check for "!hva" as opposed to "hva == 0", both because that's preferred kernel style, and because it better conveys that it's really checking for !NULL as opposed to address '0'.
Side topic, I think the code will end up in a more readable state if the GFN vs. HVA sub-commands are handled in separate case statements, especially if/when xen_lock goes away. E.g. something like this:
case KVM_XEN_ATTR_TYPE_SHARED_INFO: { int idx;
if (data->u.shared_info.gfn == KVM_XEN_INVALID_GFN) { kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache); r = 0; break; }
idx = srcu_read_lock(&kvm->srcu); r = kvm_gpc_activate(&kvm->arch.xen.shinfo_cache, gfn_to_gpa(data->u.shared_info.gfn), PAGE_SIZE); if (!r && kvm->arch.xen.shinfo_cache.active) r = kvm_xen_shared_info_init(kvm); srcu_read_unlock(&kvm->srcu, idx); break; }
case KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA: { unsigned long hva = data->u.shared_info.hva;
if (hva != untagged_addr(hva) || !access_ok((void __user *)hva) || !PAGE_ALIGNED(hva)) { r = -EINVAL; break; }
if (!hva) { kvm_gpc_deactivate(&kvm->arch.xen.shinfo_cache); r = 0; break; } r = kvm_gpc_activate_hva(&kvm->arch.xen.shinfo_cache, hva, PAGE_SIZE); if (!r && kvm->arch.xen.shinfo_cache.active) r = kvm_xen_shared_info_init(kvm); break; }
Side topic #2, the above requires that __kvm_gpc_refresh() not grab kvm_memslots() in the "using an hva" path, but I think that'd actually be a good thing as it would make it a bit more clear that using an hva bypasses memslots by design.