| Message ID | 1423753507-30542-6-git-send-email-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. Actually, I should point out that this isn't just a cleanup, but also a fix. See below. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > target-arm/helper.c | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 20e5753bd216d..c41305e7e2bdf 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -5064,30 +5064,19 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type, > } > code = 15; > } > - if (domain_prot == 3) { > - *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > - } else { > - bool is_user = regime_is_user(env, mmu_idx); > - > - if (pxn && !is_user) { > - xn = 1; > - } > - if (xn && access_type == 2) > - goto do_fault; > - > + if (regime_sctlr(env, mmu_idx) & SCTLR_AFE) { > /* The simplified model uses AP[0] as an access control bit. */ > - if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE) > - && (ap & 1) == 0) { > + if ((ap & 1) == 0) { > /* Access flag fault. */ > code = (code == 15) ? 6 : 3; > goto do_fault; > } > - *prot = get_rw_prot(env, mmu_idx, is_user, ap, domain_prot); > - *prot |= *prot && !xn ? PAGE_EXEC : 0; > - if (!(*prot & (1 << access_type))) { > - /* Access permission fault. */ > - goto do_fault; > - } > + ap >>= 1; The original code didn't take into account that it may be calling check_ap with a simple AP, AP[2:1]. The code should have always been similar to the above, i.e. if (regime_sctlr(env, mmu_idx) & SCTLR_AFE) { /* The simplified model uses AP[0] as an access control bit. */ if ((ap & 1) == 0) { /* Access flag fault. */ code = (code == 15) ? 6 : 3; goto do_fault; } *prot = <handle simple AP somehow>; } else { *prot = check_ap(env, mmu_idx, ap, domain_prot, access_type); } if (!*prot) { /* Access permission fault. */ goto do_fault; } if (!xn) { *prot |= PAGE_EXEC; } 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. I should update the commit message to point this fix out. Or, actually, I should probably add another patch to the series (3/6), which addresses just this issue, and builds it on patch 2 "...to take simple AP". Peter, please let me know your preference. Thanks, drew > + } > + *prot = get_S1prot(env, mmu_idx, false, ap, domain_prot, 0, xn, pxn); > + if (!(*prot & (1 << access_type))) { > + /* Access permission fault. */ > + goto do_fault; > } > *phys_ptr = phys_addr; > return 0; > -- > 1.9.3 > > >
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. > 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]... -- PMM
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
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. -- PMM
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
diff --git a/target-arm/helper.c b/target-arm/helper.c index 20e5753bd216d..c41305e7e2bdf 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -5064,30 +5064,19 @@ static int get_phys_addr_v6(CPUARMState *env, uint32_t address, int access_type, } code = 15; } - if (domain_prot == 3) { - *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; - } else { - bool is_user = regime_is_user(env, mmu_idx); - - if (pxn && !is_user) { - xn = 1; - } - if (xn && access_type == 2) - goto do_fault; - + if (regime_sctlr(env, mmu_idx) & SCTLR_AFE) { /* The simplified model uses AP[0] as an access control bit. */ - if ((regime_sctlr(env, mmu_idx) & SCTLR_AFE) - && (ap & 1) == 0) { + if ((ap & 1) == 0) { /* Access flag fault. */ code = (code == 15) ? 6 : 3; goto do_fault; } - *prot = get_rw_prot(env, mmu_idx, is_user, ap, domain_prot); - *prot |= *prot && !xn ? PAGE_EXEC : 0; - if (!(*prot & (1 << access_type))) { - /* Access permission fault. */ - goto do_fault; - } + ap >>= 1; + } + *prot = get_S1prot(env, mmu_idx, false, ap, domain_prot, 0, xn, pxn); + if (!(*prot & (1 << access_type))) { + /* Access permission fault. */ + goto do_fault; } *phys_ptr = phys_addr; return 0;
Now that we have get_S1prot, we can apply it to get_phys_addr_v6 for a minor code cleanup. Signed-off-by: Andrew Jones <drjones@redhat.com> --- target-arm/helper.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)