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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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

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

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



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