Re: [PATCH v8 02/14] arch/arm: add consistency check to REMOVE p2m changes

[Ian Campbell <ian.campbell@citrix.com>]

On Sun, 2014-05-25 at 16:50 +0100, Julien Grall wrote:
> Hi Arianna,
> 
> On 25/05/14 11:51, Arianna Avanzini wrote:
> > +                    unsigned long mfn = pte.p2m.base;
> > +
> > +                    /*
> > +                     * Ensure that the guest address given as argument to
> > +                     * this function is actually mapped to the specified
> > +                     * machine address. maddr here is the machine address
> > +                     * given to the function, while mfn is the machine
> > +                     * frame number actually mapped to the guest address:
> > +                     * check if the two correspond.
> > +                     */
> > +                    if ( !pte.p2m.valid || maddr != pfn_to_paddr(mfn) )
> > +                    {
> > +                        printk("p2m_remove: nonexistent mapping: "
> > +                               "%"PRIx64" and %"PRIx64"\n",
> > +                               pfn_to_paddr(mfn), maddr);
> > +                        /*
> > +                         * If we have successfully removed other mappings,
> > +                         * overload flush local variable to store if we need
> > +                         * to flush TLBs.
> > +                         */
> > +                        if (count) flush = 1; else flush = 0;
> 
> Hrrm, why do you need this line? Flush is already correctly set 
> previously (see flush |= ... few lines above).

That is:
         flush |= pte.p2m.valid;

But this is within a !pte.p2m.valid if.

Now I'm not quite sure why we need to flush if this mapping is not
present, and I'm pretty certain that setting flush=0 isn't correct,
since if it was previously true nothing we do should cause us to decide
we don't need to flush any more (or if there is it needs describing very
clearly why this is the case).

Ian.