Re: [libvirt] [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 <gleb(a)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 <ehabkost(a)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 <ehabkost(a)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