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
