On Thu, Sep 09, 2021, Brijesh Singh wrote: > > On 9/7/21 6:37 PM, Sean Christopherson wrote: > > On Tue, Sep 07, 2021, Brijesh Singh wrote: > > > I have no strong preference for either of the abstraction approaches. The > > > sheer number of argument can also make some folks wonder whether such > > > abstraction makes it easy to read. e.g send-start may need up to 11. > > > > Yeah, that's brutal, but IMO having a few ugly functions is an acceptable cost if > > it means the rest of the API is cleaner. E.g. KVM is not the right place to > > implement sev_deactivate_lock, as any coincident DEACTIVATE will be problematic. > > The current code "works" because KVM is the only in-tree user, but even that's a > > bit of a grey area because sev_guest_deactivate() is exported. > > > > If large param lists are problematic, one idea would be to reuse the sev_data_* > > structs for the API. I still don't like the idea of exposing those structs > > outside of the PSP driver, and the potential user vs. kernel pointer confusion > > is more than a bit ugly. On the other hand it's not exactly secret info, > > e.g. KVM's UAPI structs are already excrutiatingly close to sev_data_* structs. > > > > For future ioctls(), KVM could even define UAPI structs that are bit-for-bit > > compatible with the hardware structs. That would allow KVM to copy userspace's > > data directly into a "struct sev_data_*" and simply require the handle and any > > other KVM-defined params to be zero. KVM could then hand the whole struct over > > to the PSP driver for processing. > > Most of the address field in the "struct sev_data_*" are physical > addressess. The userspace will not be able to populate those fields. Yeah, that's my biggest hesitation to using struct sev_data_* in the API, it's both confusing and gross. But it's also why I think these helpers belong in the PSP driver, KVM should not need to know the "on-the-wire" format for communicating with the PSP. > PSP or KVM may still need to assist filling the final hardware structure. > Some of fields in hardware structure must be zero, so we need to add checks > for it. > I can try posting RFC post SNP series and we can see how it all looks. I'm a bit torn. I completely understand the desire to get SNP support merged, but at the same time KVM has accrued a fair bit of technical debt for SEV and SEV-ES, and the lack of tests is also a concern. I don't exactly love the idea of kicking those cans further down the road. Paolo, any thoughts?
