On Mon, Mar 28, 2022 at 04:21:02PM +0200, Jan Beulich wrote:
> On 15.03.2022 15:18, Roger Pau Monne wrote:
> > +void amd_init_ssbd(const struct cpuinfo_x86 *c)
> > +{
> > + if (cpu_has_ssb_no)
> > + return;
> > +
> > + if (cpu_has_amd_ssbd) {
> > + /* Handled by common MSR_SPEC_CTRL logic */
> > + return;
> > + }
> > +
> > + if (cpu_has_virt_ssbd) {
> > + wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> > + return;
> > + }
> > +
> > + if (!set_legacy_ssbd(c, opt_ssbd))
> > + {
>
> Nit: In this file the brace belongs on the earlier line and ...
>
> > printk_once(XENLOG_ERR "No SSBD controls available\n");
> > + if (amd_legacy_ssbd)
> > + panic("CPU feature mismatch: no legacy SSBD\n");
> > + }
> > + else if ( c == &boot_cpu_data )
>
> ... you want to omit the blanks immediately inside the parentheses here.
Ouch, yes.
> > + amd_legacy_ssbd = true;
> > +}
> > +
> > +static struct ssbd_core {
> > + spinlock_t lock;
> > + unsigned int count;
> > +} *ssbd_core;
> > +static unsigned int __ro_after_init ssbd_max_cores;
> > +
> > +bool __init amd_setup_legacy_ssbd(void)
> > +{
> > + unsigned int i;
> > +
> > + if (boot_cpu_data.x86 != 0x17 || boot_cpu_data.x86_num_siblings <= 1)
> > + return true;
> > +
> > + /*
> > + * One could be forgiven for thinking that c->x86_max_cores is the
> > + * correct value to use here.
> > + *
> > + * However, that value is derived from the current configuration, and
> > + * c->cpu_core_id is sparse on all but the top end CPUs. Derive
> > + * max_cpus from ApicIdCoreIdSize which will cover any sparseness.
> > + */
> > + if (boot_cpu_data.extended_cpuid_level >= 0x80000008) {
> > + ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x80000008), 0xf000);
> > + ssbd_max_cores /= boot_cpu_data.x86_num_siblings;
> > + }
> > + if (!ssbd_max_cores)
> > + return false;
> > +
> > + /* Max is two sockets for Fam17h hardware. */
> > + ssbd_core = xzalloc_array(struct ssbd_core, ssbd_max_cores * 2);
>
> If I'm not mistaken this literal 2, ...
>
> > + if (!ssbd_core)
> > + return false;
> > +
> > + for (i = 0; i < ssbd_max_cores * 2; i++) {
>
> ... this one, and ...
>
> > + spin_lock_init(&ssbd_core[i].lock);
> > + /* Record initial state, also applies to any hotplug CPU. */
> > + if (opt_ssbd)
> > + ssbd_core[i].count = boot_cpu_data.x86_num_siblings;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +void amd_set_legacy_ssbd(bool enable)
> > +{
> > + const struct cpuinfo_x86 *c = ¤t_cpu_data;
> > + struct ssbd_core *core;
> > + unsigned long flags;
> > +
> > + if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> > + BUG_ON(!set_legacy_ssbd(c, enable));
> > + return;
> > + }
> > +
> > + BUG_ON(c->phys_proc_id >= 2);
>
> .. this one are all referring to the same thing. Please use a #define to
> make the connection obvious.
Indeed. That's the maximum number of sockets possible with that CPU
family (2).
> > @@ -677,14 +680,17 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
> > uint64_t val)
> > if ( !cp->extd.virt_ssbd )
> > goto gp_fault;
> >
> > - /*
> > - * Only supports SSBD bit, the rest are ignored. Only modify the
> > SSBD
> > - * bit in case other bits are set.
> > - */
> > - if ( val & SPEC_CTRL_SSBD )
> > - msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> > + /* Only supports SSBD bit, the rest are ignored. */
> > + if ( cpu_has_amd_ssbd )
> > + {
> > + /* Only modify the SSBD bit in case other bits are set. */
>
> While more a comment on the earlier patch introducing this wording, it
> occurred to me only here that this is ambiguous: It can also be read as
> "Only modify the SSBD bit as long as other bits are set."
Hm, no, that's not what I meant. I meant to note that here we are
careful to only modify the SSBD bit of spec_ctrl, because other bits
might be used for other purposes. We can't do:
msrs->spec_ctrl.raw = SPEC_CTRL_SSBD;
But maybe this doesn't require a comment, as it seems to raise more
questions than answer?
> > + if ( val & SPEC_CTRL_SSBD )
> > + msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> > + else
> > + msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> > + }
> > else
> > - msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> > + msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;
>
> I also think the comment applies equally to the "else" logic, so perhaps
> the comment would best remain as is (and merely be re-worded in the
> earlier patch)?
Sure, let's see if we can get consensus on a proper wording.
Thanks, Roger.
