On Wed, Sep 23, 2015 at 09:55:05AM -0700, Peter Maydell wrote:
> On 19 September 2015 at 07:15, Edgar E. Iglesias
> <edgar.igles...@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
> > ---
> > target-arm/helper.c | 48 +++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 45 insertions(+), 3 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 33be8c2..6f0ed51 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6008,6 +6008,38 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx
> > mmu_idx, int ap)
> > return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
> > }
> >
> > +/* Translate S2 section/page access permissions to protection flags
> > + *
> > + * @env: CPUARMState
> > + * @ap: The 2-bit simple AP (AP[2:1])
>
> I think this should read "The 2-bit stage2 access permissions (S2AP)".
> The interpretation of the field differs from the simple-AP bits.
> You could helpfully call the argument "s2ap" as well.
>
> > + * @xn: XN (execute-never) bit
> > + */
> > +static int get_S2prot(CPUARMState *env, int ap, int xn)
> > +{
> > + int prot_rw;
> > +
> > + switch (ap) {
> > + default:
> > + case 0:
> > + prot_rw = 0;
> > + break;
> > + case 1:
> > + prot_rw = PAGE_READ | PAGE_EXEC;
> > + break;
> > + case 2:
> > + prot_rw = PAGE_WRITE;
> > + break;
> > + case 3:
> > + prot_rw = PAGE_READ | PAGE_EXEC | PAGE_WRITE;
> > + break;
> > + }
> > +
> > + if (xn) {
> > + prot_rw &= ~PAGE_EXEC;
> > + }
>
> This isn't right -- the XN bit controls executability and the
> S2AP bits don't affect it at all. I think you want:
>
> int prot_rw = 0;
> if (s2ap & 1) {
> prot_rw |= PAGE_READ;
> }
> if (s2ap & 2) {
> prot_rw |= PAGE_WRITE;
> }
> if (!xn) {
> prot_rw |= PAGE_EXEC;
> }
Thanks, this was the stuff I was worried about when we talked about it
last time. I've got the following now which seems to be the same as
you suggest:
static int get_S2prot(CPUARMState *env, int s2ap, int xn)
{
int prot;
prot = s2ap & 1 ? PAGE_READ : 0;
prot |= s2ap & 2 ? PAGE_WRITE : 0;
if (!xn) {
prot |= PAGE_EXEC;
}
return prot;
}
I've also fixed up your other comments on this.
Thanks,
Edgar
>
>
> > + return prot_rw;
> > +}
> > +
> > /* Translate section/page access permissions to protection flags
> > *
> > * @env: CPUARMState
> > @@ -6617,6 +6649,11 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> > target_ulong address,
> > /* Extract attributes from the descriptor and merge with table
> > attrs */
> > attrs = extract64(descriptor, 2, 10)
> > | (extract64(descriptor, 52, 12) << 10);
> > +
> > + if (mmu_idx == ARMMMUIdx_S2NS) {
> > + /* The following extractions do not apply to S2. */
>
> I think it would be clearer to say
> /* Stage 2 table descriptors do not include any attribute fields. */
>
> > + break;
> > + }
>
> and then here put
> /* Merge in attributes from table descriptors */
>
> and delete the now-redundant "and merge with table attrs" part from the
> comment at the start of this hunk.
>
> > attrs |= extract32(tableattrs, 0, 2) << 11; /* XN, PXN */
> > attrs |= extract32(tableattrs, 3, 1) << 5; /* APTable[1] => AP[2]
> > */
> > /* The sense of AP[1] vs APTable[0] is reversed, as APTable[0] == 1
> > @@ -6638,11 +6675,16 @@ static bool get_phys_addr_lpae(CPUARMState *env,
> > target_ulong address,
> > }
> >
> > ap = extract32(attrs, 4, 2);
> > - ns = extract32(attrs, 3, 1);
> > xn = extract32(attrs, 12, 1);
> > - pxn = extract32(attrs, 11, 1);
> >
> > - *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> > + if (mmu_idx == ARMMMUIdx_S2NS) {
> > + ns = true;
> > + *prot = get_S2prot(env, ap, xn);
> > + } else {
> > + ns = extract32(attrs, 3, 1);
> > + pxn = extract32(attrs, 11, 1);
> > + *prot = get_S1prot(env, mmu_idx, va_size == 64, ap, ns, xn, pxn);
> > + }
> >
> > fault_type = permission_fault;
> > if (!(*prot & (1 << access_type))) {
> > --
> > 1.9.1
> >
>
> thanks
> -- PMM
