On 8/13/2024 10:27 AM, Sean Christopherson wrote:
On Tue, Aug 13, 2024, Pratik R. Sampat wrote:
On 8/9/2024 10:40 AM, Sean Christopherson wrote:
On Wed, Jul 10, 2024, Pratik R. Sampat wrote:
@@ -98,7 +100,7 @@ static inline void sev_register_encrypted_memory(struct kvm_vm *vm, vm_ioctl(vm, KVM_MEMORY_ENCRYPT_REG_REGION, &range); } -static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, +static inline int snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, uint64_t size, uint8_t type) { struct kvm_sev_snp_launch_update update_data = { @@ -108,10 +110,10 @@ static inline void snp_launch_update_data(struct kvm_vm *vm, vm_paddr_t gpa, .type = type, };
- vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
- return __vm_sev_ioctl(vm, KVM_SEV_SNP_LAUNCH_UPDATE, &update_data);
Don't introduce APIs and then immediately rewrite all of the users. If you want to rework similar APIs, do the rework, then add the new APIs. Doing things in this order adds a pile of pointless churn.
But that's a moot point, because it's far easier to just add __snp_launch_update_data(). And if you look through other APIs in kvm_util.h, you'll see that the strong preference is to let vm_ioctl(), or in this case vm_sev_ioctl(), do the heavy lifting. Yeah, it requires copy+pasting marshalling parameters into the struct, but that's relatively uninteresting code, _and_ piggybacking the "good" version means you can't do things like pass in a garbage virtual address (because the "good" version always guarantees a good virtual address).
I am a little confused by this.
Are you suggesting that I leave the original functions intact with using vm_sev_ioctl() and have an additional variant such as __snp_launch_update_data() which calls into __vm_sev_ioctl() to decouple the ioctl from the assert for negative asserts?
Yes, this one.
Got it. Thanks a lot!
Or, do you suggest that I alter vm_sev_ioctl() to handle both positive and negative asserts?
Thanks! -Pratik