Re: [PATCH 1/6] target/i386/kvm: Add feature bit definitions for KVM CPUID

[Zhao Liu <zhao1.liu@intel.com>]

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@intel.com>
> 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@intel.com>
> > ---
> > 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