| Message ID | 20240517173926.965351-40-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
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
On Thu, Jul 04, 2024, Maxim Levitsky wrote: > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > 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). Yep, that aligns with how I view the ioctls (I haven't read the documentaion, mainly because I have a terrible habit of never reading docs). > I can post a patch to fix this or you can add something like that to your > patch series if you prefer. Go ahead and post a patch, assuming it's just a documentation update.
On Mon, 2024-07-08 at 17:13 -0700, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > 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). > > Yep, that aligns with how I view the ioctls (I haven't read the documentaion, > mainly because I have a terrible habit of never reading docs). > > > I can post a patch to fix this or you can add something like that to your > > patch series if you prefer. > > Go ahead and post a patch, assuming it's just a documentation update. > OK, will do. Best regards, Maxim Levitsky
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; }
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(-)