MSR_SPEC_CTRL intercept

MSR_SPEC_CTRL intercept

[Boris Ostrovsky]

Looking at vmx_cpuid_policy_changed():


   if ( cp->feat.ibrsb )
        vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
    else
        vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);


Is there a reason why we are not checking cp->feat.ssbd as well?


-boris

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

Re: MSR_SPEC_CTRL intercept

[Andrew Cooper]

On 23/05/2018 22:40, Boris Ostrovsky wrote:
> Looking at vmx_cpuid_policy_changed():
>
>
>    if ( cp->feat.ibrsb )
>         vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>     else
>         vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>
>
> Is there a reason why we are not checking cp->feat.ssbd as well?

Yes.  Read the final hunk of
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9df52a25e0e95a0b9971aa2fc26c5c6a5cbdf4ef

~Andrew

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

Re: MSR_SPEC_CTRL intercept

[Boris Ostrovsky]

On 05/23/2018 05:49 PM, Andrew Cooper wrote:
> On 23/05/2018 22:40, Boris Ostrovsky wrote:
>> Looking at vmx_cpuid_policy_changed():
>>
>>
>>    if ( cp->feat.ibrsb )
>>         vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>     else
>>         vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>
>>
>> Is there a reason why we are not checking cp->feat.ssbd as well?
> Yes.  Read the final hunk of
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9df52a25e0e95a0b9971aa2fc26c5c6a5cbdf4ef

SSBD implies IBRS --- yes, that's true. But not the other way around, no?

-boris

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

Re: MSR_SPEC_CTRL intercept

[Andrew Cooper]

On 23/05/2018 22:59, Boris Ostrovsky wrote:
> On 05/23/2018 05:49 PM, Andrew Cooper wrote:
>> On 23/05/2018 22:40, Boris Ostrovsky wrote:
>>> Looking at vmx_cpuid_policy_changed():
>>>
>>>
>>>    if ( cp->feat.ibrsb )
>>>         vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>     else
>>>         vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>
>>>
>>> Is there a reason why we are not checking cp->feat.ssbd as well?
>> Yes.  Read the final hunk of
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9df52a25e0e95a0b9971aa2fc26c5c6a5cbdf4ef
> SSBD implies IBRS --- yes, that's true. But not the other way around, no?

That's not the way the dependency logic works.  That hunk says "if you
haven't got IBRSB, then you don't have STIBP or SSBD either".

It is, as documented, not completely strictly true (according to the
latest revision of the spec), but is there deliberately to simply so we
don't give the guest implausible configurations.  There is not a
processor with STIBP but without IBRSB, nor is there one with SSBD
without STIBP or IBRSB, and it is unlikely that future processors would
change this arrangement.

A side effect of prohibiting the implausible configurations is that the
logic in Xen is much more simple.

~Andrew

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

Re: MSR_SPEC_CTRL intercept

[Boris Ostrovsky]

On 05/23/2018 06:09 PM, Andrew Cooper wrote:
> On 23/05/2018 22:59, Boris Ostrovsky wrote:
>> On 05/23/2018 05:49 PM, Andrew Cooper wrote:
>>> On 23/05/2018 22:40, Boris Ostrovsky wrote:
>>>> Looking at vmx_cpuid_policy_changed():
>>>>
>>>>
>>>>    if ( cp->feat.ibrsb )
>>>>         vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>     else
>>>>         vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>
>>>>
>>>> Is there a reason why we are not checking cp->feat.ssbd as well?
>>> Yes.  Read the final hunk of
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9df52a25e0e95a0b9971aa2fc26c5c6a5cbdf4ef
>> SSBD implies IBRS --- yes, that's true. But not the other way around, no?
> That's not the way the dependency logic works.  That hunk says "if you
> haven't got IBRSB, then you don't have STIBP or SSBD either".

I guess my actual question is --- If you have IBRSB but not SSBD (which
is what we have today), do we want to intercept the access and screen
for the guest writing the SSBD bit?

-boris

>
> It is, as documented, not completely strictly true (according to the
> latest revision of the spec), but is there deliberately to simply so we
> don't give the guest implausible configurations.  There is not a
> processor with STIBP but without IBRSB, nor is there one with SSBD
> without STIBP or IBRSB, and it is unlikely that future processors would
> change this arrangement.
>
> A side effect of prohibiting the implausible configurations is that the
> logic in Xen is much more simple.
>
> ~Andrew


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

Re: MSR_SPEC_CTRL intercept

[Andrew Cooper]

On 23/05/2018 23:27, Boris Ostrovsky wrote:
> On 05/23/2018 06:09 PM, Andrew Cooper wrote:
>> On 23/05/2018 22:59, Boris Ostrovsky wrote:
>>> On 05/23/2018 05:49 PM, Andrew Cooper wrote:
>>>> On 23/05/2018 22:40, Boris Ostrovsky wrote:
>>>>> Looking at vmx_cpuid_policy_changed():
>>>>>
>>>>>
>>>>>    if ( cp->feat.ibrsb )
>>>>>         vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>>     else
>>>>>         vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>>
>>>>>
>>>>> Is there a reason why we are not checking cp->feat.ssbd as well?
>>>> Yes.  Read the final hunk of
>>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9df52a25e0e95a0b9971aa2fc26c5c6a5cbdf4ef
>>> SSBD implies IBRS --- yes, that's true. But not the other way around, no?
>> That's not the way the dependency logic works.  That hunk says "if you
>> haven't got IBRSB, then you don't have STIBP or SSBD either".
> I guess my actual question is --- If you have IBRSB but not SSBD (which
> is what we have today), do we want to intercept the access and screen
> for the guest writing the SSBD bit?

What would that achieve in practice?  TBF, I did start coding in the way
you suggest, but came to the conclusion that it was a pointless waste.

Conceptually, being able to use the SSBD bit even if you can't see it in
CPUID is no different to "knowning" that certain instructions are
implemented in the pipeline and using them anyway.  Hiding the AES CPUID
bit doesn't prevent the pipeline from executing the AES instructions if
it encountered them.

Furthermore, MSR_SPEC_CTRL is a frequently written MSR used in privilege
entry/exit points - Intercepting slows your VM down massively.

~Andrew

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

Re: MSR_SPEC_CTRL intercept

[Boris Ostrovsky]

On 05/23/2018 06:34 PM, Andrew Cooper wrote:
> On 23/05/2018 23:27, Boris Ostrovsky wrote:
>> On 05/23/2018 06:09 PM, Andrew Cooper wrote:
>>> On 23/05/2018 22:59, Boris Ostrovsky wrote:
>>>> On 05/23/2018 05:49 PM, Andrew Cooper wrote:
>>>>> On 23/05/2018 22:40, Boris Ostrovsky wrote:
>>>>>> Looking at vmx_cpuid_policy_changed():
>>>>>>
>>>>>>
>>>>>>    if ( cp->feat.ibrsb )
>>>>>>         vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>>>     else
>>>>>>         vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>>>
>>>>>>
>>>>>> Is there a reason why we are not checking cp->feat.ssbd as well?
>>>>> Yes.  Read the final hunk of
>>>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9df52a25e0e95a0b9971aa2fc26c5c6a5cbdf4ef
>>>> SSBD implies IBRS --- yes, that's true. But not the other way around, no?
>>> That's not the way the dependency logic works.  That hunk says "if you
>>> haven't got IBRSB, then you don't have STIBP or SSBD either".
>> I guess my actual question is --- If you have IBRSB but not SSBD (which
>> is what we have today), do we want to intercept the access and screen
>> for the guest writing the SSBD bit?
> What would that achieve in practice?  TBF, I did start coding in the way
> you suggest, but came to the conclusion that it was a pointless waste.
>
> Conceptually, being able to use the SSBD bit even if you can't see it in
> CPUID is no different to "knowning" that certain instructions are
> implemented in the pipeline and using them anyway.  Hiding the AES CPUID
> bit doesn't prevent the pipeline from executing the AES instructions if
> it encountered them.
>
> Furthermore, MSR_SPEC_CTRL is a frequently written MSR used in privilege
> entry/exit points - Intercepting slows your VM down massively.


Oh, right. I was thinking about catching a write to the unadvertised
SSBD bit but I  guess it doesn't buy us anything.


-boris


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

Re: MSR_SPEC_CTRL intercept

[Andrew Cooper]

On 23/05/2018 23:53, Boris Ostrovsky wrote:
> On 05/23/2018 06:34 PM, Andrew Cooper wrote:
>> On 23/05/2018 23:27, Boris Ostrovsky wrote:
>>> On 05/23/2018 06:09 PM, Andrew Cooper wrote:
>>>> On 23/05/2018 22:59, Boris Ostrovsky wrote:
>>>>> On 05/23/2018 05:49 PM, Andrew Cooper wrote:
>>>>>> On 23/05/2018 22:40, Boris Ostrovsky wrote:
>>>>>>> Looking at vmx_cpuid_policy_changed():
>>>>>>>
>>>>>>>
>>>>>>>    if ( cp->feat.ibrsb )
>>>>>>>         vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>>>>     else
>>>>>>>         vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>>>>>>>
>>>>>>>
>>>>>>> Is there a reason why we are not checking cp->feat.ssbd as well?
>>>>>> Yes.  Read the final hunk of
>>>>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9df52a25e0e95a0b9971aa2fc26c5c6a5cbdf4ef
>>>>> SSBD implies IBRS --- yes, that's true. But not the other way around, no?
>>>> That's not the way the dependency logic works.  That hunk says "if you
>>>> haven't got IBRSB, then you don't have STIBP or SSBD either".
>>> I guess my actual question is --- If you have IBRSB but not SSBD (which
>>> is what we have today), do we want to intercept the access and screen
>>> for the guest writing the SSBD bit?
>> What would that achieve in practice?  TBF, I did start coding in the way
>> you suggest, but came to the conclusion that it was a pointless waste.
>>
>> Conceptually, being able to use the SSBD bit even if you can't see it in
>> CPUID is no different to "knowning" that certain instructions are
>> implemented in the pipeline and using them anyway.  Hiding the AES CPUID
>> bit doesn't prevent the pipeline from executing the AES instructions if
>> it encountered them.
>>
>> Furthermore, MSR_SPEC_CTRL is a frequently written MSR used in privilege
>> entry/exit points - Intercepting slows your VM down massively.
>
> Oh, right. I was thinking about catching a write to the unadvertised
> SSBD bit but I  guess it doesn't buy us anything.

Indeed.  Doing so would allow you to always raise a #GP fault, but the
CPUID bit being clear means that the bit has reserved/undefined
behaviour.  It doesn't mean that a #GP fault will definitely occur if
you play with it.

~Andrew

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

Re: MSR_SPEC_CTRL intercept

[Jan Beulich]

>>> On 24.05.18 at 00:09, <andrew.cooper3@citrix.com> wrote:
> It is, as documented, not completely strictly true (according to the
> latest revision of the spec), but is there deliberately to simply so we
> don't give the guest implausible configurations.  There is not a
> processor with STIBP but without IBRSB, nor is there one with SSBD
> without STIBP or IBRSB, and it is unlikely that future processors would
> change this arrangement.

As pointed out elsewhere I believe this is a wrong dependency to make,
even if perhaps current or past Intel docs suggest so (AMD ones don't
for their versions of the features). Wile it may be the case that there's
currently no case in practice with SSBD but no IBRSB, I don't see why
this would need to remain that way. The two things are strictly
independent.

I very much think there should be an indicator of the MSR itself
existing, set to the OR of any of the bits in the MSR that are valid.
Shouldn't such a mask anyway be part of struct msr_domain_policy
(as a companion to the current value of the MSR per vCPU)?

Jan



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

Re: MSR_SPEC_CTRL intercept

[Andrew Cooper]

On 24/05/18 09:13, Jan Beulich wrote:
>>>> On 24.05.18 at 00:09, <andrew.cooper3@citrix.com> wrote:
>> It is, as documented, not completely strictly true (according to the
>> latest revision of the spec), but is there deliberately to simply so we
>> don't give the guest implausible configurations.  There is not a
>> processor with STIBP but without IBRSB, nor is there one with SSBD
>> without STIBP or IBRSB, and it is unlikely that future processors would
>> change this arrangement.
> As pointed out elsewhere I believe this is a wrong dependency to make,
> even if perhaps current or past Intel docs suggest so (AMD ones don't
> for their versions of the features). Wile it may be the case that there's
> currently no case in practice with SSBD but no IBRSB, I don't see why
> this would need to remain that way. The two things are strictly
> independent.

Features will never disappear.  x86, more than most, maintains its
backwards compatibility.

> I very much think there should be an indicator of the MSR itself
> existing, set to the OR of any of the bits in the MSR that are valid.
> Shouldn't such a mask anyway be part of struct msr_domain_policy
> (as a companion to the current value of the MSR per vCPU)?

The only think this gets you is the ability to create implausible
combinations.

I don't think it is a useful or sensible use of time trying to support
combinations which will, most likely, never exist.  Certainly not for
combinations without a plausible usecase, as these will go stale very
quickly, given the churn in this area.

~Andrew

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

Re: MSR_SPEC_CTRL intercept

[Jan Beulich]

>>> On 24.05.18 at 12:13, <andrew.cooper3@citrix.com> wrote:
> On 24/05/18 09:13, Jan Beulich wrote:
>>>>> On 24.05.18 at 00:09, <andrew.cooper3@citrix.com> wrote:
>>> It is, as documented, not completely strictly true (according to the
>>> latest revision of the spec), but is there deliberately to simply so we
>>> don't give the guest implausible configurations.  There is not a
>>> processor with STIBP but without IBRSB, nor is there one with SSBD
>>> without STIBP or IBRSB, and it is unlikely that future processors would
>>> change this arrangement.
>> As pointed out elsewhere I believe this is a wrong dependency to make,
>> even if perhaps current or past Intel docs suggest so (AMD ones don't
>> for their versions of the features). Wile it may be the case that there's
>> currently no case in practice with SSBD but no IBRSB, I don't see why
>> this would need to remain that way. The two things are strictly
>> independent.
> 
> Features will never disappear.  x86, more than most, maintains its
> backwards compatibility.

See how 3dNow insns have disappeared? CPUID flags exist for the
very purpose of allowing pieces to exist / not exist independent of
one another. For the case here, just consider the case of Intel finding
that some of their micro-architecture is vulnerable to v4 but not v2.
Why would they add IBRSB to the respective microcode, when all
they'd need there is SSBD? (Of course, leaving aside the consideration
that they had specified such a dependency themselves, and iirc they
still do to at least some degree.)

Jan



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

Re: MSR_SPEC_CTRL intercept

[Andrew Cooper]

On 24/05/18 11:23, Jan Beulich wrote:
>>>> On 24.05.18 at 12:13, <andrew.cooper3@citrix.com> wrote:
>> On 24/05/18 09:13, Jan Beulich wrote:
>>>>>> On 24.05.18 at 00:09, <andrew.cooper3@citrix.com> wrote:
>>>> It is, as documented, not completely strictly true (according to the
>>>> latest revision of the spec), but is there deliberately to simply so we
>>>> don't give the guest implausible configurations.  There is not a
>>>> processor with STIBP but without IBRSB, nor is there one with SSBD
>>>> without STIBP or IBRSB, and it is unlikely that future processors would
>>>> change this arrangement.
>>> As pointed out elsewhere I believe this is a wrong dependency to make,
>>> even if perhaps current or past Intel docs suggest so (AMD ones don't
>>> for their versions of the features). Wile it may be the case that there's
>>> currently no case in practice with SSBD but no IBRSB, I don't see why
>>> this would need to remain that way. The two things are strictly
>>> independent.
>> Features will never disappear.  x86, more than most, maintains its
>> backwards compatibility.
> See how 3dNow insns have disappeared?

And LWIP, XOP (although XOP hasn't actually disappeared because it turns
out that Zen pipeline still execute FMA4 instruction).

> CPUID flags exist for the
> very purpose of allowing pieces to exist / not exist independent of
> one another.

Right, but you've picked examples that are independent of each other.

> For the case here, just consider the case of Intel finding
> that some of their micro-architecture is vulnerable to v4 but not v2.
> Why would they add IBRSB to the respective microcode, when all
> they'd need there is SSBD?

Because all of these features centre around the same MSR.

The cores requiring SSBD are subset of those requiring IBRSB/STIBP, so
this doesn't matter in this specific case.

However, as already seen with IBRS and STIBP being a disjoin set, the
implementation is specifically to allow the unnecessary bits to function
as a compatible no-op, for virtualisation purposes.

So yes, the current behaviour specifically isn't as flexible as we could
be (under the latest revision of the spec), but it is specifically
called out out as a simplifying properly, with an explanation of why
this is a safe and sensible approach to take.

~Andrew

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

Re: MSR_SPEC_CTRL intercept

[Jan Beulich]

>>> On 25.05.18 at 17:25, <andrew.cooper3@citrix.com> wrote:
> On 24/05/18 11:23, Jan Beulich wrote:
>>>>> On 24.05.18 at 12:13, <andrew.cooper3@citrix.com> wrote:
>>> On 24/05/18 09:13, Jan Beulich wrote:
>>>>>>> On 24.05.18 at 00:09, <andrew.cooper3@citrix.com> wrote:
>>>>> It is, as documented, not completely strictly true (according to the
>>>>> latest revision of the spec), but is there deliberately to simply so we
>>>>> don't give the guest implausible configurations.  There is not a
>>>>> processor with STIBP but without IBRSB, nor is there one with SSBD
>>>>> without STIBP or IBRSB, and it is unlikely that future processors would
>>>>> change this arrangement.
>>>> As pointed out elsewhere I believe this is a wrong dependency to make,
>>>> even if perhaps current or past Intel docs suggest so (AMD ones don't
>>>> for their versions of the features). Wile it may be the case that there's
>>>> currently no case in practice with SSBD but no IBRSB, I don't see why
>>>> this would need to remain that way. The two things are strictly
>>>> independent.
>>> Features will never disappear.  x86, more than most, maintains its
>>> backwards compatibility.
>> See how 3dNow insns have disappeared?
> 
> And LWIP, XOP (although XOP hasn't actually disappeared because it turns
> out that Zen pipeline still execute FMA4 instruction).
> 
>> CPUID flags exist for the
>> very purpose of allowing pieces to exist / not exist independent of
>> one another.
> 
> Right, but you've picked examples that are independent of each other.

Of course, because technically SSBD and IBRSB are, too.

>> For the case here, just consider the case of Intel finding
>> that some of their micro-architecture is vulnerable to v4 but not v2.
>> Why would they add IBRSB to the respective microcode, when all
>> they'd need there is SSBD?
> 
> Because all of these features centre around the same MSR.
> 
> The cores requiring SSBD are subset of those requiring IBRSB/STIBP, so
> this doesn't matter in this specific case.

If that's indeed the case, then I'd say this is pure luck / coincidence.

> However, as already seen with IBRS and STIBP being a disjoin set, the
> implementation is specifically to allow the unnecessary bits to function
> as a compatible no-op, for virtualisation purposes.
> 
> So yes, the current behaviour specifically isn't as flexible as we could
> be (under the latest revision of the spec), but it is specifically
> called out out as a simplifying properly, with an explanation of why
> this is a safe and sensible approach to take.

And I'm not saying we absolutely need to change how we do things. I'm
just saying that this is not the best way of putting it (spec-wise).

Jan



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