[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 27 Jan 2022 14:47:45 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=A296S82GnOFGZIwRRMQW9CHBp1hVssunKqLk7bn85Ko=; b=bkauZaTKSYPJP5jpZhRLNot+8iaeX3OIySjEfkyxCJGwVD7WTJXbRPUCsKpt8XvLHnjZbgF4ZhXQEld9Le0KO0Vdygx6vyCKsMXuphTClBXgCQxbeBGI4F4T4ed6F4GljWuuzjNVLi74oGXnf6PQ4+WTDQ8uWxZntu/BK13XeuiqsTtAdTLyenNXSbWbxUAiEWw4cSTFVWWkk0XO7yOCY3t6YXi8VZIVSpcKM/2qEks5y2rYoZt/W+fTujySYL4kC8Jh4qv//98pRYSOMZQ+ln0M7gNGtkCGMB/z6U5fKfdfFa34v7ys0nRc15hU3FegORu8yDAxCaIxAYsTZqlCBQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YrLPpDRdgJar0XTDMmhut+97ZIuC4vP0ypXfQiaYOb51A/JSKmDMII/KMJuSD4oYt0uyyExANtSz8V15d1RSA6RJ8pcgPE/wyy4BRTK22kLRBC4/3urCtz0N9Epbbbaq5OA1vZBgsstf1xOJGGV6bnReLIg/wujxrzAfu8srz++AcrF9ctGjOiyQZPJ/Q5aZFoErj8DckkZqP/EwSSOQyDYnV0qw4lyno3w6DX6I+0gDQ0S6Q3WZFU+C40tvBDD8jFABQJnYmrT0jU/F+cyOG8s0BH7hoBqVh6dqNZ85VSdPPKy0tVU8U/GWf6MU3vPuCqfIjmwcu1pTl+6iUIaPgg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 27 Jan 2022 13:47:59 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 26.01.2022 09:44, Andrew Cooper wrote:
> With all other pieces in place, MSR_SPEC_CTRL is fully working for HVM guests.
> 
> Update the CPUID derivation logic (both PV and HVM to avoid losing subtle
> changes), and explicitly enable the CPUID bits for HVM guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> 
> Given the adjustment to calculate_pv_max_policy(), we could use 'A' rather
> than 'S' which would avoid a second same-sized diff to cpufeatureset.h, but
> it's also a bit misleading to say 'A' when the PV side won't engage at all
> yet.

I agree with using 'S' at this point for most of them. I'm unsure for
SSB_NO, though - there 'A' would seem more appropriate, and afaict it
would then also be visible to PV guests (since you validly don't make
it dependent upon IBRS or anything else). Aiui this could have been
done even ahead of this work of yours.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -433,6 +433,8 @@ static void __init 
> guest_common_feature_adjustments(uint32_t *fs)
>       */
>      if ( test_bit(X86_FEATURE_IBRSB, fs) )
>          __set_bit(X86_FEATURE_STIBP, fs);
> +    if ( test_bit(X86_FEATURE_IBRS, fs) )
> +        __set_bit(X86_FEATURE_AMD_STIBP, fs);
>  
>      /*
>       * On hardware which supports IBRS/IBPB, we can offer IBPB independently

Following this comment is a cross-vendor adjustment. Shouldn't there
be more of these now? Or has this been a one-off for some reason?

> @@ -456,11 +458,14 @@ static void __init calculate_pv_max_policy(void)
>          pv_featureset[i] &= pv_max_featuremask[i];
>  
>      /*
> -     * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests because of
> -     * administrator choice, hide the feature.
> +     * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
> +     * availability, or admin choice), hide the feature.
> +     */

Unintended replacement of "PV" by "HVM"?

>      if ( !boot_cpu_has(X86_FEATURE_SC_MSR_PV) )
> +    {
>          __clear_bit(X86_FEATURE_IBRSB, pv_featureset);
> +        __clear_bit(X86_FEATURE_IBRS, pv_featureset);
> +    }
>  
>      guest_common_feature_adjustments(pv_featureset);

What I had done in a local (and incomplete) patch is pass
X86_FEATURE_SC_MSR_{PV,HVM} into guest_common_feature_adjustments()
and do what you extend above (and then again for HVM) centrally
there. (My gen-cpuid.py adjustment was smaller, so there were even
more bits to clear, and hence it became yet more relevant to avoid
doing this in two places.)

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -290,6 +290,11 @@ def crunch_numbers(state):
>  
>          # In principle the TSXLDTRK insns could also be considered 
> independent.
>          RTM: [TSXLDTRK],
> +
> +        # AMD speculative controls
> +        IBRS: [AMD_STIBP, AMD_SSBD, PSFD,
> +               IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
> +        AMD_STIBP: [STIBP_ALWAYS],
>      }

Could I talk you into moving this ahead of RTM, such that it sits
next to the related Intel stuff?

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.