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

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

On Fri, Apr 29, 2022 at 05:49:42PM +0200, Roger Pau Monné wrote:
> On Fri, Apr 29, 2022 at 12:59:58PM +0200, Jan Beulich wrote:
> > On 27.04.2022 12:47, Roger Pau Monne wrote:> Changes since v3:>  - Align 
> > ssbd per-core struct to a cache line.>  - Open code a simple spinlock to 
> > avoid playing tricks with the lock>    detector.>  - 
> > s/ssbd_core/ssbd_ls_cfg/.>  - Fix log message wording.>  - Fix define name 
> > and remove comment.>  - Also handle Hygon processors (Fam18h).>  - Add 
> > changelog entry.
> > What is this last line about?
> 
> Hm, seems like I forgot to do a patch refresh... So new version will
> have an entry about adding VIRT_SSBD support to HVM guests. Sorry for
> spoiling the surprise.
> 
> > > +bool __init amd_setup_legacy_ssbd(void)
> > > +{
> > > + unsigned int i;
> > > +
> > > + if ((boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
> > > +     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;
> > > +
> > > + ssbd_ls_cfg = xzalloc_array(struct ssbd_ls_cfg,
> > > +                             ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS);
> > > + if (!ssbd_ls_cfg)
> > > +         return false;
> > > +
> > > + for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
> > > +         /* Record initial state, also applies to any hotplug CPU. */
> > > +         if (opt_ssbd)
> > > +                 ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;
> > 
> > Perhaps flip if() and for()?
> 
> Indeed, thanks.
> 
> > > +void amd_set_legacy_ssbd(bool enable)
> > > +{
> > > + const struct cpuinfo_x86 *c = &current_cpu_data;
> > > + struct ssbd_ls_cfg *status;
> > > +
> > > + if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> > > +         BUG_ON(!set_legacy_ssbd(c, enable));
> > > +         return;
> > > + }
> > > +
> > > + BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
> > > + BUG_ON(c->cpu_core_id >= ssbd_max_cores);
> > > + status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
> > > +                       c->cpu_core_id];
> > > +
> > > + /*
> > > +  * Open code a very simple spinlock: this function is used with GIF==0
> > > +  * and different IF values, so would trigger the checklock detector.
> > > +  * Instead of trying to workaround the detector, use a very simple lock
> > > +  * implementation: it's better to reduce the amount of code executed
> > > +  * with GIF==0.
> > > +  */
> > > + while ( test_and_set_bool(status->locked) )
> > > +     cpu_relax();
> > > + status->count += enable ? 1 : -1;
> > > + ASSERT(status->count <= c->x86_num_siblings);
> > > + if (enable ? status->count == 1 : !status->count)
> > > +         BUG_ON(!set_legacy_ssbd(c, enable));
> > 
> > What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
> > region?
> 
> So AFAICT the BUG itself works, the usage of a crash kernel however
> won't work as it's booted with GIF==0.
> 
> Maybe we need to issue an stgi as part of BUG_FRAME if required?
> (maybe that's too naive...)

Well, better in panic() or kexec_crash() likely.

Roger.