[Qemu-devel] [PATCH] target-arm: fix build on fedora

[Qemu-devel] [PATCH] target-arm: fix build on fedora

[Michael S. Tsirkin]

commit 5ce4f35781028ce1aee3341e6002f925fdc7aaf3
    "target-arm: A64: add set_pc cpu method"

introduces an array aarch64_cpus which is zero
size if this code is built without CONFIG_USER_ONLY.
In particular an attempt to iterate over this array produces a warning:

 CC    aarch64-softmmu/target-arm/cpu64.o
/scm/qemu/target-arm/cpu64.c: In function ‘aarch64_cpu_register_types’:
/scm/qemu/target-arm/cpu64.c:124:5: error: comparison of unsigned
expression < 0 is always false [-Werror=type-limits]
     for (i = 0; i < ARRAY_SIZE(aarch64_cpus); i++) {
     ^
cc1: all warnings being treated as errors

This is the result of ARRAY_SIZE being an unsigned type,
causing i to be promoted to unsigned int as well.

As zero size arrays are a gcc extension, it seems
cleanest to add a dummy element with NULL name,
and test for it during registration.

Cc: Alexander Graf <agraf@suse.de>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

I have queued this in my tree since it prevents me from
being able to build and test properly.
Pls review and ack.

 target-arm/cpu64.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 04ce879..2efe189 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -58,6 +58,7 @@ static const ARMCPUInfo aarch64_cpus[] = {
 #ifdef CONFIG_USER_ONLY
     { .name = "any",         .initfn = aarch64_any_initfn },
 #endif
+    { .name = NULL }
 };
 
 static void aarch64_cpu_initfn(Object *obj)
@@ -100,6 +101,10 @@ static void aarch64_cpu_register(const ARMCPUInfo *info)
         .class_init = info->class_init,
     };
 
+    if (!info->name) {
+        return;
+    }
+
     type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
     type_register(&type_info);
     g_free((void *)type_info.name);
-- 
MST

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Peter Maydell]

On 23 December 2013 11:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> commit 5ce4f35781028ce1aee3341e6002f925fdc7aaf3
>     "target-arm: A64: add set_pc cpu method"
>
> introduces an array aarch64_cpus which is zero
> size if this code is built without CONFIG_USER_ONLY.
> In particular an attempt to iterate over this array produces a warning:
>
>  CC    aarch64-softmmu/target-arm/cpu64.o
> /scm/qemu/target-arm/cpu64.c: In function ‘aarch64_cpu_register_types’:
> /scm/qemu/target-arm/cpu64.c:124:5: error: comparison of unsigned
> expression < 0 is always false [-Werror=type-limits]
>      for (i = 0; i < ARRAY_SIZE(aarch64_cpus); i++) {
>      ^
> cc1: all warnings being treated as errors
>
> This is the result of ARRAY_SIZE being an unsigned type,
> causing i to be promoted to unsigned int as well.

I guess this is a new gcc warning, since this all builds
fine for me (gcc 4.6.3).

> As zero size arrays are a gcc extension, it seems
> cleanest to add a dummy element with NULL name,
> and test for it during registration.
>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> I have queued this in my tree since it prevents me from
> being able to build and test properly.
> Pls review and ack.
>
>  target-arm/cpu64.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index 04ce879..2efe189 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -58,6 +58,7 @@ static const ARMCPUInfo aarch64_cpus[] = {
>  #ifdef CONFIG_USER_ONLY
>      { .name = "any",         .initfn = aarch64_any_initfn },
>  #endif
> +    { .name = NULL }
>  };
>
>  static void aarch64_cpu_initfn(Object *obj)
> @@ -100,6 +101,10 @@ static void aarch64_cpu_register(const ARMCPUInfo *info)
>          .class_init = info->class_init,
>      };
>
> +    if (!info->name) {
> +        return;
> +    }
> +
>      type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
>      type_register(&type_info);
>      g_free((void *)type_info.name);

At a minimum, if we take this approach we should add TODO comments
to the effect that the NULL terminator and the if() can be removed
when the first real AArch64 CPU is added.

I think I'd rather put the if (!info->name) continue into the function
which is doing the looping over the array.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Paolo Bonzini]

Il 23/12/2013 13:37, Peter Maydell ha scritto:
> At a minimum, if we take this approach we should add TODO comments
> to the effect that the NULL terminator and the if() can be removed
> when the first real AArch64 CPU is added.
> 
> I think I'd rather put the if (!info->name) continue into the function
> which is doing the looping over the array.

Or just change the termination condition from a check on the array size
to one on info->name.

Paolo

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Peter Maydell]

On 23 December 2013 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 23/12/2013 13:37, Peter Maydell ha scritto:
>> At a minimum, if we take this approach we should add TODO comments
>> to the effect that the NULL terminator and the if() can be removed
>> when the first real AArch64 CPU is added.
>>
>> I think I'd rather put the if (!info->name) continue into the function
>> which is doing the looping over the array.
>
> Or just change the termination condition from a check on the array size
> to one on info->name.

That would take it out of line with the equivalent 32 bit ARM code
(and also moxie and openrisc for what little that's worth) and be
fractionally more tedious to revert later.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Stefan Weil]

Am 23.12.2013 13:59, schrieb Peter Maydell:
> On 23 December 2013 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 23/12/2013 13:37, Peter Maydell ha scritto:
>>> At a minimum, if we take this approach we should add TODO comments
>>> to the effect that the NULL terminator and the if() can be removed
>>> when the first real AArch64 CPU is added.
>>>
>>> I think I'd rather put the if (!info->name) continue into the function
>>> which is doing the looping over the array.
>> Or just change the termination condition from a check on the array size
>> to one on info->name.
> That would take it out of line with the equivalent 32 bit ARM code
> (and also moxie and openrisc for what little that's worth) and be
> fractionally more tedious to revert later.
>
> thanks
> -- PMM

What about adding a dummy CPU (which can be removed later)?

I also got a warning here when I used cgcc / smatch recently, but did
not send a patch because there are too many possible fixes and none of
them seemed to be elegant :-)

Nevertheless, here is one of them:

--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -46,6 +46,11 @@ static void aarch64_any_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V7MP);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
 }
+#else
+static void aarch64_dummy_initfn(Object *obj)
+{
+    hw_error("Dummy CPU not supported");
+}
 #endif
 
 typedef struct ARMCPUInfo {
@@ -57,6 +62,9 @@ typedef struct ARMCPUInfo {
 static const ARMCPUInfo aarch64_cpus[] = {
 #ifdef CONFIG_USER_ONLY
     { .name = "any",         .initfn = aarch64_any_initfn },
+#else
+    /* TODO: Dummy CPU to avoid empty array. Fix when a real CPU is
added. */
+    { .name = "dummy",       .initfn = aarch64_dummy_initfn },
 #endif
 };

Of course, any other fix is also okay.

Cheers and Merry Christmas

Stefan

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Peter Maydell]

On 23 December 2013 13:32, Stefan Weil <sw@weilnetz.de> wrote:
> Am 23.12.2013 13:59, schrieb Peter Maydell:
>> On 23 December 2013 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 23/12/2013 13:37, Peter Maydell ha scritto:
>>>> At a minimum, if we take this approach we should add TODO comments
>>>> to the effect that the NULL terminator and the if() can be removed
>>>> when the first real AArch64 CPU is added.
>>>>
>>>> I think I'd rather put the if (!info->name) continue into the function
>>>> which is doing the looping over the array.
>>> Or just change the termination condition from a check on the array size
>>> to one on info->name.
>> That would take it out of line with the equivalent 32 bit ARM code
>> (and also moxie and openrisc for what little that's worth) and be
>> fractionally more tedious to revert later.

> What about adding a dummy CPU (which can be removed later)?

That would be user-visible, which seems a bad thing.
I agree that there aren't any fantastic solutions here;
I think something more or less like Michael's patch with
a TODO note so it's easy for me to take it out again when
I add an actual A57 emulation in a couple of months will
do. This is just a temporary thing since at the moment we
only support -cpu any for userspace and -cpu host for KVM.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Michael S. Tsirkin]

On Mon, Dec 23, 2013 at 01:41:50PM +0000, Peter Maydell wrote:
> On 23 December 2013 13:32, Stefan Weil <sw@weilnetz.de> wrote:
> > Am 23.12.2013 13:59, schrieb Peter Maydell:
> >> On 23 December 2013 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>> Il 23/12/2013 13:37, Peter Maydell ha scritto:
> >>>> At a minimum, if we take this approach we should add TODO comments
> >>>> to the effect that the NULL terminator and the if() can be removed
> >>>> when the first real AArch64 CPU is added.
> >>>>
> >>>> I think I'd rather put the if (!info->name) continue into the function
> >>>> which is doing the looping over the array.
> >>> Or just change the termination condition from a check on the array size
> >>> to one on info->name.
> >> That would take it out of line with the equivalent 32 bit ARM code
> >> (and also moxie and openrisc for what little that's worth) and be
> >> fractionally more tedious to revert later.
> 
> > What about adding a dummy CPU (which can be removed later)?
> 
> That would be user-visible, which seems a bad thing.
> I agree that there aren't any fantastic solutions here;
> I think something more or less like Michael's patch with
> a TODO note so it's easy for me to take it out again when
> I add an actual A57 emulation in a couple of months will
> do. This is just a temporary thing since at the moment we
> only support -cpu any for userspace and -cpu host for KVM.
> 
> thanks
> -- PMM

OK, ack my patch then? I'll add a comment
/* TODO: remove when we support more CPUs. */

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Peter Maydell]

On 23 December 2013 14:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> OK, ack my patch then? I'll add a comment
> /* TODO: remove when we support more CPUs. */

Post an updated patch (with TODO comments in both
places) and I'll ack it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Andreas Färber]

Am 23.12.2013 13:37, schrieb Peter Maydell:
> On 23 December 2013 11:56, Michael S. Tsirkin <mst@redhat.com> wrote:
>> commit 5ce4f35781028ce1aee3341e6002f925fdc7aaf3
>>     "target-arm: A64: add set_pc cpu method"
>>
>> introduces an array aarch64_cpus which is zero
>> size if this code is built without CONFIG_USER_ONLY.
>> In particular an attempt to iterate over this array produces a warning:
>>
>>  CC    aarch64-softmmu/target-arm/cpu64.o
>> /scm/qemu/target-arm/cpu64.c: In function ‘aarch64_cpu_register_types’:
>> /scm/qemu/target-arm/cpu64.c:124:5: error: comparison of unsigned
>> expression < 0 is always false [-Werror=type-limits]
>>      for (i = 0; i < ARRAY_SIZE(aarch64_cpus); i++) {
>>      ^
>> cc1: all warnings being treated as errors
>>
>> This is the result of ARRAY_SIZE being an unsigned type,
>> causing i to be promoted to unsigned int as well.
> 
> I guess this is a new gcc warning, since this all builds
> fine for me (gcc 4.6.3).

No problem noticed with 4.8.1 on today's master either.

>> As zero size arrays are a gcc extension, it seems
>> cleanest to add a dummy element with NULL name,
>> and test for it during registration.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>
>> I have queued this in my tree since it prevents me from
>> being able to build and test properly.
>> Pls review and ack.
>>
>>  target-arm/cpu64.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
>> index 04ce879..2efe189 100644
>> --- a/target-arm/cpu64.c
>> +++ b/target-arm/cpu64.c
>> @@ -58,6 +58,7 @@ static const ARMCPUInfo aarch64_cpus[] = {
>>  #ifdef CONFIG_USER_ONLY
>>      { .name = "any",         .initfn = aarch64_any_initfn },
>>  #endif
>> +    { .name = NULL }
>>  };
>>
>>  static void aarch64_cpu_initfn(Object *obj)
>> @@ -100,6 +101,10 @@ static void aarch64_cpu_register(const ARMCPUInfo *info)
>>          .class_init = info->class_init,
>>      };
>>
>> +    if (!info->name) {
>> +        return;
>> +    }
>> +
>>      type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
>>      type_register(&type_info);
>>      g_free((void *)type_info.name);
> 
> At a minimum, if we take this approach we should add TODO comments
> to the effect that the NULL terminator and the if() can be removed
> when the first real AArch64 CPU is added.
> 
> I think I'd rather put the if (!info->name) continue into the function
> which is doing the looping over the array.

While I share your sentiment wrt this workaround, what's the status of
getting a real 64-bit CPU applied? Isn't the Cortex-A57/A53 CPU
independent of whether we have all MPCore etc. pieces in place? That
would seem the most elegant solution to me, even if not "usable" yet.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Peter Maydell]

On 23 December 2013 13:45, Andreas Färber <afaerber@suse.de> wrote:
> While I share your sentiment wrt this workaround, what's the status of
> getting a real 64-bit CPU applied? Isn't the Cortex-A57/A53 CPU
> independent of whether we have all MPCore etc. pieces in place? That
> would seem the most elegant solution to me, even if not "usable" yet.

It's next on my todo list after we get the usermode instruction
emulation wrapped up, so should be sometime in the next
few months, as part of the system emulation work.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: fix build on fedora

[Michael S. Tsirkin]

On Mon, Dec 23, 2013 at 12:37:41PM +0000, Peter Maydell wrote:
> On 23 December 2013 11:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> > commit 5ce4f35781028ce1aee3341e6002f925fdc7aaf3
> >     "target-arm: A64: add set_pc cpu method"
> >
> > introduces an array aarch64_cpus which is zero
> > size if this code is built without CONFIG_USER_ONLY.
> > In particular an attempt to iterate over this array produces a warning:
> >
> >  CC    aarch64-softmmu/target-arm/cpu64.o
> > /scm/qemu/target-arm/cpu64.c: In function ‘aarch64_cpu_register_types’:
> > /scm/qemu/target-arm/cpu64.c:124:5: error: comparison of unsigned
> > expression < 0 is always false [-Werror=type-limits]
> >      for (i = 0; i < ARRAY_SIZE(aarch64_cpus); i++) {
> >      ^
> > cc1: all warnings being treated as errors
> >
> > This is the result of ARRAY_SIZE being an unsigned type,
> > causing i to be promoted to unsigned int as well.
> 
> I guess this is a new gcc warning, since this all builds
> fine for me (gcc 4.6.3).

I see this with gcc 4.8.2 on Fedora 19.

> > As zero size arrays are a gcc extension, it seems
> > cleanest to add a dummy element with NULL name,
> > and test for it during registration.
> >
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > I have queued this in my tree since it prevents me from
> > being able to build and test properly.
> > Pls review and ack.
> >
> >  target-arm/cpu64.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> > index 04ce879..2efe189 100644
> > --- a/target-arm/cpu64.c
> > +++ b/target-arm/cpu64.c
> > @@ -58,6 +58,7 @@ static const ARMCPUInfo aarch64_cpus[] = {
> >  #ifdef CONFIG_USER_ONLY
> >      { .name = "any",         .initfn = aarch64_any_initfn },
> >  #endif
> > +    { .name = NULL }
> >  };
> >
> >  static void aarch64_cpu_initfn(Object *obj)
> > @@ -100,6 +101,10 @@ static void aarch64_cpu_register(const ARMCPUInfo *info)
> >          .class_init = info->class_init,
> >      };
> >
> > +    if (!info->name) {
> > +        return;
> > +    }
> > +
> >      type_info.name = g_strdup_printf("%s-" TYPE_ARM_CPU, info->name);
> >      type_register(&type_info);
> >      g_free((void *)type_info.name);
> 
> At a minimum, if we take this approach we should add TODO comments
> to the effect that the NULL terminator and the if() can be removed
> when the first real AArch64 CPU is added.
> 
> I think I'd rather put the if (!info->name) continue into the function
> which is doing the looping over the array.
> 
> thanks
> -- PMM