Patchwork [qom-cpu,01/11] target-i386: Don't set any KVM flag by default if KVM is disabled

login
register
mail settings
Submitter Eduardo Habkost
Date Jan. 4, 2013, 10:01 p.m.
Message ID <1357336872-7200-2-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/209564/
State New
Headers show

Comments

Eduardo Habkost - Jan. 4, 2013, 10:01 p.m.
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(-)
Gleb Natapov - Jan. 6, 2013, 11:32 a.m.
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.

> +    if (kvm_enabled()) {
> +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> +    }
> +#endif
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> -- 
> 1.7.11.7

--
			Gleb.
Eduardo Habkost - Jan. 7, 2013, 11:42 a.m.
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.


> 
> > +    if (kvm_enabled()) {
> > +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > +    }
> > +#endif
> >  }
> >  
> >  void host_cpuid(uint32_t function, uint32_t count,
> > -- 
> > 1.7.11.7
> 
> --
> 			Gleb.
Gleb Natapov - Jan. 7, 2013, 11:42 a.m.
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?

> > 
> > > +    if (kvm_enabled()) {
> > > +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > +    }
> > > +#endif
> > >  }
> > >  
> > >  void host_cpuid(uint32_t function, uint32_t count,
> > > -- 
> > > 1.7.11.7
> > 
> > --
> > 			Gleb.
> 
> -- 
> Eduardo

--
			Gleb.
Eduardo Habkost - Jan. 7, 2013, 12:09 p.m.
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.

> 
> > > 
> > > > +    if (kvm_enabled()) {
> > > > +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> > > > +    }
> > > > +#endif
> > > >  }
> > > >  
> > > >  void host_cpuid(uint32_t function, uint32_t count,
> > > > -- 
> > > > 1.7.11.7
> > > 
> > > --
> > > 			Gleb.
> > 
> > -- 
> > Eduardo
> 
> --
> 			Gleb.
Gleb Natapov - Jan. 7, 2013, 12:15 p.m.
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.
Eduardo Habkost - Jan. 7, 2013, 12:30 p.m.
On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote:
> 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.

If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef
around the real implementation. I mean, I don't think this:

  #ifdef CONFIG_KVM
  int enable_kvm_pv_eoi() {
    [...]
  }
  #endif

is any better than this:

  int enable_kvm_pv_eoi() {
  #ifdef CONFIG_KVM
    [...]
  #endif
  }

So this is probably a good reason to duplicate the KVM_FEATURE_*
#defines in the QEMU code, instead?
Gleb Natapov - Jan. 7, 2013, 12:33 p.m.
On Mon, Jan 07, 2013 at 10:30:40AM -0200, Eduardo Habkost wrote:
> On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote:
> > 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.
> 
> If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef
> around the real implementation. I mean, I don't think this:
> 
>   #ifdef CONFIG_KVM
>   int enable_kvm_pv_eoi() {
>     [...]
>   }
>   #endif
> 
You already have #ifdef CONFIG_KVM just above enable_kvm_pv_eoi(). Put
everything KVM related there instead of adding #ifdef CONFIG_KVM all
over the file.

> is any better than this:
> 
>   int enable_kvm_pv_eoi() {
>   #ifdef CONFIG_KVM
>     [...]
>   #endif
>   }
> 
> So this is probably a good reason to duplicate the KVM_FEATURE_*
> #defines in the QEMU code, instead?
> 
Not even duplicate, they can be fake just to keep compiler happy.

--
			Gleb.
Eduardo Habkost - Jan. 7, 2013, 1:01 p.m.
On Mon, Jan 07, 2013 at 02:33:25PM +0200, Gleb Natapov wrote:
> On Mon, Jan 07, 2013 at 10:30:40AM -0200, Eduardo Habkost wrote:
> > On Mon, Jan 07, 2013 at 02:15:59PM +0200, Gleb Natapov wrote:
> > > 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.
> > 
> > If we had an empty enable_kvm_pv_eoi() stub, we would need an #ifdef
> > around the real implementation. I mean, I don't think this:
> > 
> >   #ifdef CONFIG_KVM
> >   int enable_kvm_pv_eoi() {
> >     [...]
> >   }
> >   #endif
> > 
> You already have #ifdef CONFIG_KVM just above enable_kvm_pv_eoi(). Put
> everything KVM related there instead of adding #ifdef CONFIG_KVM all
> over the file.

But it also creates the need to write a separate stub function somewhere
else, while we could have a ready-to-use stub function automatically by
simply #ifdefing the whole function body. But anyway: this won't matter
if we choose the duplicate/fake #defines approach mentioned below.


> 
> > is any better than this:
> > 
> >   int enable_kvm_pv_eoi() {
> >   #ifdef CONFIG_KVM
> >     [...]
> >   #endif
> >   }
> > 
> > So this is probably a good reason to duplicate the KVM_FEATURE_*
> > #defines in the QEMU code, instead?
> > 
> Not even duplicate, they can be fake just to keep compiler happy.

I believe this would be even better. I will try that in the next version
of this series.

Patch

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
+    if (kvm_enabled()) {
+        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
+    }
+#endif
 }
 
 void host_cpuid(uint32_t function, uint32_t count,