Re: [Qemu-devel] [PATCH] kvm: Add helpers for checking and requiring kvm extensions

[Avi Kivity]

Juan Quintela wrote:
> Avi Kivity <address@hidden> wrote:
>
> Hi
>
>   
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 36659a9..1642a2a 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -64,6 +64,30 @@ struct KVMState
> >  
> >  static KVMState *kvm_state;
> >  
> > +int kvm_check_extension(int extension)
> > +{
> > +    int ret;
> > +
> > +    ret = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, extension);
> > +    if (ret < 0) {
> > +        fprintf(stderr, "KVM_CHECK_EXTENSION failed: %s\n", strerror(errno));
> > +        exit(1);
> > +    }
> > +    return ret;
> > +}
> >
> >     
> Are you sure you want the exit(1) in this case?
>   

A negative result of kvm_ioctl() means the ioctl itself failed.  This 
shouldn't happen, so we exit.

If the extension is not present, it returns 0.

> With the exit() call, you are unable to check if one extension is
> present at all.  And you check the return of the following code.
>   
> >      s->coalesced_mmio = 0;
> >  #ifdef KVM_CAP_COALESCED_MMIO
> > -    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_COALESCED_MMIO);
> > -    if (ret > 0)
> > -        s->coalesced_mmio = ret;
> > +    s->coalesced_mmio = kvm_check_extension(KVM_CAP_COALESCED_MMIO);
> >  #endif
> >     
>
>   

Here we use the return value (0 or 1).

> You can remove the ifdef at this point.
>   

It won't compile if KVM_CAP_COLAESCED_MMIO is not defined.

--
Do not meddle in the internals of kernels, for they are subtle and quick to 
panic.