Re: [Xen-devel] [v3][PATCH 03/16] xen/vtd: create RMRR mapping

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/6/12 13:59, Tian, Kevin wrote:
> > From: Chen, Tiejun
> > Sent: Friday, June 12, 2015 1:58 PM
> >
> > On 2015/6/12 10:43, Chen, Tiejun wrote:
> > > On 2015/6/11 22:07, Tim Deegan wrote:
> > > > At 17:31 +0800 on 11 Jun (1434043916), Chen, Tiejun wrote:
> > > > > > >         while ( base_pfn < end_pfn )
> > > > > > >         {
> > > > > > > -        int err = intel_iommu_map_page(d, base_pfn, base_pfn,
> > > > > > > -
> > > > > > > IOMMUF_readable|IOMMUF_writable);
> > > > > > > +        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw);
> > > > > > >
> > > > > > >             if ( err )
> > > > > > >                 return err;
> > > > > > Tim has another comment to replace earlier unmap with
> > > > > Yes, I knew this.
> > > > >
> > > > > > guest_physmap_remove_page() which will call iommu
> > > > > > unmap internally. Please include this change too.
> > > > > But,
> > > > >
> > > > > guest_physmap_remove_page()
> > > > >        |
> > > > >        + p2m_remove_page()
> > > > >      |
> > > > >      + iommu_unmap_page()
> > > > >      |
> > > > >      + p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), xxx)
> > > > >
> > > > > I think this already remove these pages both on ept/vt-d sides, right?
> > > > Yes; this is about this code further up in the same function:
> > > >
> > > >              while ( base_pfn < end_pfn )
> > > >              {
> > > >                  if ( intel_iommu_unmap_page(d, base_pfn) )
> > > >                      ret = -ENXIO;
> > > >                  base_pfn++;
> > > >              }
> > > >
> > > > which ought to be calling guest_physmap_remove_page() or similar, to
> > > > make sure that both iommu and EPT mappings get removed.
> > > I still just think current implementation might be fine at this point.
> > >
> > > We have two scenarios here, the case of shared ept and the case of
> > > non-shared ept. But no matter what case we're tracking, shouldn't
> > > guest_physmap_remove_page() always call p2m->set_entry() to clear *all*
> > > *valid* mfn which is owned by a given VM? And p2m->set_entry() also
> > > calls iommu_unmap_page() internally. So nothing special should further
> > > consider.
> > >
> > > If I'm wrong or misunderstanding, please correct me :)
> > Sorry for my misunderstanding to this. Right now Kevin help me
> > understand what you mean. Sounds like we should address something
> > specific to unmap rmrr here.
> >
> > So I will do this as follows:
> >
> > #1. Provide a clear helper
> >
> > +int clear_identity_p2m_entry(struct domain *d, unsigned long gfn,
> > +                             unsigned int page_order)
> > +{
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    int ret;
> > +    gfn_lock(p2m, gfn, page_order);
> > +    ret = p2m_remove_page(p2m, gfn, gfn, page_order);
> > +    gfn_unlock(p2m, gfn, page_order);
> > +    return ret;
> > +}
> > +
> >
> > #2. Call such a helper
> >
> > @@ -1840,7 +1840,7 @@ static int rmrr_identity_mapping(struct domain *d,
> > bool_t map,
> >
> >                while ( base_pfn < end_pfn )
> >                {
> > -                if ( intel_iommu_unmap_page(d, base_pfn) )
> > +                if ( clear_identity_p2m_entry(d, base_pfn, 0) )
> >                        ret = -ENXIO;
> >                    base_pfn++;
> >                }
> > Is this right?
> >
> > Thanks
> > Tiejun
> could you explain why existing guest_physmap_remove_page can't
> serve the purpose so you need invent a new identity mapping
> specific one? For unmapping suppose it should be common regardless
> of whether it's identity-mapped or not. :-)
I have some concerns here:

#1. guest_physmap_remove_page() is a void function without a returning 
value, so you still need a little change.
#2. guest_physmap_remove_page() doesn't make readable in such a code 
context;
rmrr_identity_mapping()
{
    ...
    guest_physmap_remove_page()
    ...
}

#3. a new helper is good to further extend if necessary.

Of course, I'd like to modify-to-use guest_physmap_remove_page() if you 
guys aren't in agreement with me :)
Thanks
Tiejun

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