Re: [PATCH v1 00/40] TDX QEMU support

[Xiaoyao Li]

On 8/4/2022 1:44 AM, Daniel P. Berrangé wrote:
> On Tue, Aug 02, 2022 at 06:55:48PM +0800, Xiaoyao Li wrote:
> > On 8/2/2022 5:49 PM, Daniel P. Berrangé wrote:
> > > On Tue, Aug 02, 2022 at 03:47:10PM +0800, Xiaoyao Li wrote:
> >
> > > > - CPU model
> > > >
> > > >     We cannot create a TD with arbitrary CPU model like what for non-TDX VMs,
> > > >     because only a subset of features can be configured for TD.
> > > >     - It's recommended to use '-cpu host' to create TD;
> > > >     - '+feature/-feature' might not work as expected;
> > > >
> > > >     future work: To introduce specific CPU model for TDs and enhance +/-features
> > > >                  for TDs.
> > >
> > > Which features are incompatible with TDX ?
> >
> > TDX enforces some features fixed to 1 (e.g., CPUID_EXT_X2APIC,
> > CPUID_EXT_HYPERVISOR)and some fixed to 0 (e.g., CPUID_EXT_VMX ).
> >
> > Details can be found in patch 8 and TDX spec chapter "CPUID virtualization"
> >
> > > Presumably you have such a list, so that KVM can block them when
> > > using '-cpu host' ?
> >
> > No, KVM doesn't do this. The result is no error reported from KVM but what
> > TD OS sees from CPUID might be different what user specifies in QEMU.
> >
> > > If so, we should be able to sanity check the
> > > use of these features in QEMU for the named CPU models / feature
> > > selection too.
> >
> > This series enhances get_supported_cpuid() for TDX. If named CPU models are
> > used to boot a TDX guest, it likely gets warning of "xxx feature is not
> > available"
>
> If the  ',check=on' arg is given to -cpu, does it ensure that the
> guest fails to startup with an incompatible feature set ? That's
> really the key thing to protect the user from mistakes.

"check=on" won't stop startup with an incompatible feature set but 
"enforce=on". Yes, this series can ensure it with "enforce=on"

> > We have another series to enhance the "-feature" for TDX, to warn out if
> > some fixed1 is specified to be removed. Besides, we will introduce specific
> > named CPU model for TDX. e.g., TDX-SapphireRapids which contains the maximum
> > feature set a TDX guest can have on SPR host.
>
> I don't know if this is the right approach or not, but we should at least
> consider making use of CPU versioning here.  ie have a single "SapphireRapids"
> alias, which resolves to a suitable specific CPU version depending on whether
> TDX is used or not.

New version of a CPU model inherits from the last version. This fits 
well with CPU model fixup when features need to be removed/added to 
existing CPU model to make it work well with the latest kernel, and a 
new version is created.

However, I think it less proper to define a TDX variant with versioned- 
cpu model. For example, we have a SPR-V(x), then we need to define 
SPR-V(x+1) and alias it as SPR-TDX. For SPR-V(x+1), we need to add and 
remove several features. In the future, we may need a SPR-V(x+2) to fix 
up the normal SPR cpu model SPR-V(x). All the changes in V(x+1)/SPR-TDX 
 has to be reverted at first.

Anyway, we can discuss it in the future when we post the series of TDX 
CPU model. We plan to do that after this basic series gets merged. :)

> With regards,
> Daniel