Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
>>> On 17.08.18 at 20:45, wrote:
> On Fri, Aug 17, 2018 at 12:59:28AM -0600, Jan Beulich wrote:
>> >>> On 16.08.18 at 22:02, wrote:
>> > On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
>> >> > +bool __read_mostly ssbd_amd_smt_en = false;
>> >> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
>> >> > uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
>> >> > +struct ssbd_amd_ls_cfg_smt_status
>> >> > *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
>> >>
>> >> Several further pointless initializers.
>> >
>> > ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
>> > ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
>> > sets it as NULL on failure to alloc
>> > default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
>> > added to an if statement
>> > ssbd_amd_smt_en -> like the above
>> >
>> > If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
>> > not be initialized, I can add code to do that.
>>
>> I don't understand: Add code? The initializers here are all pointless
>> because the values you supply are what the variables would be
>> initialized with anyway if you omitted the initializers. That's what
>> the C standard specifies.
>
> Leaving values which determine the behavior of the hypervisor to
> defaults of the compiler's implementation of the standard seems like it
> would be a possible source of subtle bugs when the compiler doesn't do
> something just right. I'd much rather have an initialized value or
> have it set in the code before use.
>
> If you strongly feel that they shouldn't be initialized or set in code,
> I'll change them though.
Yes, I do feel strongly about this: We omit pointless initializers
elsewhere, and the comment I've given here is a pretty common one
to be made in reviews. If we were to write code being prepared for
compiler bugs (which standards-non-conformance is), we could as
well stop writing any code in the first place. Workarounds are
acceptable only for _known_ compiler defects.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
On Fri, Aug 17, 2018 at 12:59:28AM -0600, Jan Beulich wrote:
> >>> On 16.08.18 at 22:02, wrote:
> > On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
> >> >>> On 09.08.18 at 21:42, wrote:
> >> > --- a/xen/arch/x86/spec_ctrl.c
> >> > +++ b/xen/arch/x86/spec_ctrl.c
> >>
> >> First of all - I'm not convinced some of the AMD specific code here
> >> wouldn't better live in cpu/amd.c.
> >
> > Well, by that logic, a lot of the other logic could go in cpu/intel.c.
> > It has to do with SSBD and when AMD's processors support it via the
> > SPEC_CTRL MSR, the support for SSBD will get merged together in
> > spec_ctrl.c and if that's the case, it makes sense to have all the SSBD
> > code together. IMO
>
> Maybe, though I'd say retpoline_safe(), should_use_eager_fpu(),
> l1tf_calculations(), and xpti_init_default() are all written in a way
> that they could be extended to other CPU vendors should it
> become known that they're also affected. I don't think we really
> know for sure whether VIA and/or Shanghai are affected. Otoh
> the code you add is not just AMD-specific, but even model-specific
> within AMD's palette.
>
> I'd be curious to know whether Andrew has an opinion here.
Since most of the spec_ctrl code is his, his opinion here would be the
most important.
> >> > @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
> >> > uint8_t __read_mostly default_xen_spec_ctrl;
> >> > uint8_t __read_mostly default_spec_ctrl_flags;
> >> >
> >> > +/* for SSBD support for AMD via LS_CFG */
> >> > +#define SSBD_AMD_MAX_SOCKET 2
> >> > +struct ssbd_amd_ls_cfg_smt_status {
> >> > +spinlock_t lock;
> >> > +uint32_t mask;
> >> > +} __attribute__ ((aligned (64)));
> >>
> >> Where's this literal 64 coming from? Do you perhaps mean
> >> SMP_CACHE_BYTES? And if this is really needed (as said before,
> >> I think the array would better be dynamically allocated than having
> >> compile time determined fixed size), then please don't open-code
> >> __aligned().
> >
> > It's the cache coherency size. The SMP_CACHE_BYTES is 128 bytes IIRC.
> > I was trying to save space since the struct is so small it would double
> > the size. That can be changed though.
>
> If SMP_CACHE_BYTES doesn't fit your need, the literal number used
> needs either an explaining comment or a suitably named #define.
I'll just use SMP_CACHE_BYTES and not worry about the extra space.
> >> > +bool __read_mostly ssbd_amd_smt_en = false;
> >> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
> >> > uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
> >> > +struct ssbd_amd_ls_cfg_smt_status
> >> > *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
> >>
> >> Several further pointless initializers.
> >
> > ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
> > ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
> > sets it as NULL on failure to alloc
> > default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
> > added to an if statement
> > ssbd_amd_smt_en -> like the above
> >
> > If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
> > not be initialized, I can add code to do that.
>
> I don't understand: Add code? The initializers here are all pointless
> because the values you supply are what the variables would be
> initialized with anyway if you omitted the initializers. That's what
> the C standard specifies.
Leaving values which determine the behavior of the hypervisor to
defaults of the compiler's implementation of the standard seems like it
would be a possible source of subtle bugs when the compiler doesn't do
something just right. I'd much rather have an initialized value or
have it set in the code before use.
If you strongly feel that they shouldn't be initialized or set in code,
I'll change them though.
> >> > +static int __init ssbd_amd_ls_cfg_init(void)
> >> > +{
> >> > +uint32_t cores_per_socket, threads_per_core;
> >> > +const struct cpuinfo_x86 *c = _cpu_data;
> >> > +uint32_t core, socket;
> >> > +
> >> > +if ( !ssbd_amd_ls_cfg_mask ||
> >> > + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
> >> > +goto ssbd_amd_ls_cfg_init_fail;
> >>
> >> Why not simply "return"?
> >
> > Because it forces all the failed returns down one code path. I can
> > change it if you wish.
>
> If any cleanup is to be done, using "goto" for this purpose is
> generally accepted (although personally I dislike "goto" in
> general). Here, however, nothing has been done yet and the
> cleanup path is non-trivial. It took me extra time to verify
> that nothing bad would happen from going that path despite
> nothing having been done before. It is far more clear to the
> reader imo if there is just "return" here.
Well, it's just a difference of opinion at this point. I'll change it.
> >> > +default_xen_ssbd_amd_ls_cfg_en = false;
> >> > +
> >> > +
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
>>> On 16.08.18 at 22:02, wrote:
> On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
>> >>> On 09.08.18 at 21:42, wrote:
>> > --- a/xen/arch/x86/spec_ctrl.c
>> > +++ b/xen/arch/x86/spec_ctrl.c
>>
>> First of all - I'm not convinced some of the AMD specific code here
>> wouldn't better live in cpu/amd.c.
>
> Well, by that logic, a lot of the other logic could go in cpu/intel.c.
> It has to do with SSBD and when AMD's processors support it via the
> SPEC_CTRL MSR, the support for SSBD will get merged together in
> spec_ctrl.c and if that's the case, it makes sense to have all the SSBD
> code together. IMO
Maybe, though I'd say retpoline_safe(), should_use_eager_fpu(),
l1tf_calculations(), and xpti_init_default() are all written in a way
that they could be extended to other CPU vendors should it
become known that they're also affected. I don't think we really
know for sure whether VIA and/or Shanghai are affected. Otoh
the code you add is not just AMD-specific, but even model-specific
within AMD's palette.
I'd be curious to know whether Andrew has an opinion here.
>> > @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
>> > uint8_t __read_mostly default_xen_spec_ctrl;
>> > uint8_t __read_mostly default_spec_ctrl_flags;
>> >
>> > +/* for SSBD support for AMD via LS_CFG */
>> > +#define SSBD_AMD_MAX_SOCKET 2
>> > +struct ssbd_amd_ls_cfg_smt_status {
>> > +spinlock_t lock;
>> > +uint32_t mask;
>> > +} __attribute__ ((aligned (64)));
>>
>> Where's this literal 64 coming from? Do you perhaps mean
>> SMP_CACHE_BYTES? And if this is really needed (as said before,
>> I think the array would better be dynamically allocated than having
>> compile time determined fixed size), then please don't open-code
>> __aligned().
>
> It's the cache coherency size. The SMP_CACHE_BYTES is 128 bytes IIRC.
> I was trying to save space since the struct is so small it would double
> the size. That can be changed though.
If SMP_CACHE_BYTES doesn't fit your need, the literal number used
needs either an explaining comment or a suitably named #define.
>> > +bool __read_mostly ssbd_amd_smt_en = false;
>> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
>> > uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
>> > +struct ssbd_amd_ls_cfg_smt_status
>> > *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
>>
>> Several further pointless initializers.
>
> ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
> ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
> sets it as NULL on failure to alloc
> default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
> added to an if statement
> ssbd_amd_smt_en -> like the above
>
> If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
> not be initialized, I can add code to do that.
I don't understand: Add code? The initializers here are all pointless
because the values you supply are what the variables would be
initialized with anyway if you omitted the initializers. That's what
the C standard specifies.
>> > +static int __init ssbd_amd_ls_cfg_init(void)
>> > +{
>> > +uint32_t cores_per_socket, threads_per_core;
>> > +const struct cpuinfo_x86 *c = _cpu_data;
>> > +uint32_t core, socket;
>> > +
>> > +if ( !ssbd_amd_ls_cfg_mask ||
>> > + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
>> > +goto ssbd_amd_ls_cfg_init_fail;
>>
>> Why not simply "return"?
>
> Because it forces all the failed returns down one code path. I can
> change it if you wish.
If any cleanup is to be done, using "goto" for this purpose is
generally accepted (although personally I dislike "goto" in
general). Here, however, nothing has been done yet and the
cleanup path is non-trivial. It took me extra time to verify
that nothing bad would happen from going that path despite
nothing having been done before. It is far more clear to the
reader imo if there is just "return" here.
>> > +default_xen_ssbd_amd_ls_cfg_en = false;
>> > +
>> > +dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to
>> > errors\n");
>> > +
>> > +return 1;
>>
>> If the function returns 0 and 1 only, it looks like you've meant to
>> use bool, false, and true respectively.
>
> Because it's more of an error code than boolean logic value. I can
> switch it over to bool if you want.
Error code style returns would please use (negative) errno-style
numbers. But if the function really just means to say "success"
or "failure", without particular error codes, then boolean would
imo still be preferable.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
> >>> On 09.08.18 at 21:42, wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -611,14 +611,9 @@ static void init_amd(struct cpuinfo_x86 *c)
> > ssbd_amd_ls_cfg_mask = 1ull << bit;
> > }
> >
> > - if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> > + if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
> > if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
> > setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
>
> If the inner if() was not to go away altogether in patch 1, please
> fold two successive if()-s like these.
>
> > --- a/xen/arch/x86/spec_ctrl.c
> > +++ b/xen/arch/x86/spec_ctrl.c
>
> First of all - I'm not convinced some of the AMD specific code here
> wouldn't better live in cpu/amd.c.
Well, by that logic, a lot of the other logic could go in cpu/intel.c.
It has to do with SSBD and when AMD's processors support it via the
SPEC_CTRL MSR, the support for SSBD will get merged together in
spec_ctrl.c and if that's the case, it makes sense to have all the SSBD
code together. IMO
> > @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
> > uint8_t __read_mostly default_xen_spec_ctrl;
> > uint8_t __read_mostly default_spec_ctrl_flags;
> >
> > +/* for SSBD support for AMD via LS_CFG */
> > +#define SSBD_AMD_MAX_SOCKET 2
> > +struct ssbd_amd_ls_cfg_smt_status {
> > +spinlock_t lock;
> > +uint32_t mask;
> > +} __attribute__ ((aligned (64)));
>
> Where's this literal 64 coming from? Do you perhaps mean
> SMP_CACHE_BYTES? And if this is really needed (as said before,
> I think the array would better be dynamically allocated than having
> compile time determined fixed size), then please don't open-code
> __aligned().
It's the cache coherency size. The SMP_CACHE_BYTES is 128 bytes IIRC.
I was trying to save space since the struct is so small it would double
the size. That can be changed though.
> > +bool __read_mostly ssbd_amd_smt_en = false;
> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
> > uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
> > +struct ssbd_amd_ls_cfg_smt_status
> > *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
>
> Several further pointless initializers.
ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
sets it as NULL on failure to alloc
default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
added to an if statement
ssbd_amd_smt_en -> like the above
If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
not be initialized, I can add code to do that.
> > +static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
> > +{
> > +uint32_t socket, core, thread;
> > +uint64_t enable_mask;
> > +uint64_t ls_cfg;
> > +struct ssbd_amd_ls_cfg_smt_status *status;
> > +const struct cpuinfo_x86 *c = _cpu_data;
> > +
> > +socket = c->phys_proc_id;
> > +core = c->cpu_core_id;
> > +thread = c->apicid & (c->x86_num_siblings - 1);
> > +status = ssbd_amd_smt_status[socket] + core;
> > +enable_mask = (1ull << thread);
> > +
> > +spin_lock(>lock);
> > +
> > +if ( enable_ssbd )
> > +{
> > +if ( !(status->mask & enable_mask) )
>
> So with ->mask being uint32_t, why does enable_mask need to be
> uint64_t? Even uint32_t seems way more than needed, but there's
> certainly no point going below this. Just that, as expressed before,
> you should please use "unsigned int" in favor of uint32_t everywhere,
> unless you really need something that's exactly 32-bits wide.
Because when changing the variable types from __32 etc, I confused it
with the enable mask for the LS_CFG reg. I'll change them.
> > +{
> > +status->mask |= enable_mask;
> > +rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> > +if ( !(ls_cfg & ssbd_amd_ls_cfg_mask) )
> > +{
> > +ls_cfg |= ssbd_amd_ls_cfg_mask;
> > +wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> > +}
> > +}
> > +}
> > +else
> > +{
> > +if ( status->mask & enable_mask )
> > +{
> > +status->mask &= ~enable_mask;
> > +rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> > +if ( (ls_cfg & ssbd_amd_ls_cfg_mask) && (status->mask == 0) )
>
> Please prefer the shorter ! over " == 0".
>
> > +{
> > +ls_cfg &= ~ssbd_amd_ls_cfg_mask;
> > +wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> > +}
> > +}
> > +}
> > +
> > +spin_unlock(>lock);
> > +}
> > +
> > +void ssbd_amd_ls_cfg_set(bool enable_ssbd)
> > +{
> > +if ( !ssbd_amd_ls_cfg_mask ||
> > + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ) {
> > +dprintk(XENLOG_ERR, "SSBD AMD
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
>>> On 09.08.18 at 21:42, wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -611,14 +611,9 @@ static void init_amd(struct cpuinfo_x86 *c)
> ssbd_amd_ls_cfg_mask = 1ull << bit;
> }
>
> - if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> + if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
> if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
> setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
If the inner if() was not to go away altogether in patch 1, please
fold two successive if()-s like these.
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
First of all - I'm not convinced some of the AMD specific code here
wouldn't better live in cpu/amd.c.
> @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
> uint8_t __read_mostly default_xen_spec_ctrl;
> uint8_t __read_mostly default_spec_ctrl_flags;
>
> +/* for SSBD support for AMD via LS_CFG */
> +#define SSBD_AMD_MAX_SOCKET 2
> +struct ssbd_amd_ls_cfg_smt_status {
> +spinlock_t lock;
> +uint32_t mask;
> +} __attribute__ ((aligned (64)));
Where's this literal 64 coming from? Do you perhaps mean
SMP_CACHE_BYTES? And if this is really needed (as said before,
I think the array would better be dynamically allocated than having
compile time determined fixed size), then please don't open-code
__aligned().
> +bool __read_mostly ssbd_amd_smt_en = false;
> +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
> uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
> +struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET]
> = {NULL};
Several further pointless initializers.
> +static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
> +{
> +uint32_t socket, core, thread;
> +uint64_t enable_mask;
> +uint64_t ls_cfg;
> +struct ssbd_amd_ls_cfg_smt_status *status;
> +const struct cpuinfo_x86 *c = _cpu_data;
> +
> +socket = c->phys_proc_id;
> +core = c->cpu_core_id;
> +thread = c->apicid & (c->x86_num_siblings - 1);
> +status = ssbd_amd_smt_status[socket] + core;
> +enable_mask = (1ull << thread);
> +
> +spin_lock(>lock);
> +
> +if ( enable_ssbd )
> +{
> +if ( !(status->mask & enable_mask) )
So with ->mask being uint32_t, why does enable_mask need to be
uint64_t? Even uint32_t seems way more than needed, but there's
certainly no point going below this. Just that, as expressed before,
you should please use "unsigned int" in favor of uint32_t everywhere,
unless you really need something that's exactly 32-bits wide.
> +{
> +status->mask |= enable_mask;
> +rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> +if ( !(ls_cfg & ssbd_amd_ls_cfg_mask) )
> +{
> +ls_cfg |= ssbd_amd_ls_cfg_mask;
> +wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> +}
> +}
> +}
> +else
> +{
> +if ( status->mask & enable_mask )
> +{
> +status->mask &= ~enable_mask;
> +rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> +if ( (ls_cfg & ssbd_amd_ls_cfg_mask) && (status->mask == 0) )
Please prefer the shorter ! over " == 0".
> +{
> +ls_cfg &= ~ssbd_amd_ls_cfg_mask;
> +wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> +}
> +}
> +}
> +
> +spin_unlock(>lock);
> +}
> +
> +void ssbd_amd_ls_cfg_set(bool enable_ssbd)
> +{
> +if ( !ssbd_amd_ls_cfg_mask ||
> + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ) {
> +dprintk(XENLOG_ERR, "SSBD AMD LS CFG: invalid mask or missing
> feature\n");
If the plan is for the function to also be called at runtime eventually,
this dprintk() needs to go away.
> +return;
> +}
> +
> +if ( ssbd_amd_smt_en )
> +ssbd_amd_ls_cfg_set_smt(enable_ssbd);
> +else
> +ssbd_amd_ls_cfg_set_nonsmt(enable_ssbd);
> +}
> +
> +static int __init ssbd_amd_ls_cfg_init(void)
> +{
> +uint32_t cores_per_socket, threads_per_core;
> +const struct cpuinfo_x86 *c = _cpu_data;
> +uint32_t core, socket;
> +
> +if ( !ssbd_amd_ls_cfg_mask ||
> + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
> +goto ssbd_amd_ls_cfg_init_fail;
Why not simply "return"?
> +switch ( c->x86 )
> +{
> +case 0x15:
> +case 0x16:
> +break;
> +
> +case 0x17:
> +cores_per_socket = c->x86_max_cores;
> +threads_per_core = c->x86_num_siblings;
> +
> +if ( threads_per_core > 1 )
> +{
> +ssbd_amd_smt_en = true;
> +for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
> +{
> +ssbd_amd_smt_status[socket] =
> + (struct ssbd_amd_ls_cfg_smt_status *)
> + xmalloc_array(struct ssbd_amd_ls_cfg_smt_status,
> +cores_per_socket);
Pointless cast.
> +
Re: [Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
On Thu, Aug 09, 2018 at 02:42:13PM -0500, Brian Woods wrote: > @@ -237,8 +247,8 @@ static void __init print_details(enum ind_thunk thunk, > uint64_t caps) > - !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" : > - (opt_ssbd && ssbd_amd_ls_cfg_mask)? " SSBD+" : " SSBD-", > + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ? "" : > + default_xen_ssbd_amd_ls_cfg_en? " SSBD+" : " SSBD-", This will change in the next version. I just noticed I only changed it in patch 2 and not 1. Sorry about that. -- Brian Woods ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR
Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
AMD CPUs. There needs to be locking logic for family 17h with SMT
enabled since both threads share the same MSR. Otherwise, a core just
needs to write to the LS_CFG MSR. For more information see:
https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
Signed-off-by: Brian Woods
---
xen/arch/x86/cpu/amd.c | 7 +-
xen/arch/x86/smpboot.c | 3 +
xen/arch/x86/spec_ctrl.c| 174 +++-
xen/include/asm-x86/spec_ctrl.h | 2 +
4 files changed, 178 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 06c9e9661b..6a5fbcae22 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -611,14 +611,9 @@ static void init_amd(struct cpuinfo_x86 *c)
ssbd_amd_ls_cfg_mask = 1ull << bit;
}
- if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+ if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
- if (opt_ssbd) {
- value |= ssbd_amd_ls_cfg_mask;
- wrmsr_safe(MSR_AMD64_LS_CFG, value);
- }
- }
/* MFENCE stops RDTSC speculation */
if (!cpu_has_lfence_dispatch)
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index d4478e6132..af881ba30a 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -366,6 +366,9 @@ void start_secondary(void *unused)
if ( boot_cpu_has(X86_FEATURE_IBRSB) )
wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
+if ( default_xen_ssbd_amd_ls_cfg_en )
+ssbd_amd_ls_cfg_set(true);
+
if ( xen_guest )
hypervisor_ap_setup();
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 62e6519d93..ff14b8f985 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
uint8_t __read_mostly default_xen_spec_ctrl;
uint8_t __read_mostly default_spec_ctrl_flags;
+/* for SSBD support for AMD via LS_CFG */
+#define SSBD_AMD_MAX_SOCKET 2
+struct ssbd_amd_ls_cfg_smt_status {
+spinlock_t lock;
+uint32_t mask;
+} __attribute__ ((aligned (64)));
+bool __read_mostly ssbd_amd_smt_en = false;
+bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
+struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] =
{NULL};
static int __init parse_bti(const char *s)
{
@@ -237,8 +247,8 @@ static void __init print_details(enum ind_thunk thunk,
uint64_t caps)
(default_xen_spec_ctrl & SPEC_CTRL_IBRS) ? "IBRS+" : "IBRS-",
!boot_cpu_has(X86_FEATURE_SSBD) ? "" :
(default_xen_spec_ctrl & SPEC_CTRL_SSBD) ? " SSBD+" : " SSBD-",
- !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
- (opt_ssbd && ssbd_amd_ls_cfg_mask)? " SSBD+" : " SSBD-",
+ !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ? "" :
+ default_xen_ssbd_amd_ls_cfg_en? " SSBD+" : " SSBD-",
opt_ibpb ? " IBPB" : "");
/*
@@ -487,6 +497,162 @@ static __init int parse_xpti(const char *s)
}
custom_param("xpti", parse_xpti);
+/*
+ * Enabling SSBD on AMD processers via the LS_CFG MSR
+ *
+ * For family 15h and 16h, there are no SMT enabled processors, so there
+ * is no need for locking, just setting an MSR bit. For 17h, it depends
+ * if SMT is enabled. If SMT, are two threads that share a single MSR
+ * so there needs to be a lock and a virtual bit for each thread,
+ * otherwise it's the same as family 15h/16h.
+ */
+
+static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
+{
+uint64_t ls_cfg, new_ls_cfg;
+
+rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+
+if ( enable_ssbd )
+new_ls_cfg = ls_cfg | ssbd_amd_ls_cfg_mask;
+else
+new_ls_cfg = ls_cfg & ~ssbd_amd_ls_cfg_mask;
+
+if ( new_ls_cfg != ls_cfg )
+wrmsrl(MSR_AMD64_LS_CFG, new_ls_cfg);
+}
+
+static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
+{
+uint32_t socket, core, thread;
+uint64_t enable_mask;
+uint64_t ls_cfg;
+struct ssbd_amd_ls_cfg_smt_status *status;
+const struct cpuinfo_x86 *c = _cpu_data;
+
+socket = c->phys_proc_id;
+core = c->cpu_core_id;
+thread = c->apicid & (c->x86_num_siblings - 1);
+status = ssbd_amd_smt_status[socket] + core;
+enable_mask = (1ull << thread);
+
+spin_lock(>lock);
+
+if ( enable_ssbd )
+{
+if ( !(status->mask & enable_mask) )
+{
+status->mask |= enable_mask;
+
