Re: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f, uint32_t
mask)
{
int i;
@@ -987,6 +988,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
*opaque,
const char *name, Error **errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def) enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
I will do. Igor probably will have to change his target-i386: move
kvm_check_features_against_host() check to realize time patch to use
the same approach, too.
--
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: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Mon, 7 Jan 2013 10:00:09 -0200
Eduardo Habkost ehabk...@redhat.com wrote:
On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f, uint32_t
mask)
{
int i;
@@ -987,6 +988,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
*opaque,
const char *name, Error **errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def)
enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
I will do. Igor probably will have to change his target-i386: move
kvm_check_features_against_host() check to realize time patch to use
the same approach, too.
Gleb,
Why do stub here? As result we will be adding more ifdef-s just in other
places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
kvm_check_features_against_host() are bundled together in cpu.c so we could
instead ifdef whole block. Like here:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
For me code looks more readable with ifdef here, if we have stub, a reader
would have to look at kvm_check_features_against_host() body to see if it does
anything.
--
Eduardo
--
Regards,
Igor
--
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: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:
On Mon, 7 Jan 2013 10:00:09 -0200
Eduardo Habkost ehabk...@redhat.com wrote:
On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t
*x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f,
uint32_t mask)
{
int i;
@@ -987,6 +988,7 @@ static int
kvm_check_features_against_host(x86_def_t *guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
*opaque,
const char *name, Error
**errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def)
enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
I will do. Igor probably will have to change his target-i386: move
kvm_check_features_against_host() check to realize time patch to use
the same approach, too.
Gleb,
Why do stub here? As result we will be adding more ifdef-s just in other
places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
kvm_check_features_against_host() are bundled together in cpu.c so we could
instead ifdef whole block. Like here:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
For me code looks more readable with ifdef here, if we have stub, a reader
would have to look at kvm_check_features_against_host() body to see if it does
anything.
If CONFIG_KVM is not set, kvm_enabled() is always zero, so the function
would never be called, so I find the ifdef-less code more readable and
obvious.
What I don't know is if we should do this:
#ifdef CONFIG_KVM
static int kvm_check_features_against_host(...)
{
/* real implementation here */
}
static int kvm_do_something_else(...)
{
/* real implementation here */
}
/* Other kvm_* functions here */
#else
static int kvm_check_features_against_host(...)
{
}
static int kvm_do_something_else(...)
{
}
/* Other kvm_* stubs here */
#endif /* CONFIG_KVM */
Or this:
static int kvm_check_features_against_host(...)
{
#ifdef CONFIG_KVM
/* real implementation here */
#endif /* CONFIG_KVM */
}
static int kvm_do_something_else(...)
{
#ifdef CONFIG_KVM
/* real implementation here */
#endif /* CONFIG_KVM */
}
I believe the latter is better, but based on Gleb's comments about
enable_kvm_pv_eoi(), he seems to prefer the former.
--
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: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:
On Mon, 7 Jan 2013 10:00:09 -0200
Eduardo Habkost ehabk...@redhat.com wrote:
On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t
*x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f,
uint32_t mask)
{
int i;
@@ -987,6 +988,7 @@ static int
kvm_check_features_against_host(x86_def_t *guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
*opaque,
const char *name, Error
**errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def)
enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
I will do. Igor probably will have to change his target-i386: move
kvm_check_features_against_host() check to realize time patch to use
the same approach, too.
Gleb,
Why do stub here? As result we will be adding more ifdef-s just in other
places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
Why will we be adding more ifdef-s in other places?
kvm_check_features_against_host() are bundled together in cpu.c so we could
instead ifdef whole block. Like here:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
That's fine, but you can avoid things like:
if (kvm_enabled() name strcmp(name, host) == 0) {
+#ifdef CONFIG_KVM
kvm_cpu_fill_host(x86_cpu_def);
+#endif
in your patch by providing stub for kvm_cpu_fill_host() for !CONFIG_KVM
case. This is common practice really. Avoid ifdefs in the code.
For me code looks more readable with ifdef here, if we have stub, a reader
would have to look at kvm_check_features_against_host() body to see if it does
anything.
If reader cares about kvm it has to anyway. If he does not, there is
friendly kvm_enabled() (which is stub in case of !CONFIG_KVM BTW) to
tell him that he does not care. No need additional ifdef there.
--
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: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Mon, 7 Jan 2013 15:30:26 +0200
Gleb Natapov g...@redhat.com wrote:
On Mon, Jan 07, 2013 at 02:15:14PM +0100, Igor Mammedov wrote:
On Mon, 7 Jan 2013 10:00:09 -0200
Eduardo Habkost ehabk...@redhat.com wrote:
On Sun, Jan 06, 2013 at 04:27:19PM +0200, Gleb Natapov wrote:
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t
*x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f,
uint32_t mask)
{
int i;
@@ -987,6 +988,7 @@ static int
kvm_check_features_against_host(x86_def_t *guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v,
void *opaque,
const char *name, Error
**errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def)
enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
I will do. Igor probably will have to change his target-i386: move
kvm_check_features_against_host() check to realize time patch to use
the same approach, too.
Gleb,
Why do stub here? As result we will be adding more ifdef-s just in other
places. Currently kvm_cpu_fill_host(), unavailable_host_feature() and
Why will we be adding more ifdef-s in other places?
unavailable_host_feature() is being ifdef-ed above
kvm_check_features_against_host() are bundled together in cpu.c so we could
instead ifdef whole block. Like here:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg146536.html
That's fine, but you can avoid things like:
if (kvm_enabled() name strcmp(name, host) == 0) {
+#ifdef CONFIG_KVM
kvm_cpu_fill_host(x86_cpu_def);
+#endif
in your patch by providing stub for kvm_cpu_fill_host() for !CONFIG_KVM
case. This is common practice really. Avoid ifdefs in the code.
This ifdef could be eliminated later when cpus are converted into sub-classes.
Then we would put host subclass close to kvm_cpu_fill_host inside of the same
ifdef. that would leave ifdef around kvm_check_features_against_host() in
cpu_x86_parse_featurestr().
For me code looks more readable with ifdef here, if we have stub, a reader
would have to look at kvm_check_features_against_host() body to see if it
does
anything.
If reader cares about kvm it has to anyway. If he does not, there is
friendly kvm_enabled() (which is stub in case of !CONFIG_KVM BTW) to
tell him that he does not care. No need additional ifdef there.
both ways would work, but if stubs are preferred style then there is no
point arguing.
--
Gleb.
--
Regards,
Igor
--
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: [Qemu-devel] [PATCH qom-cpu 10/11] target-i386: Call kvm_check_features_against_host() only if CONFIG_KVM is set
On Fri, Jan 04, 2013 at 08:01:11PM -0200, Eduardo Habkost wrote:
This will be necessary once kvm_check_features_against_host() starts
using KVM-specific definitions (so it won't compile anymore if
CONFIG_KVM is not set).
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
target-i386/cpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c3c7e1..876b0f6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -936,6 +936,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
#endif /* CONFIG_KVM */
}
+#ifdef CONFIG_KVM
static int unavailable_host_feature(struct model_features_t *f, uint32_t
mask)
{
int i;
@@ -987,6 +988,7 @@ static int kvm_check_features_against_host(x86_def_t
*guest_def)
}
return rv;
}
+#endif
static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
*opaque,
const char *name, Error **errp)
@@ -1410,10 +1412,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
*x86_cpu_def, char *features)
x86_cpu_def-kvm_features = ~minus_kvm_features;
x86_cpu_def-svm_features = ~minus_svm_features;
x86_cpu_def-cpuid_7_0_ebx_features = ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
if (check_cpuid kvm_enabled()) {
if (kvm_check_features_against_host(x86_cpu_def) enforce_cpuid)
goto error;
}
+#endif
Provide kvm_check_features_against_host() stub if !CONFIG_KVM and drop
ifdef here.
return 0;
error:
--
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
