Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

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

On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote:
> Code structure wise this looks to undo a fair part of what patch
> 1 has done. It would be nice to limit code churn.

Patch 1 stand alone just to improve reporting the capabilities of the
processor.  Currently Xen doesn't mention anything if SSBD is actually
enable on AMD processors.  Patch 2-3 add it where Xen can it
dynamically.

> Why can't this be called from init_speculation_mitigations()?

IIRC I was having memory init problems with.  It was moved to later in
the boot so that xmalloc would work correctly.  I haven't touched this
code in over month.  If you want a 100% tested answer I can retest
putting it in init_speculation_mitigations().

> Please be consistent with types: ssbd_amd_ls_cfg_mask is
> uint64_t, in which case variables like the one here should be too.

I think that was left over from something in init_amd, but yes.

> Missing blanks.

Noted

> Please simplify this to have just a single rdmsrl() and wrmsrl()
> each in the function.

Will do.

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

IIRC they're __32 in struct I'm reading from so I decided to use that.
I can change them though, that's easily.


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

See earlier in this email.

> Still I wonder whether less code duplication wouldn't be better.

current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called.

> 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())?

Yes... That was meant to be a &.  Thanks for catching that.

> Most noticeable here, but applicable elsewhere: The canonical
> placement is
> 
> void __init ssbd_amd_ls_cfg_init(void)

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

> Blank lines between case blocks please.

Noted about the above.

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

It's been a while since I wrote this but IIRC, it has to do with
nr_sockets either being off or not available when the code is run.
For Family 17h which the patches are for, there's a max of two sockets.

> Style: Blank before * and no blank before (.

> Perhaps better
>                     spin_lock_init(&ssbd_amd_smt_status[socket][core].lock);
>                     ssbd_amd_smt_status[socket][core].mask = 0;
> ?

> Labels indented by at least one blank please.

Noted about the above.

> Btw - why the xen_ prefix for the variable?

See the first reply, but basically it's for if Xen has SSBD turned on
or not via using the LS_CFG MSR.

> Pointless "return" at end of function.
> 
> Jan

Noted.


Thanks for all the feedback.  I'll try and get v2 out in a couple of
days or so.

-- 
Brian Woods

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