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, Aug 07, 2018 at 01:51:07AM -0600, Jan Beulich wrote:
> >>> On 06.08.18 at 21:07, <brian.woods@amd.com> wrote:
> > On Thu, Aug 02, 2018 at 01:09:06AM -0600, Jan Beulich wrote:
> >> >>> On 02.08.18 at 00:20, <brian.woods@amd.com> wrote:
> >> > 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.
> >> 
> >> Which is all fine, but couldn't patch 1 be written in a way that the
> >> next one(s) don't have to turn code structure all over again.
> > 
> > Rather than change 1, I changed patch 3 (well now patch 2).
> > 
> >> >> 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().
> >> 
> >> Would be nice if that could be arranged for, as the rather specialized
> >> call looks pretty odd in (iirc __start_xen(); iirc because you've dropped
> >> too much of the context in your reply, and I'm too lazy to dig out the
> >> original patch again).
> > 
> > It was __start_xen().  Well, IIRC xalloc didn't work (well writing to
> > the address given) until after arch_init_memory().  For it to work in
> > init_speculation_mitigations(), I'd assume you'd need to call
> > arch_init_memory() before init_speculation_mitigations().
> 
> I don't think that's a viable option, but it could certainly be explored.
> I wonder though whether you can't get away without allocation at
> this early point.

Well, the thing is that you could use DECLARE_PER_CPU but then you
have to initialize it during each cpu start up, but two logical cpus
share a lock and only one needs to touch it etc.  I found it better to
just initialize everything on the boot cpu before others are brought
up, but the whole thing is a little messy.  (See the last comment.)

> >> >> 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.
> >> 
> >> By "isn't available" you mean "hasn't been populated yet"? Else
> >> I don't understand.
> > 
> > It hasn't been populated yet.
> 
> Not even boot_cpu_data?

boot_cpu_data is, but current_cpu_data and nr_sockets is not.

> >> >> 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.
> >> 
> >> I've implied that from how you wrote the patches, but such hard coding
> >> will only make for more code churn later on. Try to be as generic as
> >> possible.
> > 
> > Well, nr_sockets gets set in smp_prepare_cpus, which gets called after
> > init_speculation_mitigations() and ssbd_amd_ls_cfg_init().  It could be
> > put later on in the boot, but it needs to be run at least before other
> > cpus are brought online.  I'm welcome to any suggestions about how to
> > restructure things though.
> 
> Did you consider using a presmp-initcall?
> 
> Jan

I've been looking at it, seems like that could work.  You'd still need
something in start_secondary but it would take the init call out of
__start_xen().  I'll test that.


-- 
Brian Woods

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