Hi Zide,
On Fri, Apr 26, 2024 at 10:23:27AM -0700, Chen, Zide wrote:
> Date: Fri, 26 Apr 2024 10:23:27 -0700
> From: "Chen, Zide" <zide.chen@xxxxxxxxx>
> Subject: Re: [PATCH 1/6] target/i386/kvm: Add feature bit definitions for
> KVM CPUID
>
> On 4/26/2024 3:07 AM, Zhao Liu wrote:
> > Add feature definiations for KVM_CPUID_FEATURES in CPUID (
> > CPUID[4000_0001].EAX and CPUID[4000_0001].EDX), to get rid of lots of
> > offset calculations.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@xxxxxxxxx>
> > ---
> > v2: Changed the prefix from CPUID_FEAT_KVM_* to CPUID_KVM_*. (Xiaoyao)
> > ---
> > hw/i386/kvm/clock.c | 5 ++---
> > target/i386/cpu.h | 23 +++++++++++++++++++++++
> > target/i386/kvm/kvm.c | 28 ++++++++++++++--------------
> > 3 files changed, 39 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> > index 40aa9a32c32c..ce416c05a3d0 100644
> > --- a/hw/i386/kvm/clock.c
> > +++ b/hw/i386/kvm/clock.c
> > @@ -27,7 +27,6 @@
> > #include "qapi/error.h"
> >
> > #include <linux/kvm.h>
> > -#include "standard-headers/asm-x86/kvm_para.h"
> > #include "qom/object.h"
> >
> > #define TYPE_KVM_CLOCK "kvmclock"
> > @@ -334,8 +333,8 @@ void kvmclock_create(bool create_always)
> >
> > assert(kvm_enabled());
> > if (create_always ||
> > - cpu->env.features[FEAT_KVM] & ((1ULL << KVM_FEATURE_CLOCKSOURCE) |
> > - (1ULL << KVM_FEATURE_CLOCKSOURCE2))) {
> > + cpu->env.features[FEAT_KVM] & (CPUID_KVM_CLOCK |
> > + CPUID_KVM_CLOCK2)) {
>
> To achieve this purpose, how about doing the alternative to define an
> API similar to KVM's guest_pv_has()?
>
> xxxx_has() is simpler and clearer than "features[] & CPUID_xxxxx",
> additionally, this helps to keep the definitions identical to KVM, more
> readable and easier for future maintenance.
Yes, it's a clearer way! I can explore the xxxx_has() pattern in another
series and try to expand it to more CPUID leaves.
Thanks,
Zhao
