Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6

[Andrew Jones <drjones@redhat.com>]

On Tue, Mar 10, 2015 at 05:03:48PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 16:54, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Mar 10, 2015 at 03:57:27PM +0000, Peter Maydell wrote:
> >> On 12 February 2015 at 17:08, Andrew Jones <drjones@redhat.com> wrote:
> >> > Actually, I should point out that this isn't just a cleanup, but
> >> > also a fix. See below.
> >>
> >> > The original code didn't take into account that it may be calling check_ap
> >> > with a simple AP, AP[2:1].
> >>
> >> No, because check_ap() always takes AP[2:0]...
> >
> > No, it's really wrong. It's not the 2 vs. 3 bit issue that's the
> > problem, it's the cases. You snipped most of my reply to myself.
> > This part is pertinent
> >
> >> As a simple AP wouldn't be properly translated to protection flags with
> >> check_ap (except for case 6), then I think this should have caused some
> >> problems. Maybe this path just hasn't been tested? I don't see CR_AFE
> >> getting used by Linux, so possibly not.
> >
> > So, yes, a simple (3-bit) ap would be handled by the 8-case switch with
> > cases 0, 2, 4, and 6, but only case 6 would give the correct result.
> 
> Well, we didn't correctly support the simple permission model at all
> before your patches. But the point is that check_ap() always takes
> AP[2:0] regardless.
> 
> Hmm, or you could have check_simple_ap() be totally separate
> from check_ap(), and call it from inside the check in the v6
> code path for AFE that we already have.

Agreed. Creating a check_simple_ap function for the new switch added
by 2/5 would look better.

drew