|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR
>>> On 20.07.18 at 16:57, <brian.woods@xxxxxxx> wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -607,16 +607,10 @@ static void init_amd(struct cpuinfo_x86 *c)
> case 0x17: bit = 10; break;
> }
>
> - if (bit >= 0)
> - ssbd_amd_ls_cfg_mask = 1ull << bit;
> - }
>
> - if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> - if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
> + if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> + ssbd_amd_ls_cfg_mask = 1ull << bit;
> 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);
> }
> }
Code structure wise this looks to undo a fair part of what patch
1 has done. It would be nice to limit code churn.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1579,6 +1579,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>
> arch_init_memory();
>
> + ssbd_amd_ls_cfg_init();
Why can't this be called from init_speculation_mitigations()?
> @@ -497,6 +497,201 @@ static __init int parse_xpti(const char *s)
> }
> custom_param("xpti", parse_xpti);
>
> +/*
> + * For family 15h and 16h, there are no SMT enabled processors, so there
> + * is no need for locking, just need to set an MSR bit. For 17h, it
> + * depends if SMT is enabled. If SMT, are two threads share a single
> + * MSR so there needs to be a lock and a virtual bit for each thread.
> + */
> +
> +/* used for non SMT mitigations (no shared MSRs) */
> +static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
> +{
> + unsigned long ls_cfg;
Please be consistent with types: ssbd_amd_ls_cfg_mask is
uint64_t, in which case variables like the one here should be too.
> + if ( enable_ssbd )
> + {
> + 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
> + {
> + rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> + if (ls_cfg & ssbd_amd_ls_cfg_mask)
Missing blanks.
> + {
> + ls_cfg &= ~ssbd_amd_ls_cfg_mask;
> + wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> + }
> + }
> +}
Please simplify this to have just a single rdmsrl() and wrmsrl()
each in the function.
> +/* used for family 17h with SMT enabled (shared MSRs) */
> +static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
> +{
> + __u32 socket, core, thread;
> + uint64_t enable_mask;
You really should notice such anomalies / inconsistencies yourself:
You properly use uint64_t here, so why not also uint32_t on the
preceding line? That said - why a fixed width type anyway for
those variables - unsigned int looks to be just fine there.
> +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\n");
> + return;
> + }
> +
> + if ( ssbd_amd_smt_en )
> + ssbd_amd_ls_cfg_set_smt(enable_ssbd);
> + else
> + ssbd_amd_ls_cfg_set_nonsmt(enable_ssbd);
> +}
This function is called from exactly one place, with the
parameter set to true. Makes me wonder what the actual
purpose of this patch is.
> +/*
> + * used to init the boot cpu, we don't need to lock anything because
> + * it's just the boot CPU
> + */
Still I wonder whether less code duplication wouldn't be better.
> +static __init void ssbd_amd_ls_cfg_set_init(void)
> +{
> + __u32 socket, core, thread;
> + unsigned long long ls_cfg;
> + struct ssbd_amd_ls_cfg_smt_status *status;
> + struct cpuinfo_x86 *c = &boot_cpu_data;
> +
> + if ( ssbd_amd_smt_en )
> + {
> + 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;
> + status->mask |= (1ull << thread);
This hides very well an assumption of there only ever being two
threads. Please don't. I'm also having a hard time seeing why
c->apicid being (non-)zero matters. Or wait - do you mean &
instead of && above (then also in ssbd_amd_ls_cfg_set_smt())?
> + }
> +
> + 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);
> + }
> +}
> +
> +__init void ssbd_amd_ls_cfg_init(void)
Most noticeable here, but applicable elsewhere: The canonical
placement is
void __init ssbd_amd_ls_cfg_init(void)
> +{
> + int cores_per_socket, threads_per_core;
> + struct cpuinfo_x86 *c = &boot_cpu_data;
> + int core,socket;
unsigned int please for anything that can't go negative. And a
blank is missing after the comma here, while there's one too
many on the line before.
You also don't look to be altering the data c points to - please
make this a pointer to const (and check whether there are
other places wanting such a transformation).
> + if ( !ssbd_amd_ls_cfg_mask ||
> + !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
> + goto ssbd_amd_ls_cfg_init_fail;
> +
> + switch ( c->x86 )
> + {
> + case 0x15:
> + case 0x16:
> + break;
> + case 0x17:
Blank lines between case blocks please.
> + cores_per_socket = c->x86_max_cores;
> + threads_per_core = c->x86_num_siblings;
> +
> + if ( (cores_per_socket < 1) || (threads_per_core < 1) )
> + {
> + dprintk(XENLOG_ERR,
> + "SSBD AMD LS CFG: error in topology decoding\n");
> + goto ssbd_amd_ls_cfg_init_fail;
> + }
> +
> + if ( threads_per_core > 1 )
> + {
> + ssbd_amd_smt_en = true;
> + for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
I find such a hard-coded upper bound quite concerning. Is nr_sockets
not correct in the AMD case? If so, that would want fixing, such that
you can use the variable here.
> + {
> + ssbd_amd_smt_status[socket] =
> + (struct ssbd_amd_ls_cfg_smt_status*)
> + xmalloc_array (struct ssbd_amd_ls_cfg_smt_status,
> + cores_per_socket);
Style: Blank before * and no blank before (.
> + 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;
Perhaps better
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 ( xen_ssbd_amd_ls_cfg_en )
> + ssbd_amd_ls_cfg_set_init();
> +
> + return;
> +
> +ssbd_amd_ls_cfg_init_fail:
Labels indented by at least one blank please.
> + for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
> + if ( ssbd_amd_smt_status[socket] != NULL )
> + xfree(ssbd_amd_smt_status[socket]);
> +
> + setup_clear_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
> + xen_ssbd_amd_ls_cfg_en = false;
Btw - why the xen_ prefix for the variable?
> + return;
> +}
Pointless "return" at end of function.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |