Re: [PATCH v16] x86/split_lock: Enable split lock detection by kernel

[Andy Lutomirski]

On Sat, Jan 25, 2020 at 2:07 PM Luck, Tony <tony.luck@xxxxxxxxx> wrote:
>
> From: "Peter Zijlstra (Intel)" <peterz@xxxxxxxxxxxxx>
>

> +void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
> +{
> +       u64 ia32_core_caps = 0;
> +
> +       if (c->x86_vendor != X86_VENDOR_INTEL)
> +               return;
> +       if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES)) {
> +               /* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
> +               rdmsrl(MSR_IA32_CORE_CAPS, ia32_core_caps);
> +       } else if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> +               /* Enumerate split lock detection by family and model. */
> +               if (x86_match_cpu(split_lock_cpu_ids))
> +                       ia32_core_caps |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
> +       }

I was chatting with Andrew Cooper, and apparently there are a ton of
hypervisors bugs in this space, and the bugs take two forms.  Some
hypervisors might #GP the read, and some might allow the read but
silently swallow writes.  This isn't *that* likely given that the
hypervisor bit is the default, but we could improve this like (sorry
for awful whitespace);

static bool have_split_lock_detect(void) {
      unsigned long tmp;

      if (cpu_has(c, X86_FEATURE_CORE_CAPABILITIES) {
              /* Enumerate features reported in IA32_CORE_CAPABILITIES MSR. */
               rdmsrl(MSR_IA32_CORE_CAPS, tmp);
               if (tmp & MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT)
                  return true;
      }

      if (cpu_has(c, X86_FEATURE_HYPERVISOR))
            return false;

       if (rdmsrl_safe(MSR_TEST_CTRL, &tmp))
               return false;

      if (wrmsrl_safe(MSR_TEST_CTRL, tmp ^ MSR_TEST_CTRL_SPLIT_LOCK_DETECT))
              return false;

      wrmsrl(MSR_TEST_CTRL, tmp);
}

Although I suppose the pile of wrmsrl_safes() in the existing patch
might be sufficient.

All this being said, the current code appears wrong if a CPU is in the
list but does have X86_FEATURE_CORE_CAPABILITIES.  Are there such
CPUs?  I think either the logic should be changed or a comment should
be added.