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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx>


Regards,

--
Julien Grall

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