Re: [Qemu-devel] [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jan 07, 2013 at 10:09:24AM -0200, Eduardo Habkost wrote:
> On Mon, Jan 07, 2013 at 01:42:53PM +0200, Gleb Natapov wrote:
> > On Mon, Jan 07, 2013 at 09:42:36AM -0200, Eduardo Habkost wrote:
> > > On Sun, Jan 06, 2013 at 01:32:34PM +0200, Gleb Natapov wrote:
> > > > On Fri, Jan 04, 2013 at 08:01:02PM -0200, Eduardo Habkost wrote:
> > > > > This is a cleanup that tries to solve two small issues:
> > > > > 
> > > > >  - We don't need a separate kvm_pv_eoi_features variable just to keep a
> > > > >    constant calculated at compile-time, and this style would require
> > > > >    adding a separate variable (that's declared twice because of the
> > > > >    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
> > > > >    by machine-type compat code.
> > > > >  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
> > > > >    even when KVM is disabled at runtime. This small incosistency in
> > > > >    the cpuid_kvm_features field isn't a problem today because
> > > > >    cpuid_kvm_features is ignored by the TCG code, but it may cause
> > > > >    unexpected problems later when refactoring the CPUID handling code.
> > > > > 
> > > > > This patch eliminates the kvm_pv_eoi_features variable and simply uses
> > > > > CONFIG_KVM and kvm_enabled() inside the enable_kvm_pv_eoi() compat
> > > > > function, so it enables kvm_pv_eoi only if KVM is enabled. I believe
> > > > > this makes the behavior of enable_kvm_pv_eoi() clearer and easier to
> > > > > understand.
> > > > > 
> > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > ---
> > > > > Cc: kvm@vger.kernel.org
> > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > Cc: Gleb Natapov <gleb@redhat.com>
> > > > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > > > 
> > > > > Changes v2:
> > > > >  - Coding style fix
> > > > > ---
> > > > >  target-i386/cpu.c | 8 +++++---
> > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > index 82685dc..e6435da 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -145,15 +145,17 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
> > > > >          (1 << KVM_FEATURE_ASYNC_PF) |
> > > > >          (1 << KVM_FEATURE_STEAL_TIME) |
> > > > >          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> > > > > -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> > > > >  #else
> > > > >  static uint32_t kvm_default_features = 0;
> > > > > -static const uint32_t kvm_pv_eoi_features = 0;
> > > > >  #endif
> > > > >  
> > > > >  void enable_kvm_pv_eoi(void)
> > > > >  {
> > > > > -    kvm_default_features |= kvm_pv_eoi_features;
> > > > > +#ifdef CONFIG_KVM
> > > > You do not need ifdef here.
> > > 
> > > We need it because KVM_FEATURE_PV_EOI is available only if CONFIG_KVM is
> > > set.
> > > 
> > > I could also write it as:
> > > 
> > >     if (kvm_enabled()) {
> > > #ifdef CONFIG_KVM
> > >         kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > #endif
> > >     }
> > > 
> > > But I find it less readable.
> > > 
> > > 
> > Why not define KVM_FEATURE_PV_EOI unconditionally?
> 
> It comes from the KVM kernel headers, that are included only if
> CONFIG_KVM is set, and probably won't even compile in non-Linux systems.
> 
> I have a dejavu feeling. I believe we had this exact problem before,
> maybe about some other #defines that come from the Linux KVM headers and
> won't be available in non-Linux systems.
>
It is better to hide all KVM related differences somewhere in the
headers where no one sees them instead of sprinkle them all over the
code. We can put those defines in include/sysemu/kvm.h in !CONFIG_KVM
part. Or have one ifdef CONFIG_KVM at the beginning of the file and
define enable_kvm_pv_eoi() there and provide empty stub otherwise.

--
			Gleb.