Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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.
--
Eduardo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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.
--
Eduardo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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?
--
Eduardo
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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
Re: [PATCH qom-cpu 01/11] target-i386: Don't set any KVM flag by default if KVM is disabled
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 ehabk...@redhat.com
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin m...@redhat.com
Cc: Gleb Natapov g...@redhat.com
Cc: Marcelo Tosatti mtosa...@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.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
