[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_ad
From: |
Andrew Jones |
Subject: |
Re: [Qemu-devel] [PATCH 5/5] target-arm: apply get_S1prot to get_phys_addr_v6 |
Date: |
Tue, 10 Mar 2015 17:54:11 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, Mar 10, 2015 at 03:57:27PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 17:08, Andrew Jones <address@hidden> 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