Re: [PATCH v4 7/9] arm/mem_access: Add long-descriptor based gpt

[Julien Grall <julien.grall@arm.com>]

On 23/06/17 15:23, Sergej Proskurin wrote:
> Hi Julien,
>
> [...]
>
>>> +static bool get_ttbr_and_gran_64bit(uint64_t *ttbr, unsigned int *gran,
>>> +                                    register_t tcr, enum active_ttbr
>>> ttbrx)
>>> +{
>>> +    bool disabled;
>>> +
>>> +    if ( ttbrx == TTBR0_ACTIVE )
>>> +    {
>>> +        /* Normalize granule size. */
>>> +        switch ( tcr & TCR_TG0_MASK )
>>> +        {
>>> +        case TCR_TG0_16K:
>>> +            *gran = GRANULE_SIZE_INDEX_16K;
>>> +            break;
>>> +        case TCR_TG0_64K:
>>> +            *gran = GRANULE_SIZE_INDEX_64K;
>>> +            break;
>>> +        default:
>>
>> Please explain why you use 4K by default.
>>
>
> Fair question. According to ARM DDI 0487B.a D7-2487, if the
> TCR_EL1.{TG0|TG1} holds a reserved value, it is implementation defined
> how the value is interpreted by the underlying hardware. My
> implementation in v4 strongly followed the pseudo-code in ARM DDI
> 0487B.a, which suggested to use fall back to 4K by default.
>
> However, agree with you at this point. My next patch series explicitly
> checks if 4K has to be set or not and returns an error on reserved
> values as we cannot be know how the hardware behaves on reserved values.

No. You should not return an error here as you would not be compliant 
with the ARM ARM. I just asked to add a comment explain why you choose 
4K, even if it says "We follow the pseudo-code".

>
> [...]
>
>>> +
>>> +    /*
>>> +     * According to to ARM DDI 0487B.a J1-5927, we return an error if
>>> the found
>>> +     * PTE is invalid or holds a reserved entry (PTE<1:0> == x0)) or
>>> if the PTE
>>> +     * maps a memory block at level 3 (PTE<1:0> == 01).
>>> +     */
>>> +    if ( !lpae_valid(pte) || ((level == 3) && lpae_mapping(pte)) )
>>
>> If you look at the comment on top of lpae_mapping, it should only be
>> used for L0..L2 ptes. So why are you using it on L3 ptes?
>>
>
> Yes, I saw the comment. Yet, I would like to check exactly for this
> mapping. If the mapping would as in the check above, it would be an
> error, which is treated accordingly. In v3, you have suggested to look
> at the the lpae_is_superpage macro, which, in my opinion, is not the
> right construct to use at this point. If you should not like this check,
> I could fall back to my previous implementation:
>
> if ( !p2m_valid(pte) || ((level == 3) && !p2m_table(pte)) )
>     return -EFAULT;

If you look at the ARM ARM (D4.3.1 and D4.3.2 in DDI0487B.a)
	* level 0,1,2 will have bit 1 set for table and cleared for mapping.
	* level 3 will have bit 1 set for mapping

If you use p2m_table to check it the bit is set, then it mislead 
completely the user as this entry is not a table at all.

In any, this is totally wrong to use p2m_table and p2m_mapping here as 
it would not report correctly the value. So please don't use an helper 
that does not make sense here. You should just open-code it or introduce 
a helper (such as lpae_page with appropriate) to properly check the bit.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel