[v3,3/9] target-arm: Add support for S2 page-table protection bits

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(-)

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?

>  
> -    *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))) {

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.

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.

thanks
-- PMM

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

On 14 October 2015 at 13:45, Alex Bennée <alex.bennee@linaro.org> wrote:
> 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);

It seemed neater to grab all of the attribute bits, even the
reserved ones. Otherwise we'll end up needing to change the
code back later if a new attribute we care about is added.

thanks
-- PMM