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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.18 at 22:02, <brian.woods@xxxxxxx> wrote:
> 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/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 =  &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.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel