Re: [PATCH] KVM: x86: advertise absence of X86_BUG_NULL_SEG via CPUID

[Paolo Bonzini <pbonzini@redhat.com>]

On 21/10/21 23:36, Jim Mattson wrote:
> On Thu, Oct 21, 2021 at 2:20 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> Guests have X86_BUG_NULL_SEG if and only if the host have it.  Use
>> the info from static_cpu_has_bug to form the 0x80000021 CPUID leaf that
>> was defined for Zen3.  Userspace can then set the bit even on older
>> CPUs that do not have the bug, such as Zen2.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 2d70edb0f323..b51398e1727b 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -902,7 +902,13 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>                  entry->edx = 0;
>>                  break;
>>          case 0x80000000:
>> -               entry->eax = min(entry->eax, 0x8000001f);
>> +               entry->eax = min(entry->eax, 0x80000021);
>> +               /*
>> +                * X86_BUG_NULL_SEG is not reported in CPUID on Zen2; in
>> +                * that case, provide the CPUID leaf ourselves.
>> +                */
> 
> I think this is backwards. !X86_BUG_NULL_SEG isn't reported in CPUID on Zen2.

Right I should use the name of the bit instead.

>> +               if (!static_cpu_has_bug(X86_BUG_NULL_SEG))
>> +                       entry->eax = max(entry->eax, 0x80000021);
>>                  break;
>>          case 0x80000001:
>>                  cpuid_entry_override(entry, CPUID_8000_0001_EDX);
>> @@ -973,6 +979,15 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>                          entry->ebx &= ~GENMASK(11, 6);
>>                  }
>>                  break;
>> +       case 0x80000020:
>> +               entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>> +               break;
>> +       case 0x80000021:
>> +               entry->ebx = entry->ecx = entry->edx = 0;
>> +               entry->eax &= BIT(6);
> 
> While we're here, shouldn't bit 0 (Processor ignores nested data
> breakpoints) and bit 2 (LFENCE is always dispatch serializing) also
> match the hardware?

Yes, that makes sense.  Just wanted to gauge whether anybody thought it 
a really bad idea.

Paolo

> 
>> +               if (!static_cpu_has_bug(X86_BUG_NULL_SEG))
>> +                       entry->eax |= BIT(6);
>> +               break;
>>          /*Add support for Centaur's CPUID instruction*/
>>          case 0xC0000000:
>>                  /*Just support up to 0xC0000004 now*/
>> --
>> 2.27.0
>>
>