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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.