Re: [PATCH v13 016/113] KVM: TDX: x86: Add ioctl to get TDX systemwide parameters

[Xiaoyao Li]

On 3/31/2023 8:44 PM, Zhi Wang wrote:
> On Thu, 30 Mar 2023 17:18:03 -0700
> Isaku Yamahata <isaku.yamahata@xxxxxxxxx> wrote:
>
> > On Wed, Mar 29, 2023 at 04:17:22PM -0700,
> > Isaku Yamahata <isaku.yamahata@xxxxxxxxx> wrote:
> >
> > > On Sat, Mar 25, 2023 at 10:43:06AM +0200,
> > > Zhi Wang <zhi.wang.linux@xxxxxxxxx> wrote:
> > >
> > > > On Sun, 12 Mar 2023 10:55:40 -0700
> > > > isaku.yamahata@xxxxxxxxx wrote:
> > > >
> > > > Does this have to be a new generic ioctl with a dedicated new x86_ops? SNP
> > > > does not use it at all and all the system-scoped ioctl of SNP going through
> > > > the CCP driver. So getting system-scope information of TDX/SNP will end up
> > > > differently.
> > > >
> > > > Any thought, Sean? Moving getting SNP system-wide information to
> > > > KVM dev ioctl seems not ideal and TDX does not have a dedicated driver like
> > > > CCP. Maybe make this ioctl TDX-specific? KVM_TDX_DEV_OP?
> > >
> > > We only need global parameters of the TDX module, and we don't interact with TDX
> > > module at this point.  One alternative is to export those parameters via sysfs.
> > > Also the existence of the sysfs node indicates that the TDX module is
> > > loaded(initialized?) or not in addition to boot log.  Thus we can drop system
> > > scope one.
> > > What do you think?
>
> I like this idea and the patch below, it feels right for me now. It would be nice
> if more folks can chime in and comment.

SYSFS option requires CONFIG_SYSFS, which reqiures CONFIG_KVM_TDX to 
select CONFIG_SYSFS.

> > > Regarding to other TDX KVM specific ioctls (KVM_TDX_INIT_VM, KVM_TDX_INIT_VCPU,
> > > KVM_TDX_INIT_MEM_REGION, and KVM_TDX_FINALIZE_VM), they are specific to KVM.  So
> > > I don't think it can be split out to independent driver.
>
> They can stay in KVM as they are KVM-specific. SNP also has KVM-specific ioctls
> which wraps the SEV driver calls. At this level, both TDX and SNP go their specific
> implementation without more abstraction other than KVM_ENCRYPT_MEMORY_OP. Their
> strategies are aligned.
>
> The problem of the previous approach was the abstraction that no other implementation
> is using it. It is like, TDX wants a higher abstraction to cover both TDX and SNP,
> but SNP is not using it, which makes the abstraction looks strange.

Note, before this TDX enabling series, KVM_MEMORY_ENCRYPT_OP is a VM 
scope ioctl, that only serves for SEV and no other implementation uses 
it. I see no reason why cannot introduce a new IOCTL in x86 KVM that 
serves only one vendor.

We choose KVM_MEMORY_ENCRYPT_OP for TDX platform scope, just because we 
reuse KVM_MEMORY_ENCRYPT_OP for TDX VM-scope and extend it to TDX vcpu 
scope. It's just to avoid defining a new IOCTL number.

We can rename it to KVM_GET_CC_CAPABILITIES, and even return different 
capabilities based on VM type. And even, if SNP wants to use it, I think 
it can wrap SNP driver calls inside this IOCTL?

kvm.ko is special that it needs to serve two vendors. Sometime it's 
unaviodable that an interface is only used by one vendor.