Re: [PATCH v2 39/49] KVM: x86: Extract code for generating per-entry emulated CPUID information

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> Extract the meat of __do_cpuid_func_emulated() into a separate helper,
> cpuid_func_emulated(), so that cpuid_func_emulated() can be used with a
> single CPUID entry.  This will allow marking emulated features as fully
> supported in the guest cpu_caps without needing to hardcode the set of
> emulated features in multiple locations.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index fd725cbbcce5..d1849fe874ab 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1007,14 +1007,10 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array,
>  	return entry;
>  }
>  
> -static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
> +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func)
>  {
> -	struct kvm_cpuid_entry2 *entry;
> +	memset(entry, 0, sizeof(*entry));
>  
> -	if (array->nent >= array->maxnent)
> -		return -E2BIG;
> -
> -	entry = &array->entries[array->nent];
>  	entry->function = func;
>  	entry->index = 0;
>  	entry->flags = 0;
> @@ -1022,23 +1018,27 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
>  	switch (func) {
>  	case 0:
>  		entry->eax = 7;
> -		++array->nent;
> -		break;
> +		return 1;
>  	case 1:
>  		entry->ecx = F(MOVBE);
> -		++array->nent;
> -		break;
> +		return 1;
>  	case 7:
>  		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>  		entry->eax = 0;
>  		if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP))
>  			entry->ecx = F(RDPID);
> -		++array->nent;
> -		break;
> +		return 1;
>  	default:
> -		break;
> +		return 0;
>  	}
> +}
>  
> +static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
> +{
> +	if (array->nent >= array->maxnent)
> +		return -E2BIG;
> +
> +	array->nent += cpuid_func_emulated(&array->entries[array->nent], func);
>  	return 0;
>  }
>  
Hi,

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


PS: I spoke with Paolo about the meaning of KVM_GET_EMULATED_CPUID, because it is not clear
from the documentation what it does, or what it supposed to do because qemu doesn't use this
IOCTL.

So this ioctl is meant to return a static list of CPU features which *can* be emulated
by KVM, if the cpu doesn't support them, but there is a cost to it, so they
should not be enabled by default.

This means that if you run 'qemu -cpu host', these features (like rdpid) will only
be enabled if supported by the host cpu, however if you explicitly ask
qemu for such a feature, like 'qemu -cpu host,+rdpid', 
qemu should not warn if the feature is not supported on host cpu but can be emulated
(because kvm can emulate the feature, which is stated by KVM_GET_EMULATED_CPUID ioctl).

Qemu currently doesn't support this but the support can be added.

So I think that the two ioctls should be redefined as such:

KVM_GET_SUPPORTED_CPUID - returns all CPU features that are supported
by KVM, supported by host hardware, or that KVM can efficiently emulate.


KVM_GET_EMULATED_CPUID - returns all CPU features that KVM *can* emulate
if the host cpu lacks support, but emulation is not efficient and thus
these features should be used with care when not supported by the host 
(e.g only when the user explicitly asks for them).


I can post a patch to fix this or you can add something like that to your
patch series if you prefer.


Best regards,
	Maxim Levitsky