[Qemu-devel] [PATCH] target-arm: Add VBAR support to ARM1176 CPUs

[Qemu-devel] [PATCH] target-arm: Add VBAR support to ARM1176 CPUs

[Cédric Le Goater]

ARM1176 CPUs support the Vector Base Address Register but currently,
qemu only supports VBAR on ARMv7 CPUs. Fix this by adding a new
feature ARM_FEATURE_VBAR which is used for ARMv7 and ARM1176 CPUs.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target-arm/cpu.c    |    2 ++
 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   18 ++++++++++++------
 3 files changed, 15 insertions(+), 6 deletions(-)

Index: qemu-aspeed.git/target-arm/helper.c
===================================================================
--- qemu-aspeed.git.orig/target-arm/helper.c
+++ qemu-aspeed.git/target-arm/helper.c
@@ -1251,12 +1251,6 @@ static const ARMCPRegInfo v7_cp_reginfo[
       .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .writefn = pmintenclr_write },
-    { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
-      .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL1_RW, .writefn = vbar_write,
-      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s),
-                             offsetof(CPUARMState, cp15.vbar_ns) },
-      .resetvalue = 0 },
     { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
       .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_RAW },
@@ -1410,6 +1404,15 @@ static const ARMCPRegInfo v6k_cp_reginfo
       .resetvalue = 0 },
     REGINFO_SENTINEL
 };
+static const ARMCPRegInfo vbar_cp_reginfo[] = {
+    { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
+      .access = PL1_RW, .writefn = vbar_write,
+      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s),
+                             offsetof(CPUARMState, cp15.vbar_ns) },
+      .resetvalue = 0 },
+    REGINFO_SENTINEL
+};
 
 #ifndef CONFIG_USER_ONLY
 
@@ -4486,6 +4489,9 @@ void register_cp_regs_for_features(ARMCP
     if (arm_feature(env, ARM_FEATURE_V6K)) {
         define_arm_cp_regs(cpu, v6k_cp_reginfo);
     }
+    if (arm_feature(env, ARM_FEATURE_VBAR)) {
+        define_arm_cp_regs(cpu, vbar_cp_reginfo);
+    }
     if (arm_feature(env, ARM_FEATURE_V7MP) &&
         !arm_feature(env, ARM_FEATURE_MPU)) {
         define_arm_cp_regs(cpu, v7mp_cp_reginfo);
Index: qemu-aspeed.git/target-arm/cpu.h
===================================================================
--- qemu-aspeed.git.orig/target-arm/cpu.h
+++ qemu-aspeed.git/target-arm/cpu.h
@@ -1129,6 +1129,7 @@ enum arm_features {
     ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
     ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
     ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
+    ARM_FEATURE_VBAR, /* has cp15 VBAR (ARM1176) */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
Index: qemu-aspeed.git/target-arm/cpu.c
===================================================================
--- qemu-aspeed.git.orig/target-arm/cpu.c
+++ qemu-aspeed.git/target-arm/cpu.c
@@ -584,6 +584,7 @@ static void arm_cpu_realizefn(DeviceStat
         set_feature(env, ARM_FEATURE_LPAE);
     }
     if (arm_feature(env, ARM_FEATURE_V7)) {
+        set_feature(env, ARM_FEATURE_VBAR);
         set_feature(env, ARM_FEATURE_VAPA);
         set_feature(env, ARM_FEATURE_THUMB2);
         set_feature(env, ARM_FEATURE_MPIDR);
@@ -867,6 +868,7 @@ static void arm1176_initfn(Object *obj)
 
     cpu->dtb_compatible = "arm,arm1176";
     set_feature(&cpu->env, ARM_FEATURE_V6K);
+    set_feature(&cpu->env, ARM_FEATURE_VBAR);
     set_feature(&cpu->env, ARM_FEATURE_VFP);
     set_feature(&cpu->env, ARM_FEATURE_VAPA);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);

Re: [Qemu-arm] [PATCH] target-arm: Add VBAR support to ARM1176 CPUs

[Peter Maydell]

On 23 August 2016 at 10:59, Cédric Le Goater <clg@kaod.org> wrote:
> ARM1176 CPUs support the Vector Base Address Register but currently,
> qemu only supports VBAR on ARMv7 CPUs. Fix this by adding a new
> feature ARM_FEATURE_VBAR which is used for ARMv7 and ARM1176 CPUs.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

This looks mostly good; one question below.

> Index: qemu-aspeed.git/target-arm/cpu.h
> ===================================================================
> --- qemu-aspeed.git.orig/target-arm/cpu.h
> +++ qemu-aspeed.git/target-arm/cpu.h
> @@ -1129,6 +1129,7 @@ enum arm_features {
>      ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
>      ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
>      ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
> +    ARM_FEATURE_VBAR, /* has cp15 VBAR (ARM1176) */

you don't need the bit in brackets, I think (it's not complete
anyway, since v7+ also has this).

>  };
>
>  static inline int arm_feature(CPUARMState *env, int feature)
> Index: qemu-aspeed.git/target-arm/cpu.c
> ===================================================================
> --- qemu-aspeed.git.orig/target-arm/cpu.c
> +++ qemu-aspeed.git/target-arm/cpu.c
> @@ -584,6 +584,7 @@ static void arm_cpu_realizefn(DeviceStat
>          set_feature(env, ARM_FEATURE_LPAE);
>      }
>      if (arm_feature(env, ARM_FEATURE_V7)) {
> +        set_feature(env, ARM_FEATURE_VBAR);
>          set_feature(env, ARM_FEATURE_VAPA);
>          set_feature(env, ARM_FEATURE_THUMB2);
>          set_feature(env, ARM_FEATURE_MPIDR);
> @@ -867,6 +868,7 @@ static void arm1176_initfn(Object *obj)
>
>      cpu->dtb_compatible = "arm,arm1176";
>      set_feature(&cpu->env, ARM_FEATURE_V6K);
> +    set_feature(&cpu->env, ARM_FEATURE_VBAR);
>      set_feature(&cpu->env, ARM_FEATURE_VFP);
>      set_feature(&cpu->env, ARM_FEATURE_VAPA);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);

Is it sufficient to set ARM_FEATURE_VBAR in the realizefn
if FEATURE_V7 or FEATURE_EL3? (watch out that realizefn
may change the value of the FEATURE_EL3 bit partway down
so you'd need to do the check there) ?

We implement VBAR in v7-without-EL3 even though architecturally
it should only exist in v7-with-EL3 because we have some
legacy board models which we implement as without-EL3 but
where the guest (Linux) assumes it's running on a with-EL3
CPU in NS mode. I'd rather we stick to the architectural
definition for 1176 if we can rather than expanding the
"things we do which aren't architectural" set if there's
no legacy config that requires it. (If it is necessary
to define it for 1176 always then that's OK, I'd just
prefer not to unless we know we have to.)

We should probably also have a brief comment noting that
we define VBAR always in v7, even though architecturally
it doesn't exist in non-EL3 configs, for the benefit of
legacy board models.

(In v8 VBAR is required whether EL3 is implemented or not.)

thanks
-- PMM

Re: [Qemu-arm] [PATCH] target-arm: Add VBAR support to ARM1176 CPUs

[Cédric Le Goater]

On 09/05/2016 04:39 PM, Peter Maydell wrote:
> On 23 August 2016 at 10:59, Cédric Le Goater <clg@kaod.org> wrote:
>> ARM1176 CPUs support the Vector Base Address Register but currently,
>> qemu only supports VBAR on ARMv7 CPUs. Fix this by adding a new
>> feature ARM_FEATURE_VBAR which is used for ARMv7 and ARM1176 CPUs.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> This looks mostly good; one question below.

Sorry for the late answer. I was drilling down through PPC with 
the initial support of the pnv machine. 

For 2.9, I would like to send a couple of fixes for the Aspeed  
machines, this is one of them and I feel I didn't choose the 
easiest path to enter the ARM world :)

>> Index: qemu-aspeed.git/target-arm/cpu.h
>> ===================================================================
>> --- qemu-aspeed.git.orig/target-arm/cpu.h
>> +++ qemu-aspeed.git/target-arm/cpu.h
>> @@ -1129,6 +1129,7 @@ enum arm_features {
>>      ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions */
>>      ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
>>      ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
>> +    ARM_FEATURE_VBAR, /* has cp15 VBAR (ARM1176) */
> 
> you don't need the bit in brackets, I think (it's not complete
> anyway, since v7+ also has this).

ok.

>>  };
>>
>>  static inline int arm_feature(CPUARMState *env, int feature)
>> Index: qemu-aspeed.git/target-arm/cpu.c
>> ===================================================================
>> --- qemu-aspeed.git.orig/target-arm/cpu.c
>> +++ qemu-aspeed.git/target-arm/cpu.c
>> @@ -584,6 +584,7 @@ static void arm_cpu_realizefn(DeviceStat
>>          set_feature(env, ARM_FEATURE_LPAE);
>>      }
>>      if (arm_feature(env, ARM_FEATURE_V7)) {
>> +        set_feature(env, ARM_FEATURE_VBAR);
>>          set_feature(env, ARM_FEATURE_VAPA);
>>          set_feature(env, ARM_FEATURE_THUMB2);
>>          set_feature(env, ARM_FEATURE_MPIDR);
>> @@ -867,6 +868,7 @@ static void arm1176_initfn(Object *obj)
>>
>>      cpu->dtb_compatible = "arm,arm1176";
>>      set_feature(&cpu->env, ARM_FEATURE_V6K);
>> +    set_feature(&cpu->env, ARM_FEATURE_VBAR);
>>      set_feature(&cpu->env, ARM_FEATURE_VFP);
>>      set_feature(&cpu->env, ARM_FEATURE_VAPA);
>>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
> 
> Is it sufficient to set ARM_FEATURE_VBAR in the realizefn
> if FEATURE_V7 or FEATURE_EL3? (watch out that realizefn
> may change the value of the FEATURE_EL3 bit partway down
> so you'd need to do the check there) ?

It looks like it, yes. 

The addition of the VBAR only depends on ARM_FEATURE_VBAR 
bit which is set for 1176 and V7. I followed the pattern 
used by ARM_FEATURE_V6K. FEATURE_EL3 does not come in play
but may be it should indeed, see below.

> We implement VBAR in v7-without-EL3 even though architecturally
> it should only exist in v7-with-EL3 because we have some
> legacy board models which we implement as without-EL3 but
> where the guest (Linux) assumes it's running on a with-EL3
> CPU in NS mode. I'd rather we stick to the architectural
> definition for 1176 if we can rather than expanding the
> "things we do which aren't architectural" set if there's
> no legacy config that requires it. (If it is necessary
> to define it for 1176 always then that's OK, I'd just
> prefer not to unless we know we have to.)

According to the ARM spec, 1176 is a v6 with a couple of 
extensions, among which v6k and TrustZone. So it has Secure 
and Non-Secure VBAR and MVBAR registers.

Looking at :

  https://en.wikipedia.org/wiki/List_of_ARM_microarchitectures

May be, we should instead introduce a new ARMv6Z architecture 
for the 1176 cpu which would represent a ARMv6 + TrustZone ?


But I am a bit confused with this EL3 feature. 1176 does not 
have EL3 to my understanding (I am beyond my ARM skills there).
So why is it part of the feature list in arm1176_initfn ? 
Is that a way to define the MVBAR and SCR regs ? 

Should we define VBAR under el3_cp_reginfo and not under 
v7_cp_reginfo then ? 

> We should probably also have a brief comment noting that
> we define VBAR always in v7, even though architecturally
> it doesn't exist in non-EL3 configs, for the benefit of
> legacy board models.

ok. I can add that.

Thanks,

C. 

> (In v8 VBAR is required whether EL3 is implemented or not.)
> 
> thanks
> -- PMM
> 

Re: [Qemu-arm] [PATCH] target-arm: Add VBAR support to ARM1176 CPUs

[Peter Maydell]

On 24 November 2016 at 14:29, Cédric Le Goater <clg@kaod.org> wrote:
> On 09/05/2016 04:39 PM, Peter Maydell wrote:
>> We implement VBAR in v7-without-EL3 even though architecturally
>> it should only exist in v7-with-EL3 because we have some
>> legacy board models which we implement as without-EL3 but
>> where the guest (Linux) assumes it's running on a with-EL3
>> CPU in NS mode. I'd rather we stick to the architectural
>> definition for 1176 if we can rather than expanding the
>> "things we do which aren't architectural" set if there's
>> no legacy config that requires it. (If it is necessary
>> to define it for 1176 always then that's OK, I'd just
>> prefer not to unless we know we have to.)
>
> According to the ARM spec, 1176 is a v6 with a couple of
> extensions, among which v6k and TrustZone. So it has Secure
> and Non-Secure VBAR and MVBAR registers.
>
> Looking at :
>
>   https://en.wikipedia.org/wiki/List_of_ARM_microarchitectures
>
> May be, we should instead introduce a new ARMv6Z architecture
> for the 1176 cpu which would represent a ARMv6 + TrustZone ?
>
>
> But I am a bit confused with this EL3 feature. 1176 does not
> have EL3 to my understanding (I am beyond my ARM skills there).

EL3 == TrustZone's Monitor mode level of privilege. In
ARMv8 the privilege level model was tidied up a bit so it's
more cleanly arranged in levels EL0 (user) EL1 (kernel)
EL2 (hypervisor) EL3 (secure monitor), and the old 32-bit
setup can be expressed in terms of that (User being EL0,
the various Sys/Fiq/Irq/etc modes being EL1, Hyp at EL2
and Mon at EL3). For QEMU we implemented TrustZone support
after the v8 spec was published, so we had the opportunity
to do it in a way that fit nicely with 64-bit TZ support.
So ARM_FEATURE_EL3 is our "does this CPU have TrustZone"
flag, in the same way that ARM_FEATURE_EL2 is "does it have
the Virtualization extensions".

We also support 1176-without-TrustZone, which doesn't
exist in real hardware but which we have for backwards
compatibility with code that runs on our 1176 boards
which we implemented before we had TZ support in the
CPU implementation.

What I'm suggesting is that we shoulld provide VBAR
only in the 1176-with-TZ CPUs, not in the 1176-no-TZ
CPUs, unless we must provide it for the latter as a
back-compat requirement.

> So why is it part of the feature list in arm1176_initfn ?
> Is that a way to define the MVBAR and SCR regs ?

The CPU object defaults to TZ-enabled, and then the
board model can disable it. (See for instance the
code in hw/arm/realview.c which sets the has_el3 property
on the CPU object to false.)

thanks
-- PMM

Re: [Qemu-arm] [PATCH] target-arm: Add VBAR support to ARM1176 CPUs

[Cédric Le Goater]

On 11/28/2016 11:40 AM, Peter Maydell wrote:
> On 24 November 2016 at 14:29, Cédric Le Goater <clg@kaod.org> wrote:
>> On 09/05/2016 04:39 PM, Peter Maydell wrote:
>>> We implement VBAR in v7-without-EL3 even though architecturally
>>> it should only exist in v7-with-EL3 because we have some
>>> legacy board models which we implement as without-EL3 but
>>> where the guest (Linux) assumes it's running on a with-EL3
>>> CPU in NS mode. I'd rather we stick to the architectural
>>> definition for 1176 if we can rather than expanding the
>>> "things we do which aren't architectural" set if there's
>>> no legacy config that requires it. (If it is necessary
>>> to define it for 1176 always then that's OK, I'd just
>>> prefer not to unless we know we have to.)
>>
>> According to the ARM spec, 1176 is a v6 with a couple of
>> extensions, among which v6k and TrustZone. So it has Secure
>> and Non-Secure VBAR and MVBAR registers.
>>
>> Looking at :
>>
>>   https://en.wikipedia.org/wiki/List_of_ARM_microarchitectures
>>
>> May be, we should instead introduce a new ARMv6Z architecture
>> for the 1176 cpu which would represent a ARMv6 + TrustZone ?
>>
>>
>> But I am a bit confused with this EL3 feature. 1176 does not
>> have EL3 to my understanding (I am beyond my ARM skills there).
> 
> EL3 == TrustZone's Monitor mode level of privilege. In
> ARMv8 the privilege level model was tidied up a bit so it's
> more cleanly arranged in levels EL0 (user) EL1 (kernel)
> EL2 (hypervisor) EL3 (secure monitor), and the old 32-bit
> setup can be expressed in terms of that (User being EL0,
> the various Sys/Fiq/Irq/etc modes being EL1, Hyp at EL2
> and Mon at EL3). For QEMU we implemented TrustZone support
> after the v8 spec was published, so we had the opportunity
> to do it in a way that fit nicely with 64-bit TZ support.
> So ARM_FEATURE_EL3 is our "does this CPU have TrustZone"
> flag, in the same way that ARM_FEATURE_EL2 is "does it have
> the Virtualization extensions".
>
> We also support 1176-without-TrustZone, which doesn't
> exist in real hardware but which we have for backwards
> compatibility with code that runs on our 1176 boards
> which we implemented before we had TZ support in the
> CPU implementation.

Thanks for the explanation. It is clearer now.

> What I'm suggesting is that we shoulld provide VBAR
> only in the 1176-with-TZ CPUs, not in the 1176-no-TZ
> CPUs, unless we must provide it for the latter as a
> back-compat requirement.

I don't for the back-compat requirement. 

But I need to rework the patch to keep VBAR in any case 
for the v7-without-EL3 CPUs and remove it for the 1176 CPUs 
when 'has_el3' is not set.

Thanks,

C. 

>> So why is it part of the feature list in arm1176_initfn ?
>> Is that a way to define the MVBAR and SCR regs ?
> 
> The CPU object defaults to TZ-enabled, and then the
> board model can disable it. (See for instance the
> code in hw/arm/realview.c which sets the has_el3 property
> on the CPU object to false.)
> 
> thanks
> -- PMM
>