[PATCH 0/3] xen: arm: various p2m handling fixups

[PATCH 0/3] xen: arm: various p2m handling fixups

[Ian Campbell]

Three pretty much independent fixes.

Some of these were introduced by my recent superpages series, others may
or may not have been ;-).

Ian.

[PATCH 1/3] xen: arm: avoid reusing incorrect mappings when walking the p2m.

[Ian Campbell]

If we handle a change in a superpage then we need to invalidate any cached
mappings further down, otherwise we risk using them because their offset might
match but be based on a different offset further up the table.

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

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c0c011a..917baa3 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -719,6 +719,8 @@ static int apply_p2m_changes(struct domain *d,
                 goto out;
             }
             cur_first_page = p2m_first_level_index(addr);
+            /* Any mapping further down is now invalid */
+            cur_second_offset = ~0;
         }
 
         /* We only use a 3 level p2m at the moment, so no level 0,
@@ -741,6 +743,8 @@ static int apply_p2m_changes(struct domain *d,
             if (second) unmap_domain_page(second);
             second = map_domain_page(first[first_table_offset(addr)].p2m.base);
             cur_first_offset = first_table_offset(addr);
+            /* Any mapping further down is now invalid */
+            cur_second_offset = ~0;
         }
         /* else: second already valid */
 
-- 
1.7.10.4

[PATCH 2/3] xen: arm: when ALLOCATING p2m entries maddr is meaningless

[Ian Campbell]

The maddr differs on each loop depending on the pages which happen to get
allocated.

There is already an assertion that maddr == 0.

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

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 917baa3..a10cbaf 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -501,7 +501,6 @@ static int apply_one_level(struct domain *d,
                 p2m->stats.mappings[level]++;
 
                 *addr += level_size;
-                *maddr += level_size;
 
                 return P2M_ONE_PROGRESS;
             }
-- 
1.7.10.4

[PATCH 3/3] xen: arm: correctly handle removing a subset of a superpage mapping.

[Ian Campbell]

This can be exercised for example via ballooning which will remove 4K
regions fromanywhere in the address space.

Reported-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/p2m.c |   29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a10cbaf..68a19bd 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -604,8 +604,33 @@ static int apply_one_level(struct domain *d,
             return P2M_ONE_PROGRESS_NOP;
         }
 
-        if ( level < 3 && p2m_table(orig_pte) )
-            return P2M_ONE_DESCEND;
+        if ( level < 3 )
+        {
+            if ( p2m_table(orig_pte) )
+                return P2M_ONE_DESCEND;
+
+            if ( op == REMOVE &&
+                 !is_mapping_aligned(*addr, end_gpaddr,
+                                     0, /* maddr doesn't matter for remove */
+                                     level_size) )
+            {
+                /*
+                 * Removing a mapping from the middle of a superpage. Shatter
+                 * and descend.
+                 */
+                *flush = true;
+                rc = p2m_create_table(d, entry,
+                                      level_shift - PAGE_SHIFT, flush_cache);
+                if ( rc < 0 )
+                    return rc;
+
+                p2m->stats.shattered[level]++;
+                p2m->stats.mappings[level]--;
+                p2m->stats.mappings[level+1] += LPAE_ENTRIES;
+
+                return P2M_ONE_DESCEND;
+            }
+        }
 
         *flush = true;
 
-- 
1.7.10.4

Re: [PATCH 1/3] xen: arm: avoid reusing incorrect mappings when walking the p2m.

[Julien Grall]

Hi Ian,

On 07/14/2014 05:27 PM, Ian Campbell wrote:
> If we handle a change in a superpage then we need to invalidate any cached
> mappings further down, otherwise we risk using them because their offset might
> match but be based on a different offset further up the table.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/p2m.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index c0c011a..917baa3 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -719,6 +719,8 @@ static int apply_p2m_changes(struct domain *d,
>                  goto out;
>              }
>              cur_first_page = p2m_first_level_index(addr);
> +            /* Any mapping further down is now invalid */
> +            cur_second_offset = ~0;

I think you miss to invalid cur_first_offset.

Also, I think we have the same issue on Xen 4.4.

Regards,

-- 
Julien Grall

Re: [PATCH 2/3] xen: arm: when ALLOCATING p2m entries maddr is meaningless

[Julien Grall]

Hi Ian,

On 07/14/2014 05:27 PM, Ian Campbell wrote:
> The maddr differs on each loop depending on the pages which happen to get
> allocated.
> 
> There is already an assertion that maddr == 0.

I think we hit the assert we don't use 1:1 mapping for DOM0.


> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
>  xen/arch/arm/p2m.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 917baa3..a10cbaf 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -501,7 +501,6 @@ static int apply_one_level(struct domain *d,
>                  p2m->stats.mappings[level]++;
>  
>                  *addr += level_size;
> -                *maddr += level_size;
>  
>                  return P2M_ONE_PROGRESS;
>              }
> 

Regards,

-- 
Julien Grall

Re: [PATCH 3/3] xen: arm: correctly handle removing a subset of a superpage mapping.

[Julien Grall]

Hi Ian,

On 07/14/2014 05:27 PM, Ian Campbell wrote:
> This can be exercised for example via ballooning which will remove 4K
> regions fromanywhere in the address space.

Missing space between "from" and "anywhere"

> 
> Reported-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/p2m.c |   29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index a10cbaf..68a19bd 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -604,8 +604,33 @@ static int apply_one_level(struct domain *d,
>              return P2M_ONE_PROGRESS_NOP;
>          }
>  
> -        if ( level < 3 && p2m_table(orig_pte) )
> -            return P2M_ONE_DESCEND;
> +        if ( level < 3 )
> +        {
> +            if ( p2m_table(orig_pte) )
> +                return P2M_ONE_DESCEND;
> +
> +            if ( op == REMOVE &&
> +                 !is_mapping_aligned(*addr, end_gpaddr,
> +                                     0, /* maddr doesn't matter for remove */
> +                                     level_size) )
> +            {
> +                /*
> +                 * Removing a mapping from the middle of a superpage. Shatter
> +                 * and descend.
> +                 */
> +                *flush = true;
> +                rc = p2m_create_table(d, entry,
> +                                      level_shift - PAGE_SHIFT, flush_cache);
> +                if ( rc < 0 )
> +                    return rc;
> +
> +                p2m->stats.shattered[level]++;
> +                p2m->stats.mappings[level]--;
> +                p2m->stats.mappings[level+1] += LPAE_ENTRIES;
> +
> +                return P2M_ONE_DESCEND;
> +            }
> +        }

I would put the code below (which is only for level3) in the else part.
This will also remove now useless "if ( level == 3 )".

>          *flush = true;
>  
> 

Regards,

-- 
Julien Grall

Re: [PATCH 1/3] xen: arm: avoid reusing incorrect mappings when walking the p2m.

[Ian Campbell]

On Mon, 2014-07-14 at 19:17 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 07/14/2014 05:27 PM, Ian Campbell wrote:
> > If we handle a change in a superpage then we need to invalidate any cached
> > mappings further down, otherwise we risk using them because their offset might
> > match but be based on a different offset further up the table.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/p2m.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index c0c011a..917baa3 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -719,6 +719,8 @@ static int apply_p2m_changes(struct domain *d,
> >                  goto out;
> >              }
> >              cur_first_page = p2m_first_level_index(addr);
> > +            /* Any mapping further down is now invalid */
> > +            cur_second_offset = ~0;
> 
> I think you miss to invalid cur_first_offset.

I think you are right.

> Also, I think we have the same issue on Xen 4.4.

I'll add it to my list to check. I suspect the fix will need
reimplementing there rather than backporting.

Ian.

Re: [PATCH 3/3] xen: arm: correctly handle removing a subset of a superpage mapping.

[Ian Campbell]

On Mon, 2014-07-14 at 19:34 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 07/14/2014 05:27 PM, Ian Campbell wrote:
> > This can be exercised for example via ballooning which will remove 4K
> > regions fromanywhere in the address space.
> 
> Missing space between "from" and "anywhere"
> 
> > 
> > Reported-by: Julien Grall <julien.grall@linaro.org>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/p2m.c |   29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index a10cbaf..68a19bd 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -604,8 +604,33 @@ static int apply_one_level(struct domain *d,
> >              return P2M_ONE_PROGRESS_NOP;
> >          }
> >  
> > -        if ( level < 3 && p2m_table(orig_pte) )
> > -            return P2M_ONE_DESCEND;
> > +        if ( level < 3 )
> > +        {
> > +            if ( p2m_table(orig_pte) )
> > +                return P2M_ONE_DESCEND;
> > +
> > +            if ( op == REMOVE &&
> > +                 !is_mapping_aligned(*addr, end_gpaddr,
> > +                                     0, /* maddr doesn't matter for remove */
> > +                                     level_size) )
> > +            {
> > +                /*
> > +                 * Removing a mapping from the middle of a superpage. Shatter
> > +                 * and descend.
> > +                 */
> > +                *flush = true;
> > +                rc = p2m_create_table(d, entry,
> > +                                      level_shift - PAGE_SHIFT, flush_cache);
> > +                if ( rc < 0 )
> > +                    return rc;
> > +
> > +                p2m->stats.shattered[level]++;
> > +                p2m->stats.mappings[level]--;
> > +                p2m->stats.mappings[level+1] += LPAE_ENTRIES;
> > +
> > +                return P2M_ONE_DESCEND;
> > +            }
> > +        }
> 
> I would put the code below (which is only for level3) in the else part.
> This will also remove now useless "if ( level == 3 )".

This would break the relinquish case, I think.

Although thinking about it perhaps relinquish should behave like remove
and shatter. My original thinking was that we only ever relinquish the
entire address space so we shouldn't really be hitting that case.

Ian.

Re: [PATCH 3/3] xen: arm: correctly handle removing a subset of a superpage mapping.

[Julien Grall]

On 15/07/14 10:38, Ian Campbell wrote:
> On Mon, 2014-07-14 at 19:34 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 07/14/2014 05:27 PM, Ian Campbell wrote:
>>> This can be exercised for example via ballooning which will remove 4K
>>> regions fromanywhere in the address space.
>>
>> Missing space between "from" and "anywhere"
>>
>>>
>>> Reported-by: Julien Grall <julien.grall@linaro.org>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>   xen/arch/arm/p2m.c |   29 +++++++++++++++++++++++++++--
>>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index a10cbaf..68a19bd 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -604,8 +604,33 @@ static int apply_one_level(struct domain *d,
>>>               return P2M_ONE_PROGRESS_NOP;
>>>           }
>>>
>>> -        if ( level < 3 && p2m_table(orig_pte) )
>>> -            return P2M_ONE_DESCEND;
>>> +        if ( level < 3 )
>>> +        {
>>> +            if ( p2m_table(orig_pte) )
>>> +                return P2M_ONE_DESCEND;
>>> +
>>> +            if ( op == REMOVE &&
>>> +                 !is_mapping_aligned(*addr, end_gpaddr,
>>> +                                     0, /* maddr doesn't matter for remove */
>>> +                                     level_size) )
>>> +            {
>>> +                /*
>>> +                 * Removing a mapping from the middle of a superpage. Shatter
>>> +                 * and descend.
>>> +                 */
>>> +                *flush = true;
>>> +                rc = p2m_create_table(d, entry,
>>> +                                      level_shift - PAGE_SHIFT, flush_cache);
>>> +                if ( rc < 0 )
>>> +                    return rc;
>>> +
>>> +                p2m->stats.shattered[level]++;
>>> +                p2m->stats.mappings[level]--;
>>> +                p2m->stats.mappings[level+1] += LPAE_ENTRIES;
>>> +
>>> +                return P2M_ONE_DESCEND;
>>> +            }
>>> +        }
>>
>> I would put the code below (which is only for level3) in the else part.
>> This will also remove now useless "if ( level == 3 )".
>
> This would break the relinquish case, I think.
>
> Although thinking about it perhaps relinquish should behave like remove
> and shatter. My original thinking was that we only ever relinquish the
> entire address space so we shouldn't really be hitting that case.

Oh right, I though there was a return on every part of this if. Sorry 
for the noise.

With the minor change in the commit message:

Acked-by: Julien Grall <julien.grall@linaro.org>


Regards,

-- 
Julien Grall

Re: [PATCH 3/3] xen: arm: correctly handle removing a subset of a superpage mapping.

[Ian Campbell]

On Tue, 2014-07-15 at 13:13 +0100, Julien Grall wrote:
> 
> On 15/07/14 10:38, Ian Campbell wrote:
> With the minor change in the commit message:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

Thanks, applied.

Ian.

Re: [PATCH 2/3] xen: arm: when ALLOCATING p2m entries maddr is meaningless

[Ian Campbell]

On Mon, 2014-07-14 at 19:29 +0100, Julien Grall wrote:
> Acked-by: Julien Grall <julien.grall@linaro.org>

Applied, thanks.