Re: [Xen-devel] [PATCH v2] xen/arm: p2m: Correctly flush TLB in create_p2m_entries

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-01-14 at 12:41 +0000, Julien Grall wrote:
> On 01/13/2014 05:57 PM, Ian Campbell wrote:
> > On Mon, 2014-01-13 at 17:37 +0000, Julien Grall wrote:
> >> On 01/13/2014 05:10 PM, Ian Campbell wrote:
> >>> Hrm, our TLB flush discipline is horribly confused isn't it...
> >>>
> >>> On Thu, 2014-01-09 at 16:34 +0000, Julien Grall wrote:
> >>>> The p2m is shared between VCPUs for each domain. Currently Xen only flush
> >>>> TLB on the local PCPU. This could result to mismatch between the mapping 
> >>>> in the
> >>>> p2m and TLBs.
> >>>>
> >>>> Flush TLB entries used by this domain on every PCPU. The flush can also 
> >>>> be
> >>>> moved out of the loop because:
> >>>>     - ALLOCATE: only called for dom0 RAM allocation, so the flush is 
> >>>> never called
> >>>
> >>> OK.
> >>>
> >>> An ASSERT(!third[third_table_offset(addr)].p2m.valid) might be
> >>> worthwhile if that is the case.
> >>
> >> Will add it.
> > 
> > Thanks.
> > 
> >>> (I'm not sure why ALLOCATE can't be replaced by allocation followed by
> >>> an INSERT, it's seems very special case)
> >>>
> >>>>     - INSERT: if valid = 1 that would means with have replaced a
> >>>>     page that already belongs to the domain. A VCPU can write on the 
> >>>> wrong page.
> >>>>     This can append for dom0 with the 1:1 mapping because the mapping is 
> >>>> not
> >>>>     removed from the p2m.
> >>>
> >>> "append"? Do you mean "happen"?
> >>
> >> I meant "happen".
> >>
> >>>
> >>> In the non-dom0 1:1 case eventually the page will be freed, I guess by a
> >>> subsequent put_page elsewhere -- do they all contain the correct
> >>> flushing? Or do we just leak?
> >>
> >> As for foreign mapping the INSERT function should be hardened. We don't
> > 
> > Did you mean "handled"?
> 
> I meant both :). Actually we don't have any check in this function as
> for REMOVE case.
> 
> I don't think it's possible to do it for 4.4, we take a reference on the
> mapping every time a new entrie is added in the p2m.

Can we pull the:
                /* TODO: Handle other p2m type */
                    if ( p2m_is_foreign(pte.p2m.type) )
                    {
                        ASSERT(mfn_valid(mfn));
                        put_page(mfn_to_page(mfn));
                    }
out to above the switch and have it be:
                                pte = third[third_table_offset(addr)]
                                flsuh = pte.p2m.valid
                                
                /* TODO: Handle other p2m type */
                                if (pte.p2m.valid &&
                                p2m_is_foreign(pte.p2m.type)
                                {
                                    ASSERT(mfn_valid(mfn));
                                    put_page(mfn_to_page(mfn));
                                }
I think that would be acceptable for 4.4, unless there is some
complication I'm not foreseeing?

Ian.    



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