Re: [Qemu-devel] [PATCH 2/5] target-arm: enable get_rw_prot to take simple AP

[Andrew Jones <drjones@redhat.com>]

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