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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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 =  &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.

> +        {
> +            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"?

> +    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.

> +    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


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