Peter Maydell <peter.mayd...@linaro.org> writes:
> On 7 October 2015 at 17:19, Alex Bennée <alex.ben...@linaro.org> wrote:
>>
>> Edgar E. Iglesias <edgar.igles...@gmail.com> writes:
>>
>>> From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com>
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.igles...@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
