Re: [PATCH v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR

[Brian Woods <brian.woods@amd.com>]

On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
> >>> On 09.08.18 at 21:42, <brian.woods@amd.com> 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 =  &current_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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel