| Message ID | 1357336872-7200-2-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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.
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.
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?
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.
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.
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,
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(-)