Re: [PATCH 1/2] i386: Add support for SUCCOR feature

[Joao Martins]

On 12/07/2023 20:11, John Allen wrote:
> On Fri, Jul 07, 2023 at 04:25:22PM +0200, Paolo Bonzini wrote:
>> On 7/6/23 21:40, John Allen wrote:
>>>       case 0x80000007:
>>>           *eax = 0;
>>> -        *ebx = 0;
>>> +        *ebx = env->features[FEAT_8000_0007_EBX] | 
>>> CPUID_8000_0007_EBX_SUCCOR;
>>>           *ecx = 0;
>>>           *edx = env->features[FEAT_8000_0007_EDX];
>>>           break;
>>
>> I agree that it needs no hypervisor support, but Babu is right that you
>> cannot add it unconditionally (especially not on Intel processors).
>>
>> You can special case CPUID_8000_0007_EBX_SUCCOR in
>> kvm_arch_get_supported_cpuid() so that it is added even on old kernels.
>> There are already several such cases.  Adding it to KVM is nice to have
>> anyway, so please send a patch for that.
> 
> By adding it to KVM do you mean adding a patch to the kernel to expose
> the cpuid bit? Or do you mean just adding the special case to
> kvm_arch_get_supported_cpuid?
> 
> For the kvm_arch_get_supported_cpuid case, I don't understand how this
> would be different from unconditionally exposing the bit as done above.
> Can you help me understand what you have in mind for this?
> 
> I might add a case like below:
> ...
>     } else if (function == 0x80000007 && reg == R_EBX) {
>         ret |= CPUID_8000_0007_EBX_SUCCOR;
> ...
> 

IIUC the thinking here is to cover a hypervisor that doesn't advertise SUCCOR
via supported cpuid from the kernel, by adding that elif statement above.
So when a CPU model is configured, the filtering of cpuid leafs logic
takes place, it doesn't "dissappear" when the values from the kernel isn't set.

Otherwise, if it's advertised by the kernel, everything goes as normal in this
filtering logic and everything works.

This presumes that you set "succor" feature string in the CPUID model definition
instead of unilaterally defining it like this patch. Such that only AMD CPU
models advertise the feature instead of everybody.

> If we wanted to only expose the bit for AMD cpus, we would then need to
> call IS_AMD_CPU with the CPUX86State as a paramter which would mean that
> kvm_arch_get_supported_cpuid and all of its callers would need to take
> the CPUX86State as a parameter. Is there another way to differentiate
> between AMD and Intel cpus in this case?
> 
There might be an implicit convetion here where the caller is the one selecting
all the bits -- so kvm_arch_get_supported_cpuid won't care to validate which
vendor supports a given bit. Usually steered down via CPU model names (-cpu
Genoa), or named CPU features (-cpu Genoa,-feature-name) e.g. Nothing stops you
from trying to use a Intel CPUID on a AMD host. So the IS_AMD_CPU inside
kvm_arch_get_supported_cpuid doesn't seem to matter

        Joao