[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD
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. > + 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. > @@ -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." > + 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)? Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |