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

[Brian Woods <brian.woods@xxxxxxx>]

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

> >> 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.
> 
> Thanks - we prefer to avoid u<N> and s<N> in favor of uint<N>_t
> and int<N>_t in new code, and __u<N> as well as __s<N> should
> go away rather sooner than later anyway, as representing name
> space violations (__s8, for example, already looks to be unused).

Noted, I've changed all of them.

> >> 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.
> 
> Where? I can't find anything as to the purpose. Your response to
> patch 1's comments on mine might have been a hint, but then again
> the function parameter here seems contrary to the alternatives
> patching plans, at least at the first glance. If you want to keep
> the parameter (rather than introducing it when needed), please at
> least outline the plans in the description.

To be honest, I assumed that only patch 1 would get accepted and then
once there was the infrastructure (alternative c funcs), I'd rework the
rest into a patch set that would introduce allowing HVM guests to
change SSBD via MSR interception (since there is some interest in
that).  I guess I should have been clearer up front about it.

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

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

Also, settings SSBD via the LS_CFG MSR is not a permanent method.  Once
hardware supports using SPEC_CTRL for it, it won't be needed.  For
multisocket systems, there should only be 2 sockets.  I agree with
making things generic, but this early in the boot there's limited
information.

> >> 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.
> 
> Personally I'd prefer if the xen_-prefixed sub-name-space was left to
> the public interface. Make it an infix if you want to express what you've
> explained?
> 
> Jan

Noted.  I'll change that.

Thanks again for the feedback.

-- 
Brian Woods

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