From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v3][PATCH 03/16] xen/vtd: create RMRR mapping Date: Fri, 12 Jun 2015 13:58:06 +0800 Message-ID: <557A74EE.5090805@intel.com> References: <1433985325-16676-1-git-send-email-tiejun.chen@intel.com> <1433985325-16676-4-git-send-email-tiejun.chen@intel.com> <5579558C.30709@intel.com> <20150611140729.GC1734@deinos.phlegethon.org> <557A4745.1030608@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <557A4745.1030608@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: "Tian, Kevin" , "wei.liu2@citrix.com" , "ian.campbell@citrix.com" , "andrew.cooper3@citrix.com" , "Ian.Jackson@eu.citrix.com" , "xen-devel@lists.xen.org" , "stefano.stabellini@citrix.com" , "jbeulich@suse.com" , "Zhang, Yang Z" List-Id: xen-devel@lists.xenproject.org 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