Re: [PATCH v3 1/4] x86/spec-ctrl: add logic to issue IBPB on exit to guest

[Andrew Cooper <amc96@xxxxxxxx>]

On 26/01/2023 8:02 am, Jan Beulich wrote:
> On 25.01.2023 22:10, Andrew Cooper wrote:
>> On 25/01/2023 3:25 pm, Jan Beulich wrote:
>>> In order to be able to defer the context switch IBPB to the last
>>> possible point, add logic to the exit-to-guest paths to issue the
>>> barrier there, including the "IBPB doesn't flush the RSB/RAS"
>>> workaround. Since alternatives, for now at least, can't nest, emit JMP
>>> to skip past both constructs where both are needed. This may be more
>>> efficient anyway, as the sequence of NOPs is pretty long.
>> It is very uarch specific as to when a jump is less overhead than a line
>> of nops.
>>
>> In all CPUs liable to be running Xen, even unconditional jumps take up
>> branch prediction resource, because all branch prediction is pre-decode
>> these days, so branch locations/types/destinations all need deriving
>> from %rip and "history" alone.
>>
>> So whether a branch or a line of nops is better is a tradeoff between
>> how much competition there is for branch prediction resource, and how
>> efficiently the CPU can brute-force its way through a long line of nops.
>>
>> But a very interesting datapoint.  It turns out that AMD Zen4 CPUs
>> macrofuse adjacent nops, including longnops, because it reduces the
>> amount of execute/retire resources required.  And a lot of
>> kernel/hypervisor fastpaths have a lot of nops these days.
>>
>>
>> For us, the "can't nest" is singularly more important than any worry
>> about uarch behaviour.  We've frankly got much lower hanging fruit than
>> worring about one branch vs a few nops.
>>
>>> LFENCEs are omitted - for HVM a VM entry is immanent, which already
>>> elsewhere we deem sufficiently serializing an event. For 32-bit PV
>>> we're going through IRET, which ought to be good enough as well. While
>>> 64-bit PV may use SYSRET, there are several more conditional branches
>>> there which are all unprotected.
>> Privilege changes are serialsing-ish, and this behaviour has been
>> guaranteed moving forwards, although not documented coherently.
>>
>> CPL (well - privilege, which includes SMM, root/non-root, etc) is not
>> written speculatively.  So any logic which needs to modify privilege has
>> to block until it is known to be an architectural execution path.
>>
>> This gets us "lfence-like" or "dispatch serialising" behaviour, which is
>> also the reason why INT3 is our go-to speculation halting instruction. 
>> Microcode has to be entirely certain we are going to deliver an
>> interrupt/exception/etc before it can start reading the IDT/etc.
>>
>> Either way, we've been promised that all instructions like IRET,
>> SYS{CALL,RET,ENTER,EXIT}, VM{RUN,LAUNCH,RESUME} (and ERET{U,S} in the
>> future FRED world) do, and shall continue to not execute speculatively.
>>
>> Which in practice means we don't need to worry about Spectre-v1 attack
>> against codepaths which hit an exit-from-xen path, in terms of skipping
>> protections.
>>
>> We do need to be careful about memory accesses and potential double
>> dereferences, but all the data is on the top of the stack for XPTI
>> reasons.  About the only concern is v->arch.msrs->* in the HVM path, and
>> we're fine with the current layout (AFAICT).
>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> I have to admit that I'm not really certain about the placement of the
>>> IBPB wrt the MSR_SPEC_CTRL writes. For now I've simply used "opposite of
>>> entry".
>> It really doesn't matter.  They're independent operations that both need
>> doing, and are fully serialising so can't parallelise.
>>
>> But on this note, WRMSRNS and WRMSRLIST are on the horizon.  The CPUs
>> which implement these instructions are the ones which also ought not to
>> need any adjustments in the exit paths.  So I think it is specifically
>> not worth trying to make any effort to turn *these* WRMSR's into more
>> optimised forms.
>>
>> But WRMSRLIST was designed specifically for this kind of usecase
>> (actually, more for the main context switch path) where you can prepare
>> the list of MSRs in memory, including the ability to conditionally skip
>> certain entries by adjusting the index field.
>>
>>
>> It occurs to me, having written this out, is that what we actually want
>> to do is have slightly custom not-quite-alternative blocks.  We have a
>> sequence of independent code blocks, and a small block at the end that
>> happens to contain an IRET.
>>
>> We could remove the nops at boot time if we treated it as one large
>> region, with the IRET at the end also able to have a variable position,
>> and assembles the "active" blocks tightly from the start.  Complications
>> would include adjusting the IRET extable entry, but this isn't
>> insurmountable.  Entrypoints are a bit more tricky but could be done by
>> packing from the back forward, and adjusting the entry position.
>>
>> Either way, something to ponder.  (It's also possible that it doesn't
>> make a measurable difference until we get to FRED, at which point we
>> have a set of fresh entry-points to write anyway, and a slight glimmer
>> of hope of not needing to pollute them with speculation workarounds...)
>>
>>> Since we're going to run out of SCF_* bits soon and since the new flag
>>> is meaningful only in struct cpu_info's spec_ctrl_flags, we could choose
>>> to widen that field to 16 bits right away and then use bit 8 (or higher)
>>> for the purpose here.
>> I really don't think it matters.  We've got plenty of room, and the
>> flexibility to shuffle, in both structures.  It's absolutely not worth
>> trying to introduce asymmetries to save 1 bit.
> Thanks for all the comments up to here. Just to clarify - I've not spotted
> anything there that would result in me being expected to take any action.

I'm tempted to suggest dropping the sentence about "might be more
efficient".  It's not necessary at all IMO, and it's probably not
correct if you were to compare an atom microserver vs a big Xeon.

For the lfences paragraph, I'd suggest rephrasing to something like this:

"As with all other conditional blocks on exit-to-guest paths, no
Spectre-v1 protections are necessary as execution will imminently be
hitting a serialising event."

The commentary on IRET/SYSRET and other jumps is adding confusion to
matter which is known to be safe.

>
>>> --- a/xen/arch/x86/include/asm/current.h
>>> +++ b/xen/arch/x86/include/asm/current.h
>>> @@ -55,9 +55,13 @@ struct cpu_info {
>>>  
>>>      /* See asm/spec_ctrl_asm.h for usage. */
>>>      unsigned int shadow_spec_ctrl;
>>> +    /*
>>> +     * spec_ctrl_flags can be accessed as a 32-bit entity and hence needs
>>> +     * placing suitably.
>> I'd suggest "is accessed as a 32-bit entity, and wants aligning suitably" ?
> I've tried to choose the wording carefully: The 32-bit access is in an
> alternative block, so doesn't always come into play. Hence the "may",
> not "is". Alignment alone also isn't sufficient here (and mis-aligning
> isn't merely a performance problem) - the following three bytes also
> need to be valid to access in the first place. Hence "needs" and
> "placing", not "wants" and "aligning".

"can" is actually a very ambiguous, and one valid interpretation of the
sentence is "spec_ctrl_fags is permitted to be accessed as a 32bit
quantity, but isn't actually accessed in that way".

In this case, we mean "it *is* accessed as a 32bit entity, in certain
cases".

So what about:

"spec_ctrl_flags is accessed as a 32-bit entity in some cases.  Place it
so it can be used as if it were an int."

?

>
>> If I've followed the logic correctly.  (I can't say I was specifically
>> aware that the bit test instructions didn't have byte forms, but I
>> suspect such instruction forms would be very very niche.)
> Yes, there not being byte forms of BT* is the sole reason here.
>
>>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>>> @@ -36,6 +36,8 @@
>>>  #define SCF_verw       (1 << 3)
>>>  #define SCF_ist_ibpb   (1 << 4)
>>>  #define SCF_entry_ibpb (1 << 5)
>>> +#define SCF_exit_ibpb_bit 6
>>> +#define SCF_exit_ibpb  (1 << SCF_exit_ibpb_bit)
>> One option to avoid the second define is to use ILOG2() with btrl.
> Specifically not. The assembler doesn't know the conditional operator,
> and the pre-processor won't collapse the expression resulting from
> expanding ilog2().

Oh that's dull.

I suspect we could construct equivalent logic with an .if/.else chain,
but I have no idea if the order of evaluation would be conducive to
doing so as part of evaluating an immediate operand.  We would
specifically not want something that ended looking like `ilog2 const
"btrl $" ",%eax"`, even though I could see how to make that work.

It would be nice if we could make something suitable here, but if not we
can live with the second _bit constant.

>
>>> @@ -272,6 +293,14 @@
>>>  #define SPEC_CTRL_EXIT_TO_PV                                            \
>>>      ALTERNATIVE "",                                                     \
>>>          DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_PV;              \
>>> +    ALTERNATIVE __stringify(jmp PASTE(.Lscexitpv_done, __LINE__)),      \
>>> +        __stringify(DO_SPEC_CTRL_EXIT_IBPB                              \
>>> +                    disp=(PASTE(.Lscexitpv_done, __LINE__) -            \
>>> +                          PASTE(.Lscexitpv_rsb, __LINE__))),            \
>>> +        X86_FEATURE_IBPB_EXIT_PV;                                       \
>>> +PASTE(.Lscexitpv_rsb, __LINE__):                                        \
>>> +    ALTERNATIVE "", DO_OVERWRITE_RSB, X86_BUG_IBPB_NO_RET;              \
>>> +PASTE(.Lscexitpv_done, __LINE__):                                       \
>>>      DO_SPEC_CTRL_COND_VERW
>> What's wrong with the normal %= trick?
> We're in a C macro here which is then used in assembly code. %= only
> works in asm(), though. If we were in an assembler macro, I'd have
> used \@. Yet wrapping the whole thing in an assembler macro would, for
> my taste, hide too much information from the use sites (in particular
> the X86_{FEATURE,BUG}_* which are imo relevant to be visible there).
>
>>   The use of __LINE__ makes this
>> hard to subsequently livepatch, so I'd prefer to avoid it if possible.
> Yes, I was certainly aware this would be a concern. I couldn't think of
> a (forward looking) clean solution, though: Right now we have only one
> use per source file (the native and compat PV entry.S), so we could use
> a context-independent label name. But as you say above, for FRED we're
> likely to get new entry points, and they're likely better placed in the
> same files.

I was going to suggest using __COUNTER__ but I've just realised why that
wont work.

But on further consideration, this might be ok for livepatching, so long
as __LINE__ is only ever used with a local label name.  By the time it
has been compiled to a binary, the label name wont survive; only the
resulting displacement will.

I think we probably want to merge this with TEMP_NAME() (perhaps as
UNIQ_NAME(), as it will have to move elsewhere to become common with
this) to avoid proliferating our livepatching reasoning.

~Andrew