[2/5] target-arm: enable get_rw_prot to take simple AP

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(-)

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