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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/04/2021 10:11, Jan Beulich wrote:
> 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.

Yes, but that ABI is called CPUID and MSRs, per Intel/AMD's spec.

VM's see it via the real instructions.  Control software sees it via a
pair of arrays ({leaf, subleaf, a, b, c, d} and {idx, val}) expressed in
terms of the vendors 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.

We don't need to say or do anything for experimental features.

Hitting Tech Preview and supported ought to be in release notes.  I do
agree that SUPPORT.md isn't great.

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

Well no.  They either don't go in in the first place, or they go in and
stay.  Putting them in with an expectation to take them out later is a
recipe for forgetting to do so.

I know we're making up our "how to do complicated experimental features"
process as we go here, but nothing *in Xen* will malfunction if a user
opts into CET_SS/IBT.  Therefore I think they're fine to go in and stay.

~Andrew