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

[Jan Beulich <jbeulich@suse.com>]

On 27.04.2021 19:39, Andrew Cooper wrote:
> On 27/04/2021 16:47, Jan Beulich wrote:
>> On 26.04.2021 19:54, Andrew Cooper wrote:
>>> --- 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.

Well - what features we expose is part of the (or at least something
like an) ABI. The actual numbering of course isn't (or shouldn't be).
I'm in no way opposed to moving the header out of public/ (and until
now I was of the impression that it was you who put it there), but if
we do I think we will want an alternative way of expressing which
extended features we support. I say this in particular because I think
SUPPORT.md doesn't lend itself to documenting support status at this
level of granularity. I'd much rather see that file refer to this
header, even if this may mean some difficulty to non-programmers.

> 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.

For a release I consider this undesirable. If a feature is in such a
state at the point of entering the RC phase, I think such markings
ought to be undone.

> 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.

At _that_ point I could see the markings getting introduced outside
of debug builds, but still lower-case.

> 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).

Right, this is what is a prereq for the markings to become upper-case
ones (if - not specifically for the features here, but as a general
remark - we want them to become so ever in the first place) and the
features fully supported.

Jan

> 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
>