Re: [Xen-devel] [PATCH v2 19/25] arm/altp2m: Add altp2m_propagate_change.

[Julien Grall <julien.grall@xxxxxxx>]

On 06/08/2016 12:26, Sergej Proskurin wrote:
> Hi Julien,
Hello Sergej,

> On 08/04/2016 04:50 PM, Julien Grall wrote:
> > On 01/08/16 18:10, Sergej Proskurin wrote:
> > > +
> > > +        /* Check for a dropped page that may impact this altp2m. */
> > > +        if ( (mfn_eq(smfn, INVALID_MFN) || p2mt == p2m_invalid) &&
> > Why the check to p2mt against p2m_invalid?
> I have encountered p2m entries or rather calls to p2m_apply_change with
> valid MFN's, however, invalid types. That is, a page drop would not be
> recognized if checked only against an invalid MFN.
Because currently REMOVE operation has the MFN valid to be able to check 
whether the wanted mapping has been removed.
However, I don't think this is safe to assume that p2m_invalid will 
always meant "REMOVE". It is actually used in different case.
> > > +             gfn_x(sgfn) >= gfn_x(p2m->lowest_mapped_gfn) &&
> > > +             gfn_x(sgfn) <= gfn_x(p2m->max_mapped_gfn) )
> > > +        {
> > > +            if ( !reset_count++ )
> > > +            {
> > > +                altp2m_reset(p2m);
> > > +                last_reset_idx = i;
> > > +            }
> > > +            else
> > > +            {
> > > +                /* At least 2 altp2m's impacted, so reset
> > > everything. */
> > So if you remove a 4KB page in more than 2 altp2m, you will flush all
> > the p2m. This sounds really more time consuming (you have to free all
> > the intermediate page table) than removing a single 4KB page.
> I agree: The solution has been directly adopted from the x86
> implementation. It needs further design reconsideration. However, at
> this point we would like to finish the overall architecture for ARM
> before we go into further reconstructions.
>
> > > +                for ( i = 0; i < MAX_ALTP2M; i++ )
> > > +                {
> > > +                    if ( i == last_reset_idx ||
> > > +                         d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
> > > +                        continue;
> > > +
> > > +                    p2m = d->arch.altp2m_p2m[i];
> > > +                    altp2m_reset(p2m);
> > > +                }
> > > +                goto out;
> > > +            }
> > > +        }
> > > +        else if ( !mfn_eq(m, INVALID_MFN) )
> > > +            modify_altp2m_range(d, p2m, sgfn, nr, smfn,
> > > +                                mask, p2mt, p2ma);
> > I am a bit concerned about this function. We decided to limit the size
> > of the mapping to avoid long running memory operations (see XSA-158).
> >
> > With this function you multiply up to 10 times the duration of the
> > operation.
> I see your point. However, on an active system, adaptions of the hostp2m
> (while altp2m is active) should be very limited. Another solution would
> be to simply flush the altp2m views on every hostp2m modification. IMO
> this, however, would not really be a huge performance gain due to
> de-allocation and freeing of the altp2m tables. Do you have another idea?
I would need to have a think. However, it is wrong to assume that change 
of hostp2m will be limited when altp2m is active. A guest is free to 
increase/decrease the memory reservation.
You always need to consider the worth case and not the best case (i.e a 
guest behaving nicely).
Regards,

--
Julien Grall

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