Re: [PATCH 1/3] x86/hvm: Introduce experimental guest CET support

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/04/2021 16:47, Jan Beulich wrote:
> On 26.04.2021 19:54, Andrew Cooper wrote:
>> For now, let VMs opt into using CET by setting cet_ss/ibt in the CPUID
>> policy.  Also extend cr4 handling to permit CR4.CET being set, along with
>> logic to interlock CR4.CET and CR0.WP.
>>
>> Everything else will malfunction for now, but this will help adding support
>> incrementally - there is a lot to do before CET will work properly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> Just one consideration:
>
>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>> @@ -232,7 +232,7 @@ XEN_CPUFEATURE(UMIP,          6*32+ 2) /*S  User Mode 
>> Instruction Prevention */
>>  XEN_CPUFEATURE(PKU,           6*32+ 3) /*H  Protection Keys for Userspace */
>>  XEN_CPUFEATURE(OSPKE,         6*32+ 4) /*!  OS Protection Keys Enable */
>>  XEN_CPUFEATURE(AVX512_VBMI2,  6*32+ 6) /*A  Additional AVX-512 Vector Byte 
>> Manipulation Instrs */
>> -XEN_CPUFEATURE(CET_SS,        6*32+ 7) /*   CET - Shadow Stacks */
>> +XEN_CPUFEATURE(CET_SS,        6*32+ 7) /*h  CET - Shadow Stacks */
>>  XEN_CPUFEATURE(GFNI,          6*32+ 8) /*A  Galois Field Instrs */
>>  XEN_CPUFEATURE(VAES,          6*32+ 9) /*A  Vector AES Instrs */
>>  XEN_CPUFEATURE(VPCLMULQDQ,    6*32+10) /*A  Vector Carry-less 
>> Multiplication Instrs */
>> @@ -267,7 +267,7 @@ XEN_CPUFEATURE(SRBDS_CTRL,    9*32+ 9) /*   
>> MSR_MCU_OPT_CTRL and RNGDS_MITG_DIS.
>>  XEN_CPUFEATURE(MD_CLEAR,      9*32+10) /*A  VERW clears microarchitectural 
>> buffers */
>>  XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */
>>  XEN_CPUFEATURE(SERIALIZE,     9*32+14) /*a  SERIALIZE insn */
>> -XEN_CPUFEATURE(CET_IBT,       9*32+20) /*   CET - Indirect Branch Tracking 
>> */
>> +XEN_CPUFEATURE(CET_IBT,       9*32+20) /*h  CET - Indirect Branch Tracking 
>> */
>>  XEN_CPUFEATURE(IBRSB,         9*32+26) /*A  IBRS and IBPB support (used by 
>> Intel) */
>>  XEN_CPUFEATURE(STIBP,         9*32+27) /*A  STIBP */
>>  XEN_CPUFEATURE(L1D_FLUSH,     9*32+28) /*S  MSR_FLUSH_CMD and L1D flush. */
> If by the time 4.16 finishes up the various todo items haven't been
> taken care of, should we take note to undo these markings? I would
> have suggested allowing them for debug builds only, but that's kind
> of ugly to achieve in a public header.

TBH, I still don't think this should be a public header.  If I would
have forseen how much of a PITA is it, I would have argued harder
against it.

It is, at best, tools-only (via SYSCTL_get_featureset), and I don't
intend that interface to survive the tools ABI stabilisation  work, as
it has been fully superseded by the cpu_policy interfaces and libx86.


As for the max markings themselves, I'm not sure they ought to be
debug-only.  Its important aspect of making guest support "tech preview"
to ensure the logic works irrespective of CONFIG_DEBUG, and I think it
is entirely fine for an experimental feature to be of status "your VM
will explode if you enable this right now", even if that spills over
into 4.17.

In reality, once we've got {U,S}_CET context switching working at the
Xen level, and {RD,WR}MSR plumbing done, it ought to be safe to people
to turn on an experiment with.  At this point, we're in the position of
"expected to work correctly in a subset of usecases".  I'd ideally like
to get to this point before 4.16 releases, but that will be very subject
to other work.


All the emulator work is for cases that a VM won't encounter by default
(Task Switch too, as Minix in the Intel Management Engine is the only
known 32-bit CET-SS implementation).

Obviously, we want to get the checklist complete before enabling by
default, but give the complexities in the emulator, I suspect we'll have
a long gap between "believed can be used safely in a subset of cases",
and "believed safe to use in general", and a long list of people happy
to use it in a "doesn't work under introspection yet" state.

~Andrew