Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control

[Andrew Jones <drjones@redhat.com>]

On Tue, Mar 10, 2015 at 05:14:21PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 17:02, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Mar 10, 2015 at 04:55:53PM +0000, Peter Maydell wrote:
> >> On 10 March 2015 at 16:48, Andrew Jones <drjones@redhat.com> wrote:
> >> > On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote:
> >>
> >> >> For instance, you're missing a shift here on the ap bits, because
> >> >> get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].
> >> >
> >> > Don't need the shift because get_rw_prot supports the 2-bit format.
> >>
> >> No it doesn't...
> >
> > Yes it does :-) That's the support patch 2/5 adds.
> 
> No, because patch 2 doesn't do anything in the callers to
> make them pass only bits [2:1]. So after patch 2 get_rw_prot
> still requires bits [2:0]. Except it's broken, because the
> function itself assumes it gets bits [2:1].

You've lost me. Patch 2 adds support for 2-bit ap, but
doesn't remove support for 3-bit. There are not callers
expecting it to support the simple model as 2 or 3 bits
at that time, except v6, but that was broken already, and
we fix it in patch 5 (and we add the ap shift there too).
IOW, how can preparing a function for new callers, while
still supporting the old callers, be 'broken'?

> 
> Having thought a bit more about it, I think we should
> just have two totally separate functions for the old
> style and simplified permission checks, because we can
> always call the correct one (always old for v5, either
> one for v6 since we already have the SCTLR.AFE check,
> and the simplified one for the lpae code path).

I like that.

Thanks,
drew