From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: [PATCH v2] AMD-Vi: allocate root table on demand Date: Mon, 20 Mar 2017 07:55:14 -0600 Message-ID: <58CFED5202000078001451AF@prv-mh.provo.novell.com> References: <58CFED5202000078001451AF@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=__Part724BE852.1__=" Return-path: Received: from mail6.bemta6.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cpxmM-0004jE-Ny for xen-devel@lists.xenproject.org; Mon, 20 Mar 2017 13:55:22 +0000 List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: xen-devel Cc: Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org This is a MIME message. If you are reading this text, you may want to consider changing to a mail reader or gateway that understands how to properly handle MIME multipart messages. --=__Part724BE852.1__= Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline This was my originally intended fix for the AMD side of XSA-207: There's no need to unconditionally allocate the root table, and with that there's then also no way to leak it when a guest has no devices assigned. Signed-off-by: Jan Beulich --- v2: Add AMD_IOMMU_DEBUG() + domain_crash() to amd_iommu_map_page() error path for consistency with other code in this function. --- a/xen/drivers/passthrough/amd/iommu_map.c +++ b/xen/drivers/passthrough/amd/iommu_map.c @@ -636,11 +636,10 @@ int amd_iommu_map_page(struct domain *d, { bool_t need_flush =3D 0; struct domain_iommu *hd =3D dom_iommu(d); + int rc; unsigned long pt_mfn[7]; unsigned int merge_level; =20 - BUG_ON( !hd->arch.root_table ); - if ( iommu_use_hap_pt(d) ) return 0; =20 @@ -648,6 +647,15 @@ int amd_iommu_map_page(struct domain *d, =20 spin_lock(&hd->arch.mapping_lock); =20 + rc =3D amd_iommu_alloc_root(hd); + if ( rc ) + { + spin_unlock(&hd->arch.mapping_lock); + AMD_IOMMU_DEBUG("Root table alloc failed, gfn =3D %lx\n", gfn); + domain_crash(d); + return rc; + } + /* Since HVM domain is initialized with 2 level IO page table, * we might need a deeper page table for lager gfn now */ if ( is_hvm_domain(d) ) @@ -717,8 +725,6 @@ int amd_iommu_unmap_page(struct domain * unsigned long pt_mfn[7]; struct domain_iommu *hd =3D dom_iommu(d); =20 - BUG_ON( !hd->arch.root_table ); - if ( iommu_use_hap_pt(d) ) return 0; =20 @@ -726,6 +732,12 @@ int amd_iommu_unmap_page(struct domain * =20 spin_lock(&hd->arch.mapping_lock); =20 + if ( !hd->arch.root_table ) + { + spin_unlock(&hd->arch.mapping_lock); + return 0; + } + /* Since HVM domain is initialized with 2 level IO page table, * we might need a deeper page table for lager gfn now */ if ( is_hvm_domain(d) ) --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -222,23 +222,29 @@ int __init amd_iov_detect(void) return scan_pci_devices(); } =20 -static int allocate_domain_resources(struct domain_iommu *hd) +int amd_iommu_alloc_root(struct domain_iommu *hd) { - /* allocate root table */ - spin_lock(&hd->arch.mapping_lock); - if ( !hd->arch.root_table ) + if ( unlikely(!hd->arch.root_table) ) { hd->arch.root_table =3D alloc_amd_iommu_pgtable(); if ( !hd->arch.root_table ) - { - spin_unlock(&hd->arch.mapping_lock); return -ENOMEM; - } } - spin_unlock(&hd->arch.mapping_lock); + return 0; } =20 +static int __must_check allocate_domain_resources(struct domain_iommu = *hd) +{ + int rc; + + spin_lock(&hd->arch.mapping_lock); + rc =3D amd_iommu_alloc_root(hd); + spin_unlock(&hd->arch.mapping_lock); + + return rc; +} + static int get_paging_mode(unsigned long entries) { int level =3D 1; @@ -259,14 +265,6 @@ static int amd_iommu_domain_init(struct { struct domain_iommu *hd =3D dom_iommu(d); =20 - /* allocate page directroy */ - if ( allocate_domain_resources(hd) !=3D 0 ) - { - if ( hd->arch.root_table ) - free_domheap_page(hd->arch.root_table); - return -ENOMEM; - } - /* For pv and dom0, stick with get_paging_mode(max_page) * For HVM dom0, use 2 level page table at first */ hd->arch.paging_mode =3D is_hvm_domain(d) ? @@ -280,6 +278,9 @@ static void __hwdom_init amd_iommu_hwdom unsigned long i;=20 const struct amd_iommu *iommu; =20 + if ( allocate_domain_resources(dom_iommu(d)) ) + BUG(); + if ( !iommu_passthrough && !need_iommu(d) ) { int rc =3D 0; @@ -363,7 +364,7 @@ static int reassign_device(struct domain u8 devfn, struct pci_dev *pdev) { struct amd_iommu *iommu; - int bdf; + int bdf, rc; struct domain_iommu *t =3D dom_iommu(target); =20 bdf =3D PCI_BDF2(pdev->bus, pdev->devfn); @@ -385,10 +386,9 @@ static int reassign_device(struct domain pdev->domain =3D target; } =20 - /* IO page tables might be destroyed after pci-detach the last device - * In this case, we have to re-allocate root table for next pci-attach= .*/ - if ( t->arch.root_table =3D=3D NULL ) - allocate_domain_resources(t); + rc =3D allocate_domain_resources(t); + if ( rc ) + return rc; =20 amd_iommu_setup_domain_device(target, iommu, devfn, pdev); AMD_IOMMU_DEBUG("Re-assign %04x:%02x:%02x.%u from dom%d to dom%d\n", --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h @@ -56,6 +56,7 @@ int __must_check amd_iommu_map_page(stru unsigned long mfn, unsigned int = flags); int __must_check amd_iommu_unmap_page(struct domain *d, unsigned long = gfn); u64 amd_iommu_get_next_table_from_pte(u32 *entry); +int __must_check amd_iommu_alloc_root(struct domain_iommu *hd); int amd_iommu_reserve_domain_unity_map(struct domain *domain, u64 phys_addr, unsigned long size, int iw, int ir); --=__Part724BE852.1__= Content-Type: text/plain; name="AMD-IOMMU-root-table-leak.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="AMD-IOMMU-root-table-leak.patch" AMD-Vi: allocate root table on demand=0A=0AThis was my originally intended = fix for the AMD side of XSA-207:=0AThere's no need to unconditionally = allocate the root table, and with=0Athat there's then also no way to leak = it when a guest has no devices=0Aassigned.=0A=0ASigned-off-by: Jan Beulich = =0A---=0Av2: Add AMD_IOMMU_DEBUG() + domain_crash() to = amd_iommu_map_page()=0A error path for consistency with other code in = this function.=0A=0A--- a/xen/drivers/passthrough/amd/iommu_map.c=0A+++ = b/xen/drivers/passthrough/amd/iommu_map.c=0A@@ -636,11 +636,10 @@ int = amd_iommu_map_page(struct domain *d,=0A {=0A bool_t need_flush =3D = 0;=0A struct domain_iommu *hd =3D dom_iommu(d);=0A+ int rc;=0A = unsigned long pt_mfn[7];=0A unsigned int merge_level;=0A =0A- = BUG_ON( !hd->arch.root_table );=0A-=0A if ( iommu_use_hap_pt(d) )=0A = return 0;=0A =0A@@ -648,6 +647,15 @@ int amd_iommu_map_page(struct = domain *d,=0A =0A spin_lock(&hd->arch.mapping_lock);=0A =0A+ rc =3D = amd_iommu_alloc_root(hd);=0A+ if ( rc )=0A+ {=0A+ spin_unlock(= &hd->arch.mapping_lock);=0A+ AMD_IOMMU_DEBUG("Root table alloc = failed, gfn =3D %lx\n", gfn);=0A+ domain_crash(d);=0A+ = return rc;=0A+ }=0A+=0A /* Since HVM domain is initialized with 2 = level IO page table,=0A * we might need a deeper page table for lager = gfn now */=0A if ( is_hvm_domain(d) )=0A@@ -717,8 +725,6 @@ int = amd_iommu_unmap_page(struct domain *=0A unsigned long pt_mfn[7];=0A = struct domain_iommu *hd =3D dom_iommu(d);=0A =0A- BUG_ON( !hd->arch.roo= t_table );=0A-=0A if ( iommu_use_hap_pt(d) )=0A return 0;=0A = =0A@@ -726,6 +732,12 @@ int amd_iommu_unmap_page(struct domain *=0A =0A = spin_lock(&hd->arch.mapping_lock);=0A =0A+ if ( !hd->arch.root_table = )=0A+ {=0A+ spin_unlock(&hd->arch.mapping_lock);=0A+ = return 0;=0A+ }=0A+=0A /* Since HVM domain is initialized with 2 = level IO page table,=0A * we might need a deeper page table for lager = gfn now */=0A if ( is_hvm_domain(d) )=0A--- a/xen/drivers/passthrough/a= md/pci_amd_iommu.c=0A+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c=0A@@= -222,23 +222,29 @@ int __init amd_iov_detect(void)=0A return = scan_pci_devices();=0A }=0A =0A-static int allocate_domain_resources(struct= domain_iommu *hd)=0A+int amd_iommu_alloc_root(struct domain_iommu *hd)=0A = {=0A- /* allocate root table */=0A- spin_lock(&hd->arch.mapping_lock)= ;=0A- if ( !hd->arch.root_table )=0A+ if ( unlikely(!hd->arch.root_ta= ble) )=0A {=0A hd->arch.root_table =3D alloc_amd_iommu_pgtable(= );=0A if ( !hd->arch.root_table )=0A- {=0A- = spin_unlock(&hd->arch.mapping_lock);=0A return -ENOMEM;=0A- = }=0A }=0A- spin_unlock(&hd->arch.mapping_lock);=0A+=0A = return 0;=0A }=0A =0A+static int __must_check allocate_domain_resources(str= uct domain_iommu *hd)=0A+{=0A+ int rc;=0A+=0A+ spin_lock(&hd->arch.ma= pping_lock);=0A+ rc =3D amd_iommu_alloc_root(hd);=0A+ spin_unlock(&hd= ->arch.mapping_lock);=0A+=0A+ return rc;=0A+}=0A+=0A static int = get_paging_mode(unsigned long entries)=0A {=0A int level =3D 1;=0A@@ = -259,14 +265,6 @@ static int amd_iommu_domain_init(struct=0A {=0A = struct domain_iommu *hd =3D dom_iommu(d);=0A =0A- /* allocate page = directroy */=0A- if ( allocate_domain_resources(hd) !=3D 0 )=0A- = {=0A- if ( hd->arch.root_table )=0A- free_domheap_page(hd= ->arch.root_table);=0A- return -ENOMEM;=0A- }=0A-=0A /* For = pv and dom0, stick with get_paging_mode(max_page)=0A * For HVM dom0, = use 2 level page table at first */=0A hd->arch.paging_mode =3D = is_hvm_domain(d) ?=0A@@ -280,6 +278,9 @@ static void __hwdom_init = amd_iommu_hwdom=0A unsigned long i; =0A const struct amd_iommu = *iommu;=0A =0A+ if ( allocate_domain_resources(dom_iommu(d)) )=0A+ = BUG();=0A+=0A if ( !iommu_passthrough && !need_iommu(d) )=0A = {=0A int rc =3D 0;=0A@@ -363,7 +364,7 @@ static int reassign_device= (struct domain=0A u8 devfn, struct pci_dev = *pdev)=0A {=0A struct amd_iommu *iommu;=0A- int bdf;=0A+ int = bdf, rc;=0A struct domain_iommu *t =3D dom_iommu(target);=0A =0A = bdf =3D PCI_BDF2(pdev->bus, pdev->devfn);=0A@@ -385,10 +386,9 @@ static = int reassign_device(struct domain=0A pdev->domain =3D target;=0A = }=0A =0A- /* IO page tables might be destroyed after pci-detach the = last device=0A- * In this case, we have to re-allocate root table for = next pci-attach.*/=0A- if ( t->arch.root_table =3D=3D NULL )=0A- = allocate_domain_resources(t);=0A+ rc =3D allocate_domain_resources(t);= =0A+ if ( rc )=0A+ return rc;=0A =0A amd_iommu_setup_domain_d= evice(target, iommu, devfn, pdev);=0A AMD_IOMMU_DEBUG("Re-assign = %04x:%02x:%02x.%u from dom%d to dom%d\n",=0A--- a/xen/include/asm-x86/hvm/s= vm/amd-iommu-proto.h=0A+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h= =0A@@ -56,6 +56,7 @@ int __must_check amd_iommu_map_page(stru=0A = unsigned long mfn, unsigned int flags);=0A int = __must_check amd_iommu_unmap_page(struct domain *d, unsigned long gfn);=0A = u64 amd_iommu_get_next_table_from_pte(u32 *entry);=0A+int __must_check = amd_iommu_alloc_root(struct domain_iommu *hd);=0A int amd_iommu_reserve_dom= ain_unity_map(struct domain *domain,=0A = u64 phys_addr, unsigned long size,=0A = int iw, int ir);=0A --=__Part724BE852.1__= Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --=__Part724BE852.1__=--