|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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, <brian.woods@xxxxxxx> 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 = ¤t_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(&status->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(&status->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 = &boot_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.
> > + 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.
>
> > + if ( ssbd_amd_smt_status[socket] == NULL )
> > + {
> > + dprintk(XENLOG_ERR,
> > + "SSBD AMD LS CFG: error in status allocing\n");
> > + goto ssbd_amd_ls_cfg_init_fail;
> > + }
> > + }
> > +
> > + for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
> > + {
> > + for ( core = 0; core < cores_per_socket; core++ )
> > + {
> > +
> > spin_lock_init(&ssbd_amd_smt_status[socket][core].lock);
> > + ssbd_amd_smt_status[socket][core].mask = 0;
> > + }
> > + }
> > + }
> > + break;
> > +
> > + default:
> > + goto ssbd_amd_ls_cfg_init_fail;
> > + }
> > +
> > + if ( default_xen_ssbd_amd_ls_cfg_en )
> > + ssbd_amd_ls_cfg_set(true);
> > +
> > + return 0;
> > +
> > + ssbd_amd_ls_cfg_init_fail:
> > + for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
> > + if ( ssbd_amd_smt_status[socket] != NULL )
> > + xfree(ssbd_amd_smt_status[socket]);
>
> There's no need for the if() here.
>
> > + setup_clear_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
>
> The same feature must not first be forced, and the cleared. Please
> take a look at the implementation of the functions.
Will do.
> > + 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.
>
> Jan
>
Because it's more of an error code than boolean logic value. I can
switch it over to bool if you want.
Noted about the things I didn't directly reply to.
--
Brian Woods
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |