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 03:57:27PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 17:08, Andrew Jones <drjones@redhat.com> wrote:
> > On Thu, Feb 12, 2015 at 04:05:07PM +0100, Andrew Jones wrote:
> >> Now that we have get_S1prot, we can apply it to get_phys_addr_v6
> >> for a minor code cleanup.
> 
> I think this is a bad idea -- better to keep the long
> and short descriptor code paths separate. It's too easy
> to get confused otherwise.

I don't mind keeping them separate, but I disagree with it being
confusing keeping them together :-)

> 
> > 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.

Thanks for the review.

drew