Re: [Xen-devel] [PATCH v1 2/8]: PVH mmu changes

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 24 Sep 2012, Ian Campbell wrote:
> On Mon, 2012-09-24 at 12:55 +0100, Stefano Stabellini wrote:
> > There are few code style issues on this patch, I suggest you run it
> > through scripts/checkpatch.pl, it should be able to catch all these
> > errors.
> 
> It would also be nice to starting having some changelogs entries for
> these patches for the next posting. There's a lot of complex stuff going
> on here.
> 
> > > @@ -2371,3 +2518,26 @@ out:
> > >   return err;
> > >  }
> > >  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> > > +
> > > +/* Returns: Number of pages unmapped */
> > > +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> > > +                        struct xen_pvh_pfn_info *pvhp)
> > > +{
> > > + int count = 0;
> > > +
> > > + if (!pvhp || !xen_feature(XENFEAT_auto_translated_physmap))
> > > +         return 0;
> > > +
> > > + while (pvhp->pi_next_todo--) {
> > > +         unsigned long pfn;
> > > +
> > > +         /* the mmu has already cleaned up the process mmu resources at
> > > +          * this point (lookup_address will return NULL). */
> > > +         pfn = page_to_pfn(pvhp->pi_paga[pvhp->pi_next_todo]);
> > > +         pvh_rem_xen_p2m(pfn, 1);
> > > +         count++;
> > > + }
> > > + flush_tlb_all();
> > > + return count;
> > 
> > Who is going to remove the corresponding mapping from the vma?
> > Also we might be able to replace the flush_tlb_all with a
> > flush_tlb_range.
> 
> I'm not convinced that a guest level TLB flush is either necessary or
> sufficient here. What we are doing is removing entries from the P2M
> which means that we need to do the appropriate HAP flush in the
> hypervisor, which must necessarily invalidate any stage 1 mappings which
> this flush might also touch (i.e. the HAP flush must be a super set of
> this flush).
> 
> Without the HAP flush in the hypervisor you risk guests being able to
> see old p2m mappings via the TLB entries which is a security issue
> AFAICT.

Yes, you are right, we need a flush in the hypervisor to flush the EPT.
It could probably live in the implementation of  XENMEM_add_to_physmap.

This one should be just for the vma mappings, so in the case of
xen_unmap_domain_mfn_range is unnecessary (given that it is
not removing the vma mappings).

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