Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-23 Thread Andrew Cooper 
On 23/01/18 02:19, Boris Ostrovsky wrote:
>
>
> On 01/22/2018 07:17 PM, Andrew Cooper wrote:
>> On 22/01/2018 22:27, Boris Ostrovsky wrote:
>>> On 01/19/2018 08:36 AM, Andrew Cooper wrote:
 On 19/01/18 11:43, Jan Beulich wrote:

>> @@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
>>   .Lvmx_vmentry_fail:
>>   sti
>>   SAVE_ALL
>> +
>> +    SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob:
>> acd */
> I think the use of the PV variant here requires a comment.
 Oh.  It used to have one...  I'll try to find it.
>>> I, in fact, meant to ask about this for a long time and always forgot.
>>> Perhaps your comment will say more than just why a PV variant is used
>>> here but in case it won't --- why do we have *any* mitigation here? We
>>> are never returning to the guest, do we?
>>
>> We never return to *this* guest, but we are still open to abuse from a
>> separate hyperthread, so still need to set SPEC_CTRL.IBRS if we are
>> using IBRS for safety.  (If we are using lfence+jmp or repoline then we
>> don't need this change, but its not a hotpath so doesn't warrant yet
>> another variant of SPEC_CTRL_ENTRY_FROM_*.)
>
>
> We wrote IBRS during VMEXIT. I thought this serves as barrier for all
> preceding predictions (both threads) from lower protection mode.

There is no guarantee that a sequence which sets IBRS, then clears it,
serves to flush older predictions.

In practice, some implementations of IBRS are a flush internally, and
some are a disable internally.  This is why the semantics require you to
rewrite it the value 1 on entry to a higher mode (even if it was
previously set), and keep it set for the duration of protection you want.

> Is the concern here that SPEC_CTRL_EXIT_TO_GUEST (before VMENTER) may
> set IBRS to 0 and *that* will open hypervisor to other thread's mischief?

Correct.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-22 Thread Boris Ostrovsky 



On 01/22/2018 07:17 PM, Andrew Cooper wrote:

On 22/01/2018 22:27, Boris Ostrovsky wrote:

On 01/19/2018 08:36 AM, Andrew Cooper wrote:

On 19/01/18 11:43, Jan Beulich wrote:


@@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
  .Lvmx_vmentry_fail:
  sti
  SAVE_ALL
+
+SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */

I think the use of the PV variant here requires a comment.

Oh.  It used to have one...  I'll try to find it.

I, in fact, meant to ask about this for a long time and always forgot.
Perhaps your comment will say more than just why a PV variant is used
here but in case it won't --- why do we have *any* mitigation here? We
are never returning to the guest, do we?


We never return to *this* guest, but we are still open to abuse from a
separate hyperthread, so still need to set SPEC_CTRL.IBRS if we are
using IBRS for safety.  (If we are using lfence+jmp or repoline then we
don't need this change, but its not a hotpath so doesn't warrant yet
another variant of SPEC_CTRL_ENTRY_FROM_*.)



We wrote IBRS during VMEXIT. I thought this serves as barrier for all 
preceding predictions (both threads) from lower protection mode.


Is the concern here that SPEC_CTRL_EXIT_TO_GUEST (before VMENTER) may 
set IBRS to 0 and *that* will open hypervisor to other thread's mischief?


-boris

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-22 Thread Andrew Cooper 
On 22/01/2018 22:27, Boris Ostrovsky wrote:
> On 01/19/2018 08:36 AM, Andrew Cooper wrote:
>> On 19/01/18 11:43, Jan Beulich wrote:
>>
 @@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
  .Lvmx_vmentry_fail:
  sti
  SAVE_ALL
 +
 +SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
>>> I think the use of the PV variant here requires a comment.
>> Oh.  It used to have one...  I'll try to find it.
> I, in fact, meant to ask about this for a long time and always forgot.
> Perhaps your comment will say more than just why a PV variant is used
> here but in case it won't --- why do we have *any* mitigation here? We
> are never returning to the guest, do we?

We never return to *this* guest, but we are still open to abuse from a
separate hyperthread, so still need to set SPEC_CTRL.IBRS if we are
using IBRS for safety.  (If we are using lfence+jmp or repoline then we
don't need this change, but its not a hotpath so doesn't warrant yet
another variant of SPEC_CTRL_ENTRY_FROM_*.)

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-22 Thread Boris Ostrovsky 
On 01/19/2018 08:36 AM, Andrew Cooper wrote:
> On 19/01/18 11:43, Jan Beulich wrote:
>
>>> @@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
>>>  .Lvmx_vmentry_fail:
>>>  sti
>>>  SAVE_ALL
>>> +
>>> +SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
>> I think the use of the PV variant here requires a comment.
> Oh.  It used to have one...  I'll try to find it.

I, in fact, meant to ask about this for a long time and always forgot.
Perhaps your comment will say more than just why a PV variant is used
here but in case it won't --- why do we have *any* mitigation here? We
are never returning to the guest, do we?

-boris

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-22 Thread Jan Beulich 
>>> On 22.01.18 at 12:42,  wrote:
> On 19/01/18 13:51, Jan Beulich wrote:
> On 19.01.18 at 14:36,  wrote:
>>> On 19/01/18 11:43, Jan Beulich wrote:
>>> On 18.01.18 at 16:46,  wrote:
> @@ -729,6 +760,9 @@ ENTRY(nmi)
>  handle_ist_exception:
>  SAVE_ALL CLAC
>  
> +SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, Clob: acd */
> +/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. 
> */
 Following my considerations towards alternative patching to
 eliminate as much overhead as possible from the Meltdown
 band-aid in case it is being disabled, I'm rather hesitant to see any
 patchable code being introduced into the NMI/#MC entry paths
 without the patching logic first being made safe in this regard.
 Exceptions coming here aren't very frequent (except perhaps on
 hardware about to die), so the path isn't performance critical.
 Therefore I think we should try to avoid any patching here, and
 just conditionals instead. This in fact is one of the reasons why I
 didn't want to macro-ize the assembly additions done in the
 Meltdown band-aid.

 I do realize that this then also affects the exit-to-Xen path,
 which I agree is less desirable to use conditionals on.
>>> While I agree that our lack of IST-safe patching is a problem, these
>>> alternative points are already present on the NMI and MCE paths, and
>>> need to be.  As a result, the DF handler is in no worse of a position. 
>>> As a perfect example, observe the CLAC in context.
>> Oh, indeed. We should change that.
>>
>>> I could perhaps be talked into making a SPEC_CTRL_ENTRY_FROM_IST variant
>>> which doesn't use alternatives (but IMO this is pointless in the
>>> presence of CLAC), but still don't think it is reasonable to treat DF
>>> differently to NMI/MCE.
>> #DF is debatable: On one hand I can see that if things go wrong,
>> it can equally be raised at any time. Otoh #MC and even more so
>> NMI can be raised _without_ things going (fatally) wrong, i.e. the
>> patching may break a boot which would otherwise have succeeded
>> (whereas the #DF would make the boot fail anyway).
> 
> I don't see a conclusion here, or a reason for treating #DF differently
> to NMI or #MC.

Odd - I thought my reply was pretty clear in this regard. I have
no good idea how to word it differently. Furthermore the goal
of the reply was not to settle on how to treat #DF, but to try
to convince you to avoid adding more patch points to the NMI /
#MC path (if you want #DF treated similarly, I wouldn't
object patching to be avoided there too).

> There is currently a very very slim race on boot where an NMI or #MC
> hitting the main application of alternatives may cause Xen to explode. 
> This has been the case since alternatives were introduced, and this
> patch doesn't make the problem meaningfully worse.

SMAP patching affects 3 bytes (and I'm intending to put together a
patch removing that patching from the NMI / #MC path), while you
add patching of quite a few more bytes, increasing the risk
accordingly.

If you really don't want to switch away from the patching approach,
I won't refuse to ack the patch. But it'll mean subsequent changes
will be more intrusive, to get this converted to conditionals instead
(unless someone has _immediate_ plans to deal with the issues in
the patching logic itself).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-22 Thread Andrew Cooper 
On 19/01/18 13:51, Jan Beulich wrote:
 On 19.01.18 at 14:36,  wrote:
>> On 19/01/18 11:43, Jan Beulich wrote:
>> On 18.01.18 at 16:46,  wrote:
 @@ -729,6 +760,9 @@ ENTRY(nmi)
  handle_ist_exception:
  SAVE_ALL CLAC
  
 +SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, Clob: acd */
 +/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. 
 */
>>> Following my considerations towards alternative patching to
>>> eliminate as much overhead as possible from the Meltdown
>>> band-aid in case it is being disabled, I'm rather hesitant to see any
>>> patchable code being introduced into the NMI/#MC entry paths
>>> without the patching logic first being made safe in this regard.
>>> Exceptions coming here aren't very frequent (except perhaps on
>>> hardware about to die), so the path isn't performance critical.
>>> Therefore I think we should try to avoid any patching here, and
>>> just conditionals instead. This in fact is one of the reasons why I
>>> didn't want to macro-ize the assembly additions done in the
>>> Meltdown band-aid.
>>>
>>> I do realize that this then also affects the exit-to-Xen path,
>>> which I agree is less desirable to use conditionals on.
>> While I agree that our lack of IST-safe patching is a problem, these
>> alternative points are already present on the NMI and MCE paths, and
>> need to be.  As a result, the DF handler is in no worse of a position. 
>> As a perfect example, observe the CLAC in context.
> Oh, indeed. We should change that.
>
>> I could perhaps be talked into making a SPEC_CTRL_ENTRY_FROM_IST variant
>> which doesn't use alternatives (but IMO this is pointless in the
>> presence of CLAC), but still don't think it is reasonable to treat DF
>> differently to NMI/MCE.
> #DF is debatable: On one hand I can see that if things go wrong,
> it can equally be raised at any time. Otoh #MC and even more so
> NMI can be raised _without_ things going (fatally) wrong, i.e. the
> patching may break a boot which would otherwise have succeeded
> (whereas the #DF would make the boot fail anyway).

I don't see a conclusion here, or a reason for treating #DF differently
to NMI or #MC.

There is currently a very very slim race on boot where an NMI or #MC
hitting the main application of alternatives may cause Xen to explode. 
This has been the case since alternatives were introduced, and this
patch doesn't make the problem meaningfully worse.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-19 Thread Jan Beulich 
>>> On 19.01.18 at 14:36,  wrote:
> On 19/01/18 11:43, Jan Beulich wrote:
> On 18.01.18 at 16:46,  wrote:
>>> @@ -729,6 +760,9 @@ ENTRY(nmi)
>>>  handle_ist_exception:
>>>  SAVE_ALL CLAC
>>>  
>>> +SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, Clob: acd */
>>> +/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>> Following my considerations towards alternative patching to
>> eliminate as much overhead as possible from the Meltdown
>> band-aid in case it is being disabled, I'm rather hesitant to see any
>> patchable code being introduced into the NMI/#MC entry paths
>> without the patching logic first being made safe in this regard.
>> Exceptions coming here aren't very frequent (except perhaps on
>> hardware about to die), so the path isn't performance critical.
>> Therefore I think we should try to avoid any patching here, and
>> just conditionals instead. This in fact is one of the reasons why I
>> didn't want to macro-ize the assembly additions done in the
>> Meltdown band-aid.
>>
>> I do realize that this then also affects the exit-to-Xen path,
>> which I agree is less desirable to use conditionals on.
> 
> While I agree that our lack of IST-safe patching is a problem, these
> alternative points are already present on the NMI and MCE paths, and
> need to be.  As a result, the DF handler is in no worse of a position. 
> As a perfect example, observe the CLAC in context.

Oh, indeed. We should change that.

> I could perhaps be talked into making a SPEC_CTRL_ENTRY_FROM_IST variant
> which doesn't use alternatives (but IMO this is pointless in the
> presence of CLAC), but still don't think it is reasonable to treat DF
> differently to NMI/MCE.

#DF is debatable: On one hand I can see that if things go wrong,
it can equally be raised at any time. Otoh #MC and even more so
NMI can be raised _without_ things going (fatally) wrong, i.e. the
patching may break a boot which would otherwise have succeeded
(whereas the #DF would make the boot fail anyway).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-19 Thread Andrew Cooper 
On 19/01/18 11:43, Jan Beulich wrote:
 On 18.01.18 at 16:46,  wrote:
>> We need to be able to either set or clear IBRS in Xen context, as well as
>> restore appropriate guest values in guest context.  See the documentation in
>> asm-x86/spec_ctrl_asm.h for details.
>>
>> Writes to %cr3 are slower when SPEC_CTRL.IBRS is set, so the
>> SPEC_CTRL_{ENTRY/EXIT}* positioning is important, and optimised for the
>> expected common case of Xen not using IBRS itself.
> And this expectation is because on pre-Skylake we're fine using just
> retpoline, and we expect post-Skylake to not have the issue anymore?
> Such reasoning would help being spelled out here.

Kaby Lake is in the same bucket as Skylake.  Coffee Lake and Cannon Lake
are unknown quantities.

Intel have stated that the expect these issues to be resolved on the
same hardware as CET, as retpoline is a deliberate ROP-gadget and won't
function with Controlflow Enforcement Technology enabled.

As far as I'm aware, CET is expected in Ice Lake, but who knows what
this issue has done to Intel's release schedule.

> As to the behavior of CR3 writes - is this written down anywhere in
> Intel's and/or AMD's docs? Or else, how do you know this is uniformly
> the case on all past, current, and future hardware?

AMD don't implement IBRS, so this isn't applicable to them.

I'm not sure if this is stated in any public documentation.  I'll see
about poking people...

Irrespective, it can be demonstrated trivially with some timing analysis.

>> @@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
>>  .Lvmx_vmentry_fail:
>>  sti
>>  SAVE_ALL
>> +
>> +SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
> I think the use of the PV variant here requires a comment.

Oh.  It used to have one...  I'll try to find it.

>
>> @@ -142,6 +146,13 @@ ENTRY(compat_restore_all_guest)
>>  .popsection
>>  or$X86_EFLAGS_IF,%r11
>>  mov   %r11d,UREGS_eflags(%rsp)
>> +
>> +mov VCPU_arch_msr(%rbx), %rax
>> +mov VCPUMSR_spec_ctrl_raw(%rax), %eax
> I understand that it's code like this which is missing from the SVM
> and VMX exit paths.

Correct.

>
>> @@ -729,6 +760,9 @@ ENTRY(nmi)
>>  handle_ist_exception:
>>  SAVE_ALL CLAC
>>  
>> +SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, Clob: acd */
>> +/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> Following my considerations towards alternative patching to
> eliminate as much overhead as possible from the Meltdown
> band-aid in case it is being disabled, I'm rather hesitant to see any
> patchable code being introduced into the NMI/#MC entry paths
> without the patching logic first being made safe in this regard.
> Exceptions coming here aren't very frequent (except perhaps on
> hardware about to die), so the path isn't performance critical.
> Therefore I think we should try to avoid any patching here, and
> just conditionals instead. This in fact is one of the reasons why I
> didn't want to macro-ize the assembly additions done in the
> Meltdown band-aid.
>
> I do realize that this then also affects the exit-to-Xen path,
> which I agree is less desirable to use conditionals on.

While I agree that our lack of IST-safe patching is a problem, these
alternative points are already present on the NMI and MCE paths, and
need to be.  As a result, the DF handler is in no worse of a position. 
As a perfect example, observe the CLAC in context.

I could perhaps be talked into making a SPEC_CTRL_ENTRY_FROM_IST variant
which doesn't use alternatives (but IMO this is pointless in the
presence of CLAC), but still don't think it is reasonable to treat DF
differently to NMI/MCE.

>
>> --- /dev/null
>> +++ b/xen/include/asm-x86/spec_ctrl_asm.h
>> @@ -0,0 +1,227 @@
>> +/**
>> + * include/asm-x86/spec_ctrl.h
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; If not, see .
>> + *
>> + * Copyright (c) 2017-2018 Citrix Systems Ltd.
>> + */
>> +
>> +#ifndef __X86_SPEC_CTRL_ASM_H__
>> +#define __X86_SPEC_CTRL_ASM_H__
>> +
>> +#ifdef __ASSEMBLY__
>> +#include 
>> +
>> +/*
>> + * Saving and restoring MSR_SPEC_CTRL state is a little tricky.
>> + *
>> + * We want the guests choice of SPEC_CTRL while in guest context, and IBRS
>> + *

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-19 Thread Jan Beulich 
>>> On 18.01.18 at 16:46,  wrote:
> We need to be able to either set or clear IBRS in Xen context, as well as
> restore appropriate guest values in guest context.  See the documentation in
> asm-x86/spec_ctrl_asm.h for details.
> 
> Writes to %cr3 are slower when SPEC_CTRL.IBRS is set, so the
> SPEC_CTRL_{ENTRY/EXIT}* positioning is important, and optimised for the
> expected common case of Xen not using IBRS itself.

And this expectation is because on pre-Skylake we're fine using just
retpoline, and we expect post-Skylake to not have the issue anymore?
Such reasoning would help being spelled out here.

As to the behavior of CR3 writes - is this written down anywhere in
Intel's and/or AMD's docs? Or else, how do you know this is uniformly
the case on all past, current, and future hardware?

> @@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
>  .Lvmx_vmentry_fail:
>  sti
>  SAVE_ALL
> +
> +SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */

I think the use of the PV variant here requires a comment.

> @@ -142,6 +146,13 @@ ENTRY(compat_restore_all_guest)
>  .popsection
>  or$X86_EFLAGS_IF,%r11
>  mov   %r11d,UREGS_eflags(%rsp)
> +
> +mov VCPU_arch_msr(%rbx), %rax
> +mov VCPUMSR_spec_ctrl_raw(%rax), %eax

I understand that it's code like this which is missing from the SVM
and VMX exit paths.

> @@ -729,6 +760,9 @@ ENTRY(nmi)
>  handle_ist_exception:
>  SAVE_ALL CLAC
>  
> +SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, Clob: acd */
> +/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */

Following my considerations towards alternative patching to
eliminate as much overhead as possible from the Meltdown
band-aid in case it is being disabled, I'm rather hesitant to see any
patchable code being introduced into the NMI/#MC entry paths
without the patching logic first being made safe in this regard.
Exceptions coming here aren't very frequent (except perhaps on
hardware about to die), so the path isn't performance critical.
Therefore I think we should try to avoid any patching here, and
just conditionals instead. This in fact is one of the reasons why I
didn't want to macro-ize the assembly additions done in the
Meltdown band-aid.

I do realize that this then also affects the exit-to-Xen path,
which I agree is less desirable to use conditionals on.

> --- /dev/null
> +++ b/xen/include/asm-x86/spec_ctrl_asm.h
> @@ -0,0 +1,227 @@
> +/**
> + * include/asm-x86/spec_ctrl.h
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see .
> + *
> + * Copyright (c) 2017-2018 Citrix Systems Ltd.
> + */
> +
> +#ifndef __X86_SPEC_CTRL_ASM_H__
> +#define __X86_SPEC_CTRL_ASM_H__
> +
> +#ifdef __ASSEMBLY__
> +#include 
> +
> +/*
> + * Saving and restoring MSR_SPEC_CTRL state is a little tricky.
> + *
> + * We want the guests choice of SPEC_CTRL while in guest context, and IBRS
> + * (set or clear, depending on the hardware) while running in Xen context.

To me this continues to be somewhat strange to read. One possibility
to improve it would be to move the opening parenthesis after "set or
clear", another would be to say "..., and Xen's choice (set or ...".
But you're the native speaker, so I'm open to other alternatives you
may consider even better.

> + * Therefore, a simplistic algorithm is:
> + *
> + *  - Set/clear IBRS on entry to Xen
> + *  - Set the guests' choice on exit to guest
> + *  - Leave SPEC_CTRL unchanged on exit to xen
> + *
> + * There are two complicating factors:
> + *  1) HVM guests can have direct access to the MSR, so it can change
> + * behind Xen's back.
> + *  2) An NMI or MCE can interrupt at any point, including early in the entry
> + * path, or late in the exit path after restoring the guest value.  This
> + * will corrupt the guest value.
> + *
> + * Factor 1 is dealt with by relying on NMIs/MCEs being blocked immediately
> + * after VMEXIT.  The VMEXIT-specific code reads MSR_SPEC_CTRL and updates
> + * current before loading Xen's MSR_SPEC_CTRL setting.
> + *
> + * Factor 2 is harder.  We maintain a shadow_spec_ctrl value, and
> + * use_shadow_spec_ctrl boolean per cpu.  The synchronous use is:
> + *
> + *  1) Store guest value in shadow_spec_ctrl
> + *  2) Set

Re: [Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-19 Thread Andrew Cooper 
On 18/01/18 15:46, Andrew Cooper wrote:
> We need to be able to either set or clear IBRS in Xen context, as well as
> restore appropriate guest values in guest context.  See the documentation in
> asm-x86/spec_ctrl_asm.h for details.
>
> Writes to %cr3 are slower when SPEC_CTRL.IBRS is set, so the
> SPEC_CTRL_{ENTRY/EXIT}* positioning is important, and optimised for the
> expected common case of Xen not using IBRS itself.
>
> There is a semi-unrelated bugfix, where various asm_defn.h macros have a
> hidden dependency on PAGE_SIZE, which results in an assembler error if used in
> a .macro definition.
>
> Signed-off-by: Andrew Cooper 

/sigh - It appears that I've missed two hunks from this patch, and as a
result, its subtly broken for HVM guests.

I'll post an incremental version when I've unbroken my automatic tests
and refreshed the XTF unit tests for these MSRs.

It is expected to be two extra mov's in the vmx/svm exit paths, so the
rest of the patch is ok for review in general.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v9 06/11] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point

2018-01-18 Thread Andrew Cooper 
We need to be able to either set or clear IBRS in Xen context, as well as
restore appropriate guest values in guest context.  See the documentation in
asm-x86/spec_ctrl_asm.h for details.

Writes to %cr3 are slower when SPEC_CTRL.IBRS is set, so the
SPEC_CTRL_{ENTRY/EXIT}* positioning is important, and optimised for the
expected common case of Xen not using IBRS itself.

There is a semi-unrelated bugfix, where various asm_defn.h macros have a
hidden dependency on PAGE_SIZE, which results in an assembler error if used in
a .macro definition.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 

v7:
 * Spelling fixes
 * Reposition the semicolon fix.
v9:
 * Rebase over XPTI.  Moderate changes in the exit-to-PV paths.
 * Swap cmpw for testb when looking for cpl0
---
 xen/arch/x86/hvm/svm/entry.S|   8 +-
 xen/arch/x86/hvm/vmx/entry.S|  11 ++
 xen/arch/x86/setup.c|   1 +
 xen/arch/x86/smpboot.c  |   2 +
 xen/arch/x86/x86_64/asm-offsets.c   |   6 +
 xen/arch/x86/x86_64/compat/entry.S  |  14 +++
 xen/arch/x86/x86_64/entry.S |  34 ++
 xen/include/asm-x86/asm_defns.h |   3 +
 xen/include/asm-x86/current.h   |   6 +
 xen/include/asm-x86/nops.h  |   6 +
 xen/include/asm-x86/spec_ctrl.h |   9 ++
 xen/include/asm-x86/spec_ctrl_asm.h | 227 
 12 files changed, 326 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/asm-x86/spec_ctrl_asm.h

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index df86da0..fb1048b 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -79,6 +79,9 @@ UNLIKELY_END(svm_trace)
 or   $X86_EFLAGS_MBS,%rax
 mov  %rax,VMCB_rflags(%rcx)
 
+/* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+SPEC_CTRL_EXIT_TO_GUEST /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+
 pop  %r15
 pop  %r14
 pop  %r13
@@ -101,8 +104,11 @@ UNLIKELY_END(svm_trace)
 SAVE_ALL
 
 GET_CURRENT(bx)
-mov  VCPU_svm_vmcb(%rbx),%rcx
 
+SPEC_CTRL_ENTRY_FROM_VMEXIT /* Req: b=curr %rsp=regs/cpuinfo, Clob: 
acd */
+/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
+mov  VCPU_svm_vmcb(%rbx),%rcx
 movb $0,VCPU_svm_vmcb_in_sync(%rbx)
 mov  VMCB_rax(%rcx),%rax
 mov  %rax,UREGS_rax(%rsp)
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index b2f98be..21e959f 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -38,6 +38,9 @@ ENTRY(vmx_asm_vmexit_handler)
 movb $1,VCPU_vmx_launched(%rbx)
 mov  %rax,VCPU_hvm_guest_cr2(%rbx)
 
+SPEC_CTRL_ENTRY_FROM_VMEXIT /* Req: b=curr %rsp=regs/cpuinfo, Clob: 
acd */
+/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
 mov  %rsp,%rdi
 call vmx_vmexit_handler
 
@@ -68,6 +71,10 @@ UNLIKELY_END(realmode)
 call vmx_vmenter_helper
 test %al, %al
 jz .Lvmx_vmentry_restart
+
+/* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+SPEC_CTRL_EXIT_TO_GUEST /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+
 mov  VCPU_hvm_guest_cr2(%rbx),%rax
 
 pop  %r15
@@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
 .Lvmx_vmentry_fail:
 sti
 SAVE_ALL
+
+SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
+/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
 call vmx_vmentry_failure
 BUG  /* vmx_vmentry_failure() shouldn't return. */
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b6888c7..41afd2a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -678,6 +678,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 set_processor_id(0);
 set_current(INVALID_VCPU); /* debug sanity. */
 idle_vcpu[0] = current;
+init_shadow_spec_ctrl_state();
 
 percpu_init_areas();
 
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 2cdd431..196ee7e 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -309,6 +310,7 @@ void start_secondary(void *unused)
 set_current(idle_vcpu[cpu]);
 this_cpu(curr_vcpu) = idle_vcpu[cpu];
 rdmsrl(MSR_EFER, this_cpu(efer));
+init_shadow_spec_ctrl_state();
 
 /*
  * Just as during early bootstrap, it is convenient here to disable
diff --git a/xen/arch/x86/x86_64/asm-offsets.c 
b/xen/arch/x86/x86_64/asm-offsets.c
index b1a4310..17f1d77 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -88,6 +88,7 @@ void __dummy__(void)
 OFFSET(VCPU_kernel_ss, struct vcpu, arch.pv_vcpu.kernel_ss);
 OFFSET(VCPU_iopl, struct vcpu, arch.pv_vcpu.iopl);