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 14:13:27 +0800 Message-ID: <557A7887.3030104@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> <557A74EE.5090805@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Tian, Kevin" , Tim Deegan Cc: "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 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