| Message ID | 1405355225-4623-3-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | Accepted |
| Commit | 675498f8ca744f21a6f48d4d2d0ac4bc8db3bb89 |
| Headers | show |
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,
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.
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,
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.
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;
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(-)