Re: [PATCH v2 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD

["Roger Pau Monné" <roger.pau@citrix.com>]

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 = &current_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.