| Message ID | 20200225180831.26078-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Honor more HCR_EL2 traps | expand |
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
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~
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
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~
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
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~
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;
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(-) -- 2.20.1