Re: [RFC v2-fix-v3 1/1] x86/tdx: Skip WBINVD instruction for TDX guest

[Dave Hansen]

On 6/8/21 3:36 PM, Kuppuswamy, Sathyanarayanan wrote:
> On 6/8/21 3:17 PM, Dave Hansen wrote:
>> On 6/8/21 2:35 PM, Kuppuswamy Sathyanarayanan wrote:
>>> Persistent memory is also currently not supported. Another code
>>> path that uses WBINVD is the MTRR driver, but EPT/virtualization
>>> always disables MTRRs so those are not needed. This all implies
>>> WBINVD is not needed with current TDX.
>>
>> It's one thing to declare something unsupported.  It's quite another to
>> declare it unsupported and then back it up with code to ensure that any
>> attempted use is thwarted.
> 
> Only audited and supported drivers will be allowed to enumerate after
> device filter support patch is merged. Till we merge that patch, If
> any of these unsupported features (with WBINVD usage) are enabled in TDX,
> it will lead to sigfault (due to unhandled #VE).

A kernel driver using WBINVD will "sigfault"?  I'm not sure what that
means.  How does the kernel "sigfault"?

> In this patch we only create exception for ACPI sleep driver code. If
> commit log is confusing, I can remove information about other unsupported
> feature (with WBINVD usage).

Yes, the changelog is horribly confusing.  But simply removing this
information is insufficient to rectify the deficiency.

I've lost trust that due diligence will be performed on this series on
its own.  I've seen too many broken promises and too many holes.

Here's what I want to see: a list of all of the unique call sites for
WBINVD in the kernel.  I want a written down methodology for how the
list of call sites was generated.  I want to see an item-by-item list of
why those call sites are unreachable with the TDX guest code.  It might
be because they've been patched in this patch, or the driver has been
disabled, or because the TDX architecture spec would somehow prohibit
the situation where it might be needed.  But, there needs to be a list,
and you have to show your work.  If you refer to code from this series
as helping to prevent WBINVD, then it has to be earlier in this series,
not in some other series and not later in this series.

Just eyeballing it, there are ~50 places in the kernel that need auditing.

Right now, we mostly have indiscriminate hand-waving about this not
being a problem.  It's a hard NAK from me on this patch until this audit
is in place.