diff mbox

[2/2] target-arm: Add "anyvfp" CPU

Message ID 1436208832-17651-3-git-send-email-meadori@codesourcery.com
State New
Headers show

Commit Message

Meador Inge July 6, 2015, 6:53 p.m. UTC
From: Meador Inge <meadori@codesourcery.com>

This patch adds support for `-cpu anyvfp`, which is just
like `-cpu any`, but enables the coprocessors by default.
This is useful for working with a wide range of bare-metal
C/C++ applications that have been compiled in different
ways and assume the coprocessor has been enabled already.

Original patch by Daniel Jacobowitz.

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 target-arm/cpu.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Peter Crosthwaite July 6, 2015, 10 p.m. UTC | #1
On Mon, Jul 6, 2015 at 11:53 AM,  <meadori@codesourcery.com> wrote:
> From: Meador Inge <meadori@codesourcery.com>
>
> This patch adds support for `-cpu anyvfp`, which is just
> like `-cpu any`, but enables the coprocessors by default.

So a better way to do this is via QOM properties. You can propertyify
VFP support on the QOM type ARMCPU then users can use -global to set
in on the command line. You could do this for any number of ARM CPU
features you care about to create the combos you want on the command
line rather than having an in tree CPU def for special cases.

Have a look at the way the has_el3 feature is handled as a CPU property.

> This is useful for working with a wide range of bare-metal
> C/C++ applications that have been compiled in different
> ways and assume the coprocessor has been enabled already.
>
> Original patch by Daniel Jacobowitz.

Do you have originals in a git with the author SOBs that you can just
amend with your own editor notes?

>
> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> ---
>  target-arm/cpu.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 3665f6b..2a00e73 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -181,7 +181,14 @@ static void arm_cpu_reset(CPUState *s)
>          env->regs[15] = 0xFFFF0000;
>      }
>
> -    env->vfp.xregs[ARM_VFP_FPEXC] = 0;
> +    /* For -cpu anyvfp, enable coprocessors by default.  Useful for
> +       testing code that expects something else to turn on the
> +       coprocessor.  */

Comment style should be this:

/* multi line comment style
 * needs *'s on each line.
 */

> +    if (cpu->midr == 0xfffffffe) {
> +        env->vfp.xregs[ARM_VFP_FPEXC] = 1 << 30;
> +    } else {
> +        env->vfp.xregs[ARM_VFP_FPEXC] = 0;
> +    }
>  #endif
>      set_flush_to_zero(1, &env->vfp.standard_fp_status);
>      set_flush_inputs_to_zero(1, &env->vfp.standard_fp_status);
> @@ -1258,6 +1265,13 @@ static void arm_any_initfn(Object *obj)
>      cpu->midr = 0xffffffff;
>  }
>
> +static void arm_anyvfp_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +    arm_any_initfn(obj);
> +    cpu->midr = 0xfffffffe;


Do you need to set the MIDR like this or is it just a means to
communicate feature support?

Regards,
Peter

> +}
> +
>  #endif /* !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) */
>
>  typedef struct ARMCPUInfo {
> @@ -1304,6 +1318,7 @@ static const ARMCPUInfo arm_cpus[] = {
>      { .name = "pxa270-c0",   .initfn = pxa270c0_initfn },
>      { .name = "pxa270-c5",   .initfn = pxa270c5_initfn },
>      { .name = "any",         .initfn = arm_any_initfn },
> +    { .name = "anyvfp",      .initfn = arm_anyvfp_initfn },
>  #endif
>      { .name = NULL }
>  };
> --
> 1.8.1.1
>
>
Peter Maydell July 6, 2015, 10:31 p.m. UTC | #2
On 6 July 2015 at 19:53,  <meadori@codesourcery.com> wrote:
> From: Meador Inge <meadori@codesourcery.com>
>
> This patch adds support for `-cpu anyvfp`, which is just
> like `-cpu any`, but enables the coprocessors by default.
> This is useful for working with a wide range of bare-metal
> C/C++ applications that have been compiled in different
> ways and assume the coprocessor has been enabled already.

I'm not convinced. System mode gives you a bare metal
system -- it's the bare metal app's job to enable
VFP if it wants to use it. If your bare metal app
doesn't do that then it is broken.

Alternatively you can use a firmware blob which
does enough setup for whatever environment your
code is expecting.

I really don't want to get QEMU into the business of
providing random ad-hoc execution environments for
things. It's hard enough maintaining the Linux boot
loader code...

thanks
-- PMM
Meador Inge July 13, 2015, 8:21 p.m. UTC | #3
On Mon, Jul 06, 2015 at 03:00:20PM -0700, Peter Crosthwaite wrote:

> So a better way to do this is via QOM properties. You can propertyify
> VFP support on the QOM type ARMCPU then users can use -global to set
> in on the command line. You could do this for any number of ARM CPU
> features you care about to create the combos you want on the command
> line rather than having an in tree CPU def for special cases.
> 
> Have a look at the way the has_el3 feature is handled as a CPU property.

I will look into that.  Thank you.

> Do you have originals in a git with the author SOBs that you can just
> amend with your own editor notes?

Unfortunately for this case, no.  This is a very hold patch that was
originally in an SVN mirror.

-- Meador
Meador Inge July 13, 2015, 8:36 p.m. UTC | #4
On Mon, Jul 06, 2015 at 11:31:37PM +0100, Peter Maydell wrote:

> I'm not convinced. System mode gives you a bare metal
> system -- it's the bare metal app's job to enable
> VFP if it wants to use it. If your bare metal app
> doesn't do that then it is broken.

Fair enough.  I knew this patch would be a bit of a stretch :-)

Thanks.

-- Meador
Peter Crosthwaite July 13, 2015, 9:32 p.m. UTC | #5
On Mon, Jul 13, 2015 at 1:21 PM, Meador Inge <meadori@codesourcery.com> wrote:
> On Mon, Jul 06, 2015 at 03:00:20PM -0700, Peter Crosthwaite wrote:
>
>> So a better way to do this is via QOM properties. You can propertyify
>> VFP support on the QOM type ARMCPU then users can use -global to set
>> in on the command line. You could do this for any number of ARM CPU
>> features you care about to create the combos you want on the command
>> line rather than having an in tree CPU def for special cases.
>>
>> Have a look at the way the has_el3 feature is handled as a CPU property.
>
> I will look into that.  Thank you.
>
>> Do you have originals in a git with the author SOBs that you can just
>> amend with your own editor notes?
>
> Unfortunately for this case, no.  This is a very hold patch that was
> originally in an SVN mirror.
>

Ok, not a problem.

Regards,
Peter

> -- Meador
>
diff mbox

Patch

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 3665f6b..2a00e73 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -181,7 +181,14 @@  static void arm_cpu_reset(CPUState *s)
         env->regs[15] = 0xFFFF0000;
     }
 
-    env->vfp.xregs[ARM_VFP_FPEXC] = 0;
+    /* For -cpu anyvfp, enable coprocessors by default.  Useful for
+       testing code that expects something else to turn on the
+       coprocessor.  */
+    if (cpu->midr == 0xfffffffe) {
+        env->vfp.xregs[ARM_VFP_FPEXC] = 1 << 30;
+    } else {
+        env->vfp.xregs[ARM_VFP_FPEXC] = 0;
+    }
 #endif
     set_flush_to_zero(1, &env->vfp.standard_fp_status);
     set_flush_inputs_to_zero(1, &env->vfp.standard_fp_status);
@@ -1258,6 +1265,13 @@  static void arm_any_initfn(Object *obj)
     cpu->midr = 0xffffffff;
 }
 
+static void arm_anyvfp_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    arm_any_initfn(obj);
+    cpu->midr = 0xfffffffe;
+}
+
 #endif /* !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64) */
 
 typedef struct ARMCPUInfo {
@@ -1304,6 +1318,7 @@  static const ARMCPUInfo arm_cpus[] = {
     { .name = "pxa270-c0",   .initfn = pxa270c0_initfn },
     { .name = "pxa270-c5",   .initfn = pxa270c5_initfn },
     { .name = "any",         .initfn = arm_any_initfn },
+    { .name = "anyvfp",      .initfn = arm_anyvfp_initfn },
 #endif
     { .name = NULL }
 };