diff mbox

[Xen-devel,RFC,6/9] xen: arm: handle variable p2m levels in p2m_lookup

Message ID 5a0397cc676556da56a3204c9e056052f5dcb9da.1406728037.git.ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell July 30, 2014, 1:47 p.m. UTC 
This paves the way for boot-time selection of the number of levels to
use in the p2m, which is required to support both 40-bit and 48-bit
systems. For now the starting level remains a compile time constant.

Implemented by turning the linear sequence of lookups into a loop.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/p2m.c |   70 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 25 deletions(-)

Comments

Julien Grall July 31, 2014, 11:14 a.m. UTC | #1 
Hi Ian,

On 07/30/2014 02:47 PM, Ian Campbell wrote:
> This paves the way for boot-time selection of the number of levels to
> use in the p2m, which is required to support both 40-bit and 48-bit
> systems. For now the starting level remains a compile time constant.
> 
> Implemented by turning the linear sequence of lookups into a loop.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/p2m.c |   70 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 225d125..557663f 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -149,45 +149,69 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
>  paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> -    lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
> +    const unsigned int offsets[4] = {
> +        zeroeth_table_offset(paddr),
> +        first_table_offset(paddr),
> +        second_table_offset(paddr),
> +        third_table_offset(paddr)
> +    };
> +    const paddr_t masks[4] = {
> +        ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
> +    };
> +    lpae_t pte, *map;
>      paddr_t maddr = INVALID_PADDR;
>      paddr_t mask;
>      p2m_type_t _t;
> +    int level, root_table;

I would use unsigned int here.

> +
> +    BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
>  
>      /* Allow t to be NULL */
>      t = t ?: &_t;
>  
>      *t = p2m_invalid;
>  
> +    if ( P2M_ROOT_PAGES > 1 )
> +    {
> +        /*
> +         * Concatenated root-level tables. The table number will be
> +         * the offset at the previous level. It is not possible to
> +         * concetenate a level-0 root.

concatenate

> +         */
> +        BUG_ON(P2M_ROOT_LEVEL == 0);

I don't think this check is necessary in a production build (ie
debug=n). If people is porting a new board, then it should be done in
debug mode.

Hence it should be a bit "faster" as this code will be called often.

I would switch this BUG_ON to ASSERT.

> +        root_table = offsets[P2M_ROOT_LEVEL - 1];
> +        if ( root_table >= P2M_ROOT_PAGES )
> +            goto err;
> +    }
> +    else
> +        root_table = 0;
> +
>      spin_lock(&p2m->lock);
>  
> -    first = p2m_map_first(p2m, paddr);
> -    if ( !first )
> -        goto err;
> +    map = __map_domain_page(p2m->root + root_table);
>  
> -    mask = FIRST_MASK;
> -    pte = first[first_table_offset(paddr)];
> -    if ( !p2m_table(pte) )
> -        goto done;
> +    for ( level = P2M_ROOT_LEVEL ; level < 4 ; level++ )
> +    {
> +        mask = masks[level];
>  
> -    mask = SECOND_MASK;
> -    second = map_domain_page(pte.p2m.base);
> -    pte = second[second_table_offset(paddr)];
> -    if ( !p2m_table(pte) )
> -        goto done;
> +        pte = map[offsets[level]];
>  
> -    mask = THIRD_MASK;
> +        if ( level == 3 && !p2m_table(pte) )
> +            /* Invalid, clobber the pte */
> +            pte.bits = 0;
> +        if ( level == 3 || !p2m_table(pte) )
> +            /* Done */
> +            break;
>  
> -    BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
> +        BUG_ON(level == 3);

Rather than checking level == 3, I would add
BUG_ON(level < 4) at the end of loop.
The only drawback to be there is we map one more page.

And same remark as the previous BUG_ON().

Regards,
diff mbox

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 225d125..557663f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -149,45 +149,69 @@  static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
 paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
-    lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
+    const unsigned int offsets[4] = {
+        zeroeth_table_offset(paddr),
+        first_table_offset(paddr),
+        second_table_offset(paddr),
+        third_table_offset(paddr)
+    };
+    const paddr_t masks[4] = {
+        ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
+    };
+    lpae_t pte, *map;
     paddr_t maddr = INVALID_PADDR;
     paddr_t mask;
     p2m_type_t _t;
+    int level, root_table;
+
+    BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
 
     /* Allow t to be NULL */
     t = t ?: &_t;
 
     *t = p2m_invalid;
 
+    if ( P2M_ROOT_PAGES > 1 )
+    {
+        /*
+         * Concatenated root-level tables. The table number will be
+         * the offset at the previous level. It is not possible to
+         * concetenate a level-0 root.
+         */
+        BUG_ON(P2M_ROOT_LEVEL == 0);
+        root_table = offsets[P2M_ROOT_LEVEL - 1];
+        if ( root_table >= P2M_ROOT_PAGES )
+            goto err;
+    }
+    else
+        root_table = 0;
+
     spin_lock(&p2m->lock);
 
-    first = p2m_map_first(p2m, paddr);
-    if ( !first )
-        goto err;
+    map = __map_domain_page(p2m->root + root_table);
 
-    mask = FIRST_MASK;
-    pte = first[first_table_offset(paddr)];
-    if ( !p2m_table(pte) )
-        goto done;
+    for ( level = P2M_ROOT_LEVEL ; level < 4 ; level++ )
+    {
+        mask = masks[level];
 
-    mask = SECOND_MASK;
-    second = map_domain_page(pte.p2m.base);
-    pte = second[second_table_offset(paddr)];
-    if ( !p2m_table(pte) )
-        goto done;
+        pte = map[offsets[level]];
 
-    mask = THIRD_MASK;
+        if ( level == 3 && !p2m_table(pte) )
+            /* Invalid, clobber the pte */
+            pte.bits = 0;
+        if ( level == 3 || !p2m_table(pte) )
+            /* Done */
+            break;
 
-    BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
+        BUG_ON(level == 3);
 
-    third = map_domain_page(pte.p2m.base);
-    pte = third[third_table_offset(paddr)];
+        /* Map for next level */
+        unmap_domain_page(map);
+        map = map_domain_page(pte.p2m.base);
+    }
 
-    /* This bit must be one in the level 3 entry */
-    if ( !p2m_table(pte) )
-        pte.bits = 0;
+    unmap_domain_page(map);
 
-done:
     if ( p2m_valid(pte) )
     {
         ASSERT(pte.p2m.type != p2m_invalid);
@@ -195,10 +219,6 @@  done:
         *t = pte.p2m.type;
     }
 
-    if (third) unmap_domain_page(third);
-    if (second) unmap_domain_page(second);
-    if (first) unmap_domain_page(first);
-
 err:
     spin_unlock(&p2m->lock);