From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] IOMMU/x86: avoid pages without GFN in page table creation/updating Date: Thu, 7 May 2015 14:13:38 +0100 Message-ID: <554B6502.8010206@citrix.com> References: <554B7DEB020000780007793D@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YqLpr-0001nW-GU for xen-devel@lists.xenproject.org; Thu, 07 May 2015 13:27:31 +0000 In-Reply-To: <554B7DEB020000780007793D@mail.emea.novell.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: Jan Beulich , xen-devel Cc: Kevin Tian , Tim Deegan , Sander Eikelenboom , Aravind Gopalakrishnan , suravee.suthikulpanit@amd.com, Yang Z Zhang List-Id: xen-devel@lists.xenproject.org On 07/05/15 13:59, Jan Beulich wrote: > Handing INVALID_GFN to functions like hd->platform_ops->map_page() > just can't do any good, and the ioreq server code results in such pages > being on the list of ones owned by a guest. > > While - as suggested by Tim - we should use get_gfn()/put_gfn() there > to eliminate races, we really can't due to holding the domain's page > alloc lock. Ultimately arch_iommu_populate_page_table() may need to be > switched to be GFN based. Here is what Tim said in this regard: > "Ideally this loop would be iterating over all gfns in the p2m rather > than over all owned MFNs. As long as needs_iommu gets set first, > such a loop could safely be paused and restarted without worrying > about concurrent updates. The code sould even stay in this file, > though exposing an iterator from the p2m code would be a lot more > efficient." > > Original by Andrew Cooper , using further > suggestions from Tim Deegan . > > Reported-by: Sander Eikelenboom > Signed-off-by: Jan Beulich > Tested-by: Sander Eikelenboom > Acked-by: Tim Deegan Reviewed-by: Andrew Cooper , albeit with one further suggestion. > > --- a/xen/drivers/passthrough/amd/iommu_map.c > +++ b/xen/drivers/passthrough/amd/iommu_map.c > @@ -557,6 +557,10 @@ static int update_paging_mode(struct dom > unsigned long old_root_mfn; > struct hvm_iommu *hd = domain_hvm_iommu(d); > > + if ( gfn == INVALID_MFN ) > + return -EADDRNOTAVAIL; > + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH)); > + > level = hd->arch.paging_mode; > old_root = hd->arch.root_table; > offset = gfn >> (PTE_PER_TABLE_SHIFT * (level - 1)); > @@ -729,12 +733,15 @@ int amd_iommu_unmap_page(struct domain * > * we might need a deeper page table for lager gfn now */ > if ( is_hvm_domain(d) ) > { > - if ( update_paging_mode(d, gfn) ) > + int rc = update_paging_mode(d, gfn); > + > + if ( rc ) > { > spin_unlock(&hd->arch.mapping_lock); > AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn); Given the domain_crash(), this should probably at error priority rather than debug. ~Andrew > - domain_crash(d); > - return -EFAULT; > + if ( rc != -EADDRNOTAVAIL ) > + domain_crash(d); > + return rc; > } > } > > --- a/xen/drivers/passthrough/vtd/iommu.h > +++ b/xen/drivers/passthrough/vtd/iommu.h > @@ -482,7 +482,6 @@ struct qinval_entry { > #define VTD_PAGE_TABLE_LEVEL_3 3 > #define VTD_PAGE_TABLE_LEVEL_4 4 > > -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 > #define MAX_IOMMU_REGS 0xc0 > > extern struct list_head acpi_drhd_units; > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -59,10 +59,17 @@ int arch_iommu_populate_page_table(struc > if ( has_hvm_container_domain(d) || > (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page ) > { > - BUG_ON(SHARED_M2P(mfn_to_gmfn(d, page_to_mfn(page)))); > - rc = hd->platform_ops->map_page( > - d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page), > - IOMMUF_readable|IOMMUF_writable); > + unsigned long mfn = page_to_mfn(page); > + unsigned long gfn = mfn_to_gmfn(d, mfn); > + > + if ( gfn != INVALID_MFN ) > + { > + ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH)); > + BUG_ON(SHARED_M2P(gfn)); > + rc = hd->platform_ops->map_page(d, gfn, mfn, > + IOMMUF_readable | > + IOMMUF_writable); > + } > if ( rc ) > { > page_list_add(page, &d->page_list); > --- a/xen/include/asm-x86/hvm/iommu.h > +++ b/xen/include/asm-x86/hvm/iommu.h > @@ -46,6 +46,8 @@ struct g2m_ioport { > unsigned int np; > }; > > +#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 > + > struct arch_hvm_iommu > { > u64 pgd_maddr; /* io page directory machine address */ > --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h > +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h > @@ -464,8 +464,6 @@ > #define IOMMU_CONTROL_DISABLED 0 > #define IOMMU_CONTROL_ENABLED 1 > > -#define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 > - > /* interrupt remapping table */ > #define INT_REMAP_ENTRY_REMAPEN_MASK 0x00000001 > #define INT_REMAP_ENTRY_REMAPEN_SHIFT 0 > >