[v4,1/7] target/arm: Improve masking of HCR RES0 bits
diff mbox series

Message ID 20200225180831.26078-2-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Honor more HCR_EL2 traps
Related show

Commit Message

Richard Henderson Feb. 25, 2020, 6:08 p.m. UTC
Don't merely start with v8.0, handle v7VE as well.
Notice writes from aarch32 mode, and the bits that
ought not be settable from there.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Peter Maydell Feb. 28, 2020, 4:22 p.m. UTC | #1
On Tue, 25 Feb 2020 at 18:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Don't merely start with v8.0, handle v7VE as well.
> Notice writes from aarch32 mode, and the bits that
> ought not be settable from there.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 79db169e04..d65160fdb3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5089,8 +5089,13 @@ static const ARMCPRegInfo el3_no_el2_v8_cp_reginfo[] = {
>  static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = env_archcpu(env);
> -    /* Begin with bits defined in base ARMv8.0.  */
> -    uint64_t valid_mask = MAKE_64BIT_MASK(0, 34);
> +    uint64_t valid_mask;
> +
> +    if (arm_feature(env, ARM_FEATURE_V8)) {
> +        valid_mask = MAKE_64BIT_MASK(0, 34);  /* ARMv8.0 */
> +    } else {
> +        valid_mask = MAKE_64BIT_MASK(0, 28);  /* ARMv7VE */
> +    }
>
>      if (arm_feature(env, ARM_FEATURE_EL3)) {
>          valid_mask &= ~HCR_HCD;
> @@ -5114,6 +5119,14 @@ static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>          valid_mask |= HCR_API | HCR_APK;
>      }
>
> +    if (ri->state == ARM_CP_STATE_AA32) {
> +        /*
> +         * Writes from aarch32 mode have more RES0 bits.
> +         * This includes TDZ, RW, E2H, and more.
> +         */
> +        valid_mask &= ~0xff80ff8c90000000ull;
> +    }

Isn't bit HCR2 bit 16 (aka bit 32+16==48 here) also RES0 from AArch32 ?
I suppose it's RES0 from AArch64 too, but as far as what we've
implemented goes so are a bunch of other bits.

I'm not really a fan of the hex-number here either, given we
have HCR_* constants.

thanks
-- PMM
Richard Henderson Feb. 28, 2020, 4:57 p.m. UTC | #2
On 2/28/20 8:22 AM, Peter Maydell wrote:
>> +    if (ri->state == ARM_CP_STATE_AA32) {
>> +        /*
>> +         * Writes from aarch32 mode have more RES0 bits.
>> +         * This includes TDZ, RW, E2H, and more.
>> +         */
>> +        valid_mask &= ~0xff80ff8c90000000ull;
>> +    }
> 
> Isn't bit HCR2 bit 16 (aka bit 32+16==48 here) also RES0 from AArch32 ?

Yes, and it's set in the above.

> I'm not really a fan of the hex-number here either, given we
> have HCR_* constants.

While plenty of those bits have names, many don't.  Shall I simply name all of
the ones that have names, and that differ from the aa64 masking?


r~
Peter Maydell Feb. 28, 2020, 5:34 p.m. UTC | #3
On Fri, 28 Feb 2020 at 16:57, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/28/20 8:22 AM, Peter Maydell wrote:
> >> +    if (ri->state == ARM_CP_STATE_AA32) {
> >> +        /*
> >> +         * Writes from aarch32 mode have more RES0 bits.
> >> +         * This includes TDZ, RW, E2H, and more.
> >> +         */
> >> +        valid_mask &= ~0xff80ff8c90000000ull;
> >> +    }
> >
> > Isn't bit HCR2 bit 16 (aka bit 32+16==48 here) also RES0 from AArch32 ?
>
> Yes, and it's set in the above.

One of us is miscounting, and I don't *think* it's me...

bits 63..0:  ff80ff8c90000000
bits 63..32: ff80ff8c
bits 64..48: ff80

bit 48 looks like it's 0 to me.

> > I'm not really a fan of the hex-number here either, given we
> > have HCR_* constants.
>
> While plenty of those bits have names, many don't.  Shall I simply name all of
> the ones that have names, and that differ from the aa64 masking?

You could refine the valid mask as the & of the bits which we
do want to exist in aarch32, rather than &~ of the reserved bits:

 valid_mask &= TTLBIS | TOCU | TICAB | ...

?

thanks
-- PMM
Richard Henderson Feb. 28, 2020, 6:55 p.m. UTC | #4
On 2/28/20 9:34 AM, Peter Maydell wrote:
> One of us is miscounting, and I don't *think* it's me...
> 
> bits 63..0:  ff80ff8c90000000
> bits 63..32: ff80ff8c
> bits 64..48: ff80
> 
> bit 48 looks like it's 0 to me.

Oops, yes, it's me.

> You could refine the valid mask as the & of the bits which we
> do want to exist in aarch32, rather than &~ of the reserved bits:
> 
>  valid_mask &= TTLBIS | TOCU | TICAB | ...
> 
> ?

Yes, that's a good idea.


r~
Peter Maydell Feb. 28, 2020, 7:03 p.m. UTC | #5
On Fri, 28 Feb 2020 at 18:55, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/28/20 9:34 AM, Peter Maydell wrote:
> > You could refine the valid mask as the & of the bits which we
> > do want to exist in aarch32, rather than &~ of the reserved bits:
> >
> >  valid_mask &= TTLBIS | TOCU | TICAB | ...
> >
> > ?
>
> Yes, that's a good idea.

It occurs to me that we should check what the required
semantics are for the opposite half of the register
if the guest writes to one half of it via hcr_writehigh()
or hcr_writelow() -- is the un-accessed half supposed
to stay exactly as it is, or is it ok for the
RES0-for-aarch32 bits to get squashed in the process?
That would seem at least a bit odd even if it's valid,
so maybe better to do aarch32 RES0 masking in
hcr_writehigh() and hcr_writelow()?

thanks
-- PMM
Richard Henderson Feb. 28, 2020, 10:24 p.m. UTC | #6
On 2/28/20 11:03 AM, Peter Maydell wrote:
> It occurs to me that we should check what the required
> semantics are for the opposite half of the register
> if the guest writes to one half of it via hcr_writehigh()
> or hcr_writelow() -- is the un-accessed half supposed
> to stay exactly as it is, or is it ok for the
> RES0-for-aarch32 bits to get squashed in the process?
> That would seem at least a bit odd even if it's valid,
> so maybe better to do aarch32 RES0 masking in
> hcr_writehigh() and hcr_writelow()?

Hmm.  You're thinking of a situation in which

 1) EL3 invokes EL2 with aa64 which sets HCR_EL2,
 2) EL3 invokes EL2 in aa32 which sets HCR which
    as a side-effect clears some of the bits in HCR2
 3) EL3 invokes EL2 with aa64 again and HCR_EL2
    incorrectly has some high bits clear.

I can't find any language that explicitly says, but "architecturally mapped"
means that the bits backing the storage must be the same.  So while it isn't
legal for aa32 to set HCR2 bit 8 (HCR_EL2.APK), I imagine that it would still
read as written.

So I think you're correct that we shouldn't alter the half B when writing to
half A.

Perhaps I should do some masking for aa32 in arm_hcr_el2_eff.


r~

Patch
diff mbox series

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 79db169e04..d65160fdb3 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5089,8 +5089,13 @@  static const ARMCPRegInfo el3_no_el2_v8_cp_reginfo[] = {
 static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = env_archcpu(env);
-    /* Begin with bits defined in base ARMv8.0.  */
-    uint64_t valid_mask = MAKE_64BIT_MASK(0, 34);
+    uint64_t valid_mask;
+
+    if (arm_feature(env, ARM_FEATURE_V8)) {
+        valid_mask = MAKE_64BIT_MASK(0, 34);  /* ARMv8.0 */
+    } else {
+        valid_mask = MAKE_64BIT_MASK(0, 28);  /* ARMv7VE */
+    }
 
     if (arm_feature(env, ARM_FEATURE_EL3)) {
         valid_mask &= ~HCR_HCD;
@@ -5114,6 +5119,14 @@  static void hcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
         valid_mask |= HCR_API | HCR_APK;
     }
 
+    if (ri->state == ARM_CP_STATE_AA32) {
+        /*
+         * Writes from aarch32 mode have more RES0 bits.
+         * This includes TDZ, RW, E2H, and more.
+         */
+        valid_mask &= ~0xff80ff8c90000000ull;
+    }
+
     /* Clear RES0 bits.  */
     value &= valid_mask;