Re: [PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 7/8] x86/spec-ctrl: Issue VERW during IST exit to Xen
Date: Fri, 15 Sep 2023 09:15:47 +0200	[thread overview]
Message-ID: <7b4c0bfd-3eb2-3d0a-859a-b94b12397876@suse.com> (raw)
In-Reply-To: <22864483-9cb8-7220-f82e-883fd2ef617a@citrix.com>

On 14.09.2023 21:49, Andrew Cooper wrote:
> On 14/09/2023 11:01 am, Jan Beulich wrote:
>> On 13.09.2023 22:27, Andrew Cooper wrote:
>>> There is a corner case where e.g. an NMI hitting an exit-to-guest path after
>>> SPEC_CTRL_EXIT_TO_* would have run the entire NMI handler *after* the VERW
>>> flush to scrub potentially sensitive data from uarch buffers.
>>>
>>> In order to compensate, issue VERW when exiting to Xen from an IST entry.
>>>
>>> SPEC_CTRL_EXIT_TO_XEN already has two reads of spec_ctrl_flags off the stack,
>>> and we're about to add a third.  Load the field into %ebx, and list the
>>> register as clobbered.
>>>
>>> %r12 has been arranged to be the ist_exit signal, so add this as an input
>>> dependency and use it to identify when to issue a VERW.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> While looking technically okay, I still have a couple of remarks:
>>
>>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> @@ -344,10 +344,12 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>>   */
>>>  .macro SPEC_CTRL_EXIT_TO_XEN
>>>  /*
>>> - * Requires %r14=stack_end
>>> - * Clobbers %rax, %rcx, %rdx
>>> + * Requires %r12=ist_exit, %r14=stack_end
>>> + * Clobbers %rax, %rbx, %rcx, %rdx
>> While I'd generally be a little concerned of the growing dependency and
>> clobber lists, there being a single use site makes this pretty okay. The
>> macro invocation being immediately followed by RESTORE_ALL effectively
>> means we can clobber almost everything here.
>>
>> As to register usage and my comment on patch 5: There's no real need
>> to switch %rbx to %r14 there if I'm not mistaken
> 
> As said, it's about consistency of the helpers.

While I'm not entirely happy with this, ...

>>> @@ -367,8 +369,16 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>>  
>>>  .L\@_skip_sc_msr:
>>>  
>>> -    /* TODO VERW */
>>> +    test %r12, %r12
>>> +    jz .L\@_skip_ist_exit
>>> +
>>> +    /* Logically DO_SPEC_CTRL_COND_VERW but without the %rsp=cpuinfo dependency */
>>> +    testb $SCF_verw, %bl
>>> +    jz .L\@_verw_skip
>>> +    verw STACK_CPUINFO_FIELD(verw_sel)(%r14)
>>> +.L\@_verw_skip:
>> Nit: .L\@_skip_verw would be more consistent with the other label names.
> 
> So it would.  I'll tweak.

... then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan