Re: [Xen-devel] [PATCH v6.5 20/26] x86: Protect unaware domains from meddling hyperthreads

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 01:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Fundamentally (as before)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
However:

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2027,6 +2027,25 @@ int domain_relinquish_resources(struct domain *d)
>   */
>  void cpuid_policy_updated(struct vcpu *v)
>  {
> +    const struct cpuid_policy *cp = v->domain->arch.cpuid;
> +    struct msr_vcpu_policy *vp = v->arch.msr;
> +
> +    /*
> +     * For guests which know about IBRS but are not told about STIBP running
> +     * on hardware supporting hyperthreading, the guest doesn't know to
> +     * protect itself fully.  (Such a guest won't be permitted direct access
> +     * to the MSR.)  Have Xen fill in the gaps, so an unaware guest can't be
> +     * interfered with by a meddling guest on an adjacent hyperthread.
> +     */
> +    if ( cp->feat.ibrsb )
> +    {
> +        if ( !cp->feat.stibp && cpu_has_stibp &&
> +             !(vp->spec_ctrl.guest & (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) )
> +            vp->spec_ctrl.host = SPEC_CTRL_STIBP;
> +        else
> +            vp->spec_ctrl.host = vp->spec_ctrl.guest;

This code is so similar to ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -181,7 +181,20 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>                       (cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
>              goto gp_fault; /* Rsvd bit set? */
>          vp->spec_ctrl.guest = val;
> -        vp->spec_ctrl.host  = val;
> +
> +        /*
> +         * For guests which are not told about STIBP, running on hardware
> +         * supporting hyperthreading, the guest doesn't know to protect 
> itself
> +         * fully.  (Such a guest won't be permitted direct access to the 
> MSR.)
> +         * When IBRS is not in force, have Xen fill in the gaps, so an 
> unaware
> +         * guest can't be interfered with by a meddling guest on an adjacent
> +         * hyperthread.
> +         */
> +        if ( !cp->feat.stibp && cpu_has_stibp &&
> +             !(val & (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) )
> +            vp->spec_ctrl.host = SPEC_CTRL_STIBP;
> +        else
> +            vp->spec_ctrl.host = val;

... this that I think a helper function would be warranted, unless you
have reasons to believe that future changes might break the
similarity.

I'm also a little puzzled by you checking SPEC_CTRL_STIBP there -
this bit ought to be clear when !cp->feat.stibp due to the earlier
reserved bit check (part of which is even visible in context above).
IOW the check is not wrong, but perhaps misleading. You had
replied to this remark with

"The SPEC_CTRL_STIBP check exists solely because of v3 review which
 objected to me implying a link between IBRS and STIPB."

I'm afraid I don't understand, for two reasons:

1) The change to guest_wrmsr() is the same here as it was in v3.
The difference is that now the change to cpuid_policy_updated()
is rather more similar to that to guest_wrmsr().

2) Going through the mails I've sent, it must have been review
comments by someone else, which I didn't get to see. Hence I
understand neither the context nor the reasons.

Jan


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