[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.