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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.08.18 at 21:07, <brian.woods@xxxxxxx> wrote:
> On Thu, Aug 02, 2018 at 01:09:06AM -0600, Jan Beulich wrote:
>> >>> On 02.08.18 at 00:20, <brian.woods@xxxxxxx> 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.

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

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



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