Re: [Qemu-devel] [PATCH v3 3/9] target-arm: Add support for S2 page-table protection bits

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 October 2015 at 17:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>>
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> ---
>>>  target-arm/helper.c | 41 +++++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 37 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 507324f..610f1b5 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -6015,6 +6015,28 @@ 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
>>> + * @s2ap:    The 2-bit stage2 access permissions (S2AP)
>>> + * @xn:      XN (execute-never) bit
>>> + */
>>> +static int get_S2prot(CPUARMState *env, int s2ap, int xn)
>>> +{
>>> +    int prot = 0;
>>> +
>>> +    if (s2ap & 1) {
>>> +        prot |= PAGE_READ;
>>> +    }
>>> +    if (s2ap & 2) {
>>> +        prot |= PAGE_WRITE;
>>> +    }
>>> +    if (!xn) {
>>> +        prot |= PAGE_EXEC;
>>> +    }
>>> +    return prot;
>>> +}
>>> +
>>>  /* Translate section/page access permissions to protection flags
>>>   *
>>>   * @env:     CPUARMState
>>> @@ -6628,9 +6650,15 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>>>           */
>>>          page_size = (1ULL << ((granule_sz * (4 - level)) + 3));
>>>          descaddr |= (address & (page_size - 1));
>>> -        /* Extract attributes from the descriptor and merge with table attrs */
>>> +        /* Extract attributes from the descriptor */
>>>          attrs = extract64(descriptor, 2, 10)
>>>              | (extract64(descriptor, 52, 12) << 10);
>>> +
>>> +        if (mmu_idx == ARMMMUIdx_S2NS) {
>>> +            /* Stage 2 table descriptors do not include any attribute fields */
>>> +            break;
>>> +        }
>>> +        /* Merge in attributes from table descriptors */
>>>          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
>>> @@ -6652,11 +6680,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);
>>
>> OK I've gotten lost in the ARM ARM. Is there an architecture defined
>> format the final attrs we construct from the page tables is meant to
>> conform to? Or is the choice of the final structure arbitrary?
>
> The bit of the ARM ARM you want is D4.3.3 in rev A.g of the v8 ARM ARM,
> which describes all the attribute fields in the various descriptors.
>
> At this point 'attrs' is in the format of the attribute bits from
> a stage 1 Block/Page descriptor, all shifted down to the bottom of
> a word (ie attrs[21:0] is descriptor bits [63:52] + [11:2]). You can
> see us shifting [63:52] and [11:2] into place in the line just below
> the "Extract attributes from the descriptor" line. tableattrs[4:0] is
> bits [63:59] of the stage 1 Table descriptor format.

Right, so I was getting lost between these. Perhaps a handy diagram is
warranted?

        /* Extract attributes from the descriptors. We combine the
         * two sections [58:52] and [11:2] into a contiguous attrs
         * bitfield:
         *
         *O:58   55   54     53   52 / 11   10  9  8 7   6  5   4    2
         *  16   13   12     11   10    9    8  7  6 5   4  3   2    0
         * +-------+-------+-----+---+----+----+----+-----+----+------+
         * | SWUse | (U)XN | PXN | C | nG | AF | SH | AP* | NS | Attr |
         * +-------+-------+-----+---+----+----+----+-----+----+------+
         *
         * AP*, is S2AP for stage 2 translation
         */

While looking I noticed we probably extract more than we need into attrs
considering the tableattrs is snarfed earlier. Maybe we should only do:

        attrs = extract64(descriptor, 2, 10)
            | (extract64(descriptor, 52, 7) << 10);

Or even:

        attrs = extract64(descriptor, 2, 10)
            | (extract64(descriptor, 52, 3) << 10);

> For stage 2 translations (which is what the code Edgar's adding
> is handling) the "stage 2 block and page descriptors" format is
> a bit different (mostly it's missing bits that don't have any
> meaning for stage 2), and stage 2 table descriptors have no
> attribute bits at all.

Hmm yeah my ascii diagram kinda glosses over that :-/

Anyway I can follow what's going on now. I'm just left with a nagging
feeling that the code could be cleaner but I'm not sure how....

>
> thanks
> -- PMM

-- 
Alex Bennée