Re: [Xen-devel] [RFC 18/22] xen/arm: p2m: Introduce p2m_set_entry and __p2m_set_entry

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,

[...]

> +static int p2m_set_entry(struct p2m_domain *p2m,
> +                         gfn_t sgfn,
> +                         unsigned long todo,
> +                         mfn_t smfn,
> +                         p2m_type_t t,
> +                         p2m_access_t a)
> +{
> +    int rc = 0;
> +
> +    while ( todo )
> +    {
> +        unsigned long mask = gfn_x(sgfn) | mfn_x(smfn) | todo;
> +        unsigned long order;
> +
> +        /* Always map 4k by 4k when memaccess is enabled */
> +        if ( unlikely(p2m->mem_access_enabled) )
> +            order = THIRD_ORDER;
> +        if ( !(mask & ((1UL << FIRST_ORDER) - 1)) )
> +            order = FIRST_ORDER;

The 2nd outer if statement does not consider p2m->mem_access_enabled.
That is, even if p2m->mem_access_enabled is set, the 2nd if statement
would set an order, which is potentially not equal to the THIRD_ORDER
(please use the THIRD_ORDER define in the ASSERT -- see ASSERT beyond).

As far as I understand, p2m->mem_access_enabled should prevent mapping
of superpages. With this implementation, however, this would not be the
case. Even worse: the ASSERT in __p2m_set_entry would crash the system,
if p2m->mem_access_enabled was set and order was not equal to the
THIRD_ORDER:

+    /*
+     * The radix-tree can only work on 4KB. This is only used when
+     * memaccess is enabled.
+     */
+    ASSERT(!p2m->mem_access_enabled || page_order == 0);

This can be fixed, by simply unifying both outer if statements:

+        /* Always map 4k by 4k when memaccess is enabled */
+        if ( unlikely(p2m->mem_access_enabled) )
+            order = THIRD_ORDER;
+        else if ( !(mask & ((1UL << FIRST_ORDER) - 1)) )
+            order = FIRST_ORDER;
+        else if ( !(mask & ((1UL << SECOND_ORDER) - 1)) )
+            order = SECOND_ORDER;
+        else
+            order = THIRD_ORDER;


> +        else if ( !(mask & ((1UL << SECOND_ORDER) - 1)) )
> +            order = SECOND_ORDER;
> +        else
> +            order = THIRD_ORDER;
> +
> +        rc = __p2m_set_entry(p2m, sgfn, order, smfn, t, a);
> +        if ( rc )
> +            break;
> +
> +        sgfn = gfn_add(sgfn, (1 << order));
> +        if ( !mfn_eq(smfn, INVALID_MFN) )
> +           smfn = mfn_add(smfn, (1 << order));
> +
> +        todo -= (1 << order);
> +    }
> +
> +    return rc;
> +}
> +#endif
> +
>  /*
>   * Returns true if start_gpaddr..end_gpaddr contains at least one
>   * suitably aligned level_size mappping of maddr.
> 

Best regards,
~Sergej

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