Re: [Qemu-devel] [PATCH] kvm: Fix enable_cap helpers on older gcc

[Cornelia Huck]

On Mon, 12 May 2014 09:33:39 +0200
Thomas Huth <address@hidden> wrote:

> On Sun, 11 May 2014 18:11:04 +0200
> Alexander Graf <address@hidden> wrote:
> 
> > Commit 40f1ee27aa1 introduced handy helpers for enable_cap calls on
> > vcpu and vm level. Unfortunately some older gcc versions (4.7.1, 4.6)
> > seem to choke on signedness detection in inline created variables:
> > 
> > target-ppc/kvm.c: In function 'kvmppc_booke_watchdog_enable':
> > target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0 is 
> > always false [-Werror=type-limits]
> > target-ppc/kvm.c: In function 'kvmppc_set_papr':
> > target-ppc/kvm.c:1504:21: error: comparison of unsigned expression < 0 is 
> > always false [-Werror=type-limits]
> > 
> > We can easily work around this by not doing a compare with 0, but instead
> > shift all values up by one. The resulting binary should look the same, as
> > all those ARRAY_SIZE values are compile time statically optimized.
> > 
> > This patch fixes compile errors on some of my systems.
> > 
> > Signed-off-by: Alexander Graf <address@hidden>
> > ---
> >  include/sysemu/kvm.h | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 192fe89..989c261 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -302,9 +302,9 @@ int kvm_check_extension(KVMState *s, unsigned int 
> > extension);
> >          };                                                           \
> >          uint64_t args_tmp[] = { __VA_ARGS__ };                       \
> >          int i;                                                       \
> > -        for (i = 0; i < ARRAY_SIZE(args_tmp) &&                      \
> > -                     i < ARRAY_SIZE(cap.args); i++) {                \
> > -            cap.args[i] = args_tmp[i];                               \
> > +        for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp),                   \
> > +                             ARRAY_SIZE(cap.args)) + 1); i++) {      \
> > +            cap.args[i - 1] = args_tmp[i - 1];                       \
> >          }                                                            \
> >          kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap);                       \
> >      })
> > @@ -317,9 +317,9 @@ int kvm_check_extension(KVMState *s, unsigned int 
> > extension);
> >          };                                                           \
> >          uint64_t args_tmp[] = { __VA_ARGS__ };                       \
> >          int i;                                                       \
> > -        for (i = 0; i < ARRAY_SIZE(args_tmp) &&                      \
> > -                     i < ARRAY_SIZE(cap.args); i++) {                \
> > -            cap.args[i] = args_tmp[i];                               \
> > +        for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp),                   \
> > +                             ARRAY_SIZE(cap.args)) + 1); i++) {      \
> > +            cap.args[i - 1] = args_tmp[i - 1];                       \
> >          }                                                            \
> >          kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap);                   \
> >      })
> 
> That looks ... somewhat ugly. I think you either should add a proper
> comment here (or somebody will fix it back in the future when reading
> the code), ... or would it also work to type-cast the result of
> ARRAY_SIZE to "int"? Something like this:
> 
>         for (i = 0; i < (int)ARRAY_SIZE(args_tmp) &&                   \
>                      i < (int)ARRAY_SIZE(cap.args); i++)

If that one works, I'd prefer that. The other version hurts my eyes :(

If it doesn't, I agree it should be properly commented.