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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> 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. :-)

Thanks
Kevin


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