Message ID | 1423753507-30542-3-git-send-email-drjones@redhat.com |
---|---|
State | New |
Headers | show |
On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote: > Teach get_rw_prot about the simple AP format AP[2:1]. An additional > switch was added, as opposed to converting ap := AP[2:1] to AP[2:0] > with a simple shift - and then modifying cases 0,2,4,6, because the > resulting code is easier to read with the switch. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target-arm/helper.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 610f305c4d661..b63ec7b7979f9 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) > static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, > int ap, int domain_prot) > { > + bool simple_ap = regime_using_lpae_format(env, mmu_idx) > + || (regime_sctlr(env, mmu_idx) & SCTLR_AFE); We should check arm_feature(env, ARM_FEATURE_V6K) && (SCTLR.AFE is set); that bit isn't defined til v6K. > + bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx); Given that the lpae code path is totally separate (and not even calling this function yet), can't you just have it pass in a zero domain_prot ? Or have the callers do the domain protection check themselves... > bool is_user = regime_is_user(env, mmu_idx); > > - if (domain_prot == 3) { > + if (domain_prot_valid && domain_prot == 3) { > return PAGE_READ | PAGE_WRITE; > } > > + /* ap is AP[2:1] */ > + if (simple_ap) { > + switch (ap) { > + case 0: > + return is_user ? 0 : PAGE_READ | PAGE_WRITE; > + case 1: > + return PAGE_READ | PAGE_WRITE; > + case 2: > + return is_user ? 0 : PAGE_READ; > + case 3: > + return PAGE_READ; > + default: > + g_assert_not_reached(); > + } > + } I'm confused. Even if we're using the simple-permissions model, the ap parameter is still AP[2:0]. Shouldn't this switch be for cases 0, 2, 4, 6 ? > + > + /* ap is AP[2:0] */ > switch (ap) { > case 0: > if (arm_feature(env, ARM_FEATURE_V7)) { > -- > 1.9.3 > -- PMM
On Tue, Mar 10, 2015 at 03:22:55PM +0000, Peter Maydell wrote: > On 12 February 2015 at 15:05, Andrew Jones <drjones@redhat.com> wrote: > > Teach get_rw_prot about the simple AP format AP[2:1]. An additional > > switch was added, as opposed to converting ap := AP[2:1] to AP[2:0] > > with a simple shift - and then modifying cases 0,2,4,6, because the > > resulting code is easier to read with the switch. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > target-arm/helper.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 610f305c4d661..b63ec7b7979f9 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) > > static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, > > int ap, int domain_prot) > > { > > + bool simple_ap = regime_using_lpae_format(env, mmu_idx) > > + || (regime_sctlr(env, mmu_idx) & SCTLR_AFE); > > We should check arm_feature(env, ARM_FEATURE_V6K) && (SCTLR.AFE is set); > that bit isn't defined til v6K. Indeed. Will send v2 for that. > > > + bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx); > > Given that the lpae code path is totally separate (and not even > calling this function yet), can't you just have it pass in a > zero domain_prot ? Or have the callers do the domain protection > check themselves... domain_prot=0 is a valid access permission (no access), so I didn't want to overload the meaning with 'not used'. I can move the check to the callers that need it though. It would actually be nice to remove the need for a 0 place holder from the other callers. > > > bool is_user = regime_is_user(env, mmu_idx); > > > > - if (domain_prot == 3) { > > + if (domain_prot_valid && domain_prot == 3) { > > return PAGE_READ | PAGE_WRITE; > > } > > > > + /* ap is AP[2:1] */ > > + if (simple_ap) { > > + switch (ap) { > > + case 0: > > + return is_user ? 0 : PAGE_READ | PAGE_WRITE; > > + case 1: > > + return PAGE_READ | PAGE_WRITE; > > + case 2: > > + return is_user ? 0 : PAGE_READ; > > + case 3: > > + return PAGE_READ; > > + default: > > + g_assert_not_reached(); > > + } > > + } > > I'm confused. Even if we're using the simple-permissions > model, the ap parameter is still AP[2:0]. Shouldn't this > switch be for cases 0, 2, 4, 6 ? Depends on how we choose to implement the callers. Currently I only require the caller to send in 2 bits for the simple model. If we want to require them to send in 3, then we'll need to shift a zero in for the lpae caller, rather than shift a zero out for the v6 caller. > > > + > > + /* ap is AP[2:0] */ > > switch (ap) { > > case 0: > > if (arm_feature(env, ARM_FEATURE_V7)) { > > -- > > 1.9.3 > > > > -- PMM
On 10 March 2015 at 16:32, Andrew Jones <drjones@redhat.com> wrote: > On Tue, Mar 10, 2015 at 03:22:55PM +0000, Peter Maydell wrote: >> I'm confused. Even if we're using the simple-permissions >> model, the ap parameter is still AP[2:0]. Shouldn't this >> switch be for cases 0, 2, 4, 6 ? > > Depends on how we choose to implement the callers. Currently > I only require the caller to send in 2 bits for the simple > model. If we want to require them to send in 3, then we'll > need to shift a zero in for the lpae caller, rather than > shift a zero out for the v6 caller. You have to have the callers just pass in AP[2:0], unless you want them to have to duplicate the "are we using the simple permissions model?" condition to figure out whether to shift the argument around, which doesn't seem very sensible. -- PMM
On Tue, Mar 10, 2015 at 04:41:45PM +0000, Peter Maydell wrote: > On 10 March 2015 at 16:32, Andrew Jones <drjones@redhat.com> wrote: > > On Tue, Mar 10, 2015 at 03:22:55PM +0000, Peter Maydell wrote: > >> I'm confused. Even if we're using the simple-permissions > >> model, the ap parameter is still AP[2:0]. Shouldn't this > >> switch be for cases 0, 2, 4, 6 ? > > > > Depends on how we choose to implement the callers. Currently > > I only require the caller to send in 2 bits for the simple > > model. If we want to require them to send in 3, then we'll > > need to shift a zero in for the lpae caller, rather than > > shift a zero out for the v6 caller. > > You have to have the callers just pass in AP[2:0], unless > you want them to have to duplicate the "are we using the > simple permissions model?" condition to figure out whether > to shift the argument around, which doesn't seem very > sensible. > The v6 caller is the only one that could be either-or, and it already checked regime_sctlr(env, mmu_idx) & SCTLR_AFE, as it wanted to see if it cared about the access flag anyway. I suspect any new callers that could be either-or would do the same. drew
diff --git a/target-arm/helper.c b/target-arm/helper.c index 610f305c4d661..b63ec7b7979f9 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -4698,12 +4698,32 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap, int domain_prot) { + bool simple_ap = regime_using_lpae_format(env, mmu_idx) + || (regime_sctlr(env, mmu_idx) & SCTLR_AFE); + bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx); bool is_user = regime_is_user(env, mmu_idx); - if (domain_prot == 3) { + if (domain_prot_valid && domain_prot == 3) { return PAGE_READ | PAGE_WRITE; } + /* ap is AP[2:1] */ + if (simple_ap) { + switch (ap) { + case 0: + return is_user ? 0 : PAGE_READ | PAGE_WRITE; + case 1: + return PAGE_READ | PAGE_WRITE; + case 2: + return is_user ? 0 : PAGE_READ; + case 3: + return PAGE_READ; + default: + g_assert_not_reached(); + } + } + + /* ap is AP[2:0] */ switch (ap) { case 0: if (arm_feature(env, ARM_FEATURE_V7)) {
Teach get_rw_prot about the simple AP format AP[2:1]. An additional switch was added, as opposed to converting ap := AP[2:1] to AP[2:0] with a simple shift - and then modifying cases 0,2,4,6, because the resulting code is easier to read with the switch. Signed-off-by: Andrew Jones <drjones@redhat.com> --- target-arm/helper.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)