[prev in list] [next in list] [prev in thread] [next in thread]
List: xen-users
Subject: [Xen-users] Xen Security Advisory 311 v4 (CVE-2019-19577) - Bugs in dynamic height handling for AMD
From: Xen.org security team <security () xen ! org>
Date: 2019-12-11 12:09:24
Message-ID: E1if0o0-0001cS-Uo () xenbits ! xenproject ! org
[Download RAW message or body]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2019-19577 / XSA-311
version 4
Bugs in dynamic height handling for AMD IOMMU pagetables
UPDATES IN VERSION 4
====================
Public release.
Re-base 4.12 patch onto latest stable tree commits.
Updated metadata to add 4.13, update StableRef's
ISSUE DESCRIPTION
=================
When running on AMD systems with an IOMMU, Xen attempted to
dynamically adapt the number of levels of pagetables (the pagetable
height) in the IOMMU according to the guest's address space size. The
code to select and update the height had several bugs.
Notably, the update was done without taking a lock which is necessary
for safe operation.
IMPACT
======
A malicious guest administrator can cause Xen to access data
structures while they are being modified, causing Xen to crash.
Privilege escalation is thought to be very difficult but cannot be
ruled out.
Additionally, there is a potential memory leak of 4kb per guest boot,
under memory pressure.
VULNERABLE SYSTEMS
==================
Only Xen on AMD CPUs is vulnerable. Xen running on Intel CPUs is not
vulnerable. ARM systems are not vulnerable.
Only systems where guests are given direct access to physical devices
are vulnerable. Systems which do not use PCI pass-through are not
vulnerable.
Only HVM guests can exploit the vulnerability. PV and PVH guests
cannot.
All versions of Xen with IOMMU support are vulnerable.
MITIGATION
==========
In some configurations, use of passthrough can be replaced with a
higher-level protocol such as Xen PV block or network devices.
There is no other mitigation.
CREDITS
=======
This issue was discovered by Sander Eikelenboom, along with Andrew Cooper of
Citrix.
RESOLUTION
==========
Applying the appropriate (set of) attached patch(es) resolves this issue.
xsa311.patch xen-unstable, Xen 4.13.x
xsa311-4.12.patch Xen 4.12.x
xsa311-4.11.patch Xen 4.11.x
xsa311-4.10-*.patch Xen 4.10.x
xsa311-4.9-*.patch Xen 4.9.x
xsa311-4.8-*.patch Xen 4.8.x
$ sha256sum xsa311*
ea929752043b5d4659cb605314887441daa33ee6450e755d6f077e57fc7abf9e xsa311.meta
732975f33b6d893b984540c4c748eb5cdf1cf81bd565e41b57795458cae3ccad xsa311.patch
27e30da9360eec850f6e7d8f2ea465d2f00a5a5a45c43042e4c18786c6c9338f xsa311-4.8-1.patch
6e2372eb18f3ca25093445a93bcdf674ed2d7d3012e8611911ea2b9ca8d58bd4 xsa311-4.8-2.patch
c73bee7aa8fac02d0982b4fb21de053918f80cc0158bd5bfca68e3dc994759be xsa311-4.9-1.patch
e89f5c381bd6a8fa8c5f63a829b586fdbefefe311c0f1084d2baeea3e933da66 xsa311-4.9-2.patch
c73bee7aa8fac02d0982b4fb21de053918f80cc0158bd5bfca68e3dc994759be xsa311-4.10-1.patch
189a51048ad88efd855e6e78a307fff68e0c139225ce528c253558d266fffe02 xsa311-4.10-2.patch
1aaf26d1c231c8b5dd00900c00c18bf884d23b9568c9746866d92f39daf1c02f xsa311-4.11.patch
5f43fa4628f6d1a8f6f903e662226a09524b8c354e06e1a6039837db656c0218 xsa311-4.12.patch
$
DEPLOYMENT DURING EMBARGO
=========================
Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.
But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).
Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.
(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable. This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)
For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----
iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAl3w3F8MHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZgF0IAIOtY9LMbRkBWgc16lOs+MTDOC7h4fYqofjQetFN
wAJ2Q3w2QXN+Zt54L8dmc6+Zzvn9Do4AJeMvfCzFxuw2OaMBwcwI9DcEbZ+CvYsa
hiXf9xKBBEfCu8PjisRnBqKuyqrLQdBSad9vXcGOVloXiFzJ1wbKnSMBNig9ZTi2
us3c9MeUTnf95W/KTQNe2Gu8KQiogzzBUUifdB6YU0MNNhL60OzfSwgautD9XHfA
+NcRogDnf6KgAs6VKgHSDxyVWbvnaWvKWGF2M2QXwXHjqCH/ox87OIIgZ/HSodXB
e07vCaweCG4GgWDGQN5K3+9Cu1B6+t0RYzPYmuhPDy/kWF0=
=RJ0B
-----END PGP SIGNATURE-----
["xsa311.meta" (application/octet-stream)]
["xsa311.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables
update_paging_mode() has multiple bugs:
1) Booting with iommu=debug will cause it to inform you that that it called
without the pdev_list lock held.
2) When growing by more than a single level, it leaks the newly allocated
table(s) in the case of a further error.
Furthermore, the choice of default level for a domain has issues:
1) All HVM guests grow from 2 to 3 levels during construction because of the
position of the VRAM just below the 4G boundary, so defaulting to 2 is a
waste of effort.
2) The limit for PV guests doesn't take memory hotplug into account, and
isn't dynamic at runtime like HVM guests. This means that a PV guest may
get RAM which it can't map in the IOMMU.
The dynamic height is a property unique to AMD, and adds a substantial
quantity of complexity for what is a marginal performance improvement. Remove
the complexity by removing the dynamic height.
PV guests now get 3 or 4 levels based on any hotplug regions in the host.
This only makes a difference for hardware which previously had all RAM below
the 512G boundary, and a hotplug region above.
HVM guests now get 4 levels (which will be sufficient until 256TB guests
become a thing), because we don't currently have the information to know when
3 would be safe to use.
The overhead of this extra level is not expected to be noticeable. It costs
one page (4k) per domain, and one extra IO-TLB paging structure cache entry
which is very hot and less likely to be evicted.
This is XSA-311.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 54e1d132d9..4e041b960f 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -285,100 +285,6 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
return 0;
}
-static int update_paging_mode(struct domain *d, unsigned long dfn)
-{
- uint16_t bdf;
- struct amd_iommu_dte *table, *dte;
- unsigned int req_id, level, offset;
- unsigned long flags;
- struct pci_dev *pdev;
- struct amd_iommu *iommu = NULL;
- struct page_info *new_root = NULL;
- struct page_info *old_root = NULL;
- struct amd_iommu_pte *new_root_vaddr;
- unsigned long old_root_mfn;
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( dfn == dfn_x(INVALID_DFN) )
- return -EADDRNOTAVAIL;
- ASSERT(!(dfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
-
- level = hd->arch.paging_mode;
- old_root = hd->arch.root_table;
- offset = dfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
-
- ASSERT(spin_is_locked(&hd->arch.mapping_lock) && is_hvm_domain(d));
-
- while ( offset >= PTE_PER_TABLE_SIZE )
- {
- /* Allocate and install a new root table.
- * Only upper I/O page table grows, no need to fix next level bits */
- new_root = alloc_amd_iommu_pgtable();
- if ( new_root == NULL )
- {
- AMD_IOMMU_DEBUG("%s Cannot allocate I/O page table\n",
- __func__);
- return -ENOMEM;
- }
-
- new_root_vaddr = __map_domain_page(new_root);
- old_root_mfn = mfn_x(page_to_mfn(old_root));
- set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
- true, true);
- level++;
- old_root = new_root;
- offset >>= PTE_PER_TABLE_SHIFT;
- unmap_domain_page(new_root_vaddr);
- }
-
- if ( new_root != NULL )
- {
- hd->arch.paging_mode = level;
- hd->arch.root_table = new_root;
-
- if ( !pcidevs_locked() )
- AMD_IOMMU_DEBUG("%s Try to access pdev_list "
- "without aquiring pcidevs_lock.\n", __func__);
-
- /* Update device table entries using new root table and paging mode */
- for_each_pdev( d, pdev )
- {
- if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE )
- continue;
-
- bdf = PCI_BDF2(pdev->bus, pdev->devfn);
- iommu = find_iommu_for_device(pdev->seg, bdf);
- if ( !iommu )
- {
- AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
- return -ENODEV;
- }
-
- spin_lock_irqsave(&iommu->lock, flags);
- do {
- req_id = get_dma_requestor_id(pdev->seg, bdf);
- table = iommu->dev_table.buffer;
- dte = &table[req_id];
-
- /* valid = 0 only works for dom0 passthrough mode */
- amd_iommu_set_root_page_table(dte,
- page_to_maddr(hd->arch.root_table),
- d->domain_id,
- hd->arch.paging_mode, 1);
-
- amd_iommu_flush_device(iommu, req_id);
- bdf += pdev->phantom_stride;
- } while ( PCI_DEVFN2(bdf) != pdev->devfn &&
- PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
- spin_unlock_irqrestore(&iommu->lock, flags);
- }
-
- /* For safety, invalidate all entries */
- amd_iommu_flush_all_pages(d);
- }
- return 0;
-}
-
int amd_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
unsigned int flags, unsigned int *flush_flags)
{
@@ -400,20 +306,6 @@ int amd_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
return rc;
}
- /* Since HVM domain is initialized with 2 level IO page table,
- * we might need a deeper page table for wider dfn now */
- if ( is_hvm_domain(d) )
- {
- if ( update_paging_mode(d, dfn_x(dfn)) )
- {
- spin_unlock(&hd->arch.mapping_lock);
- AMD_IOMMU_DEBUG("Update page mode failed dfn = %"PRI_dfn"\n",
- dfn_x(dfn));
- domain_crash(d);
- return -EFAULT;
- }
- }
-
if ( iommu_pde_from_dfn(d, dfn_x(dfn), pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 4da6518773..dd3401f0dc 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -251,10 +251,17 @@ static int amd_iommu_domain_init(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
- /* 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 = is_hvm_domain(d) ?
- 2 : amd_iommu_get_paging_mode(max_page);
+ /*
+ * Choose the number of levels for the IOMMU page tables.
+ * - PV needs 3 or 4, depending on whether there is RAM (including hotplug
+ * RAM) above the 512G boundary.
+ * - HVM could in principle use 3 or 4 depending on how much guest
+ * physical address space we give it, but this isn't known yet so use 4
+ * unilaterally.
+ */
+ hd->arch.paging_mode = is_hvm_domain(d)
+ ? 4 : amd_iommu_get_paging_mode(get_upper_mfn_bound());
+
return 0;
}
["xsa311-4.8-1.patch" (application/octet-stream)]
From: Jan Beulich <jbeulich@suse.com>
Subject: AMD/IOMMU: don't needlessly trigger errors/crashes when unmapping a page
Unmapping a page which has never been mapped should be a no-op (note how
it already is in case there was no root page table allocated). There's
in particular no need to grow the number of page table levels in use,
and there's also no need to allocate intermediate page tables except
when needing to split a large page.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -457,7 +457,7 @@ static int iommu_merge_pages(struct doma
* page tables.
*/
static int iommu_pde_from_gfn(struct domain *d, unsigned long pfn,
- unsigned long pt_mfn[])
+ unsigned long pt_mfn[], bool map)
{
u64 *pde, *next_table_vaddr;
unsigned long next_table_mfn;
@@ -471,6 +471,13 @@ static int iommu_pde_from_gfn(struct dom
BUG_ON( table == NULL || level < IOMMU_PAGING_MODE_LEVEL_1 ||
level > IOMMU_PAGING_MODE_LEVEL_6 );
+ /*
+ * A frame number past what the current page tables can represent can't
+ * possibly have a mapping.
+ */
+ if ( pfn >> (PTE_PER_TABLE_SHIFT * level) )
+ return 0;
+
next_table_mfn = page_to_mfn(table);
if ( level == IOMMU_PAGING_MODE_LEVEL_1 )
@@ -531,6 +538,9 @@ static int iommu_pde_from_gfn(struct dom
/* Install lower level page table for non-present entries */
else if ( !iommu_is_pte_present((u32*)pde) )
{
+ if ( !map )
+ return 0;
+
if ( next_table_mfn == 0 )
{
table = alloc_amd_iommu_pgtable();
@@ -681,7 +691,7 @@ int amd_iommu_map_page(struct domain *d,
}
}
- if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
+ if ( iommu_pde_from_gfn(d, gfn, pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
@@ -756,23 +766,7 @@ int amd_iommu_unmap_page(struct domain *
spin_lock(&hd->arch.mapping_lock);
- /* 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) )
- {
- 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);
- if ( rc != -EADDRNOTAVAIL )
- domain_crash(d);
- return rc;
- }
- }
-
- if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
+ if ( iommu_pde_from_gfn(d, gfn, pt_mfn, false) )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
@@ -780,8 +774,11 @@ int amd_iommu_unmap_page(struct domain *
return -EFAULT;
}
- /* mark PTE as 'page not present' */
- clear_iommu_pte_present(pt_mfn[1], gfn);
+ if ( pt_mfn[1] )
+ {
+ /* Mark PTE as 'page not present'. */
+ clear_iommu_pte_present(pt_mfn[1], gfn);
+ }
/* No further merging in amd_iommu_map_page(), as the logic doesn't cope. */
hd->arch.no_merge = true;
["xsa311-4.8-2.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables
update_paging_mode() has multiple bugs:
1) Booting with iommu=debug will cause it to inform you that that it called
without the pdev_list lock held.
2) When growing by more than a single level, it leaks the newly allocated
table(s) in the case of a further error.
Furthermore, the choice of default level for a domain has issues:
1) All HVM guests grow from 2 to 3 levels during construction because of the
position of the VRAM just below the 4G boundary, so defaulting to 2 is a
waste of effort.
2) The limit for PV guests doesn't take memory hotplug into account, and
isn't dynamic at runtime like HVM guests. This means that a PV guest may
get RAM which it can't map in the IOMMU.
The dynamic height is a property unique to AMD, and adds a substantial
quantity of complexity for what is a marginal performance improvement. Remove
the complexity by removing the dynamic height.
PV guests now get 3 or 4 levels based on any hotplug regions in the host.
This only makes a difference for hardware which previously had all RAM below
the 512G boundary, and a hotplug region above.
HVM guests now get 4 levels (which will be sufficient until 256TB guests
become a thing), because we don't currently have the information to know when
3 would be safe to use.
The overhead of this extra level is not expected to be noticeable. It costs
one page (4k) per domain, and one extra IO-TLB paging structure cache entry
which is very hot and less likely to be evicted.
This is XSA-311.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -7403,6 +7403,17 @@ void paging_invlpg(struct vcpu *v, unsig
hvm_funcs.invlpg(v, va);
}
+unsigned long get_upper_mfn_bound(void)
+{
+ unsigned long max_mfn;
+
+ max_mfn = mem_hotplug ? PFN_DOWN(mem_hotplug) : max_page;
+#ifndef CONFIG_BIGMEM
+ max_mfn = min(max_mfn, 1UL << 32);
+#endif
+ return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
+}
+
/*
* Local variables:
* mode: C
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -570,97 +570,6 @@ static int iommu_pde_from_gfn(struct dom
return 0;
}
-static int update_paging_mode(struct domain *d, unsigned long gfn)
-{
- u16 bdf;
- void *device_entry;
- unsigned int req_id, level, offset;
- unsigned long flags;
- struct pci_dev *pdev;
- struct amd_iommu *iommu = NULL;
- struct page_info *new_root = NULL;
- struct page_info *old_root = NULL;
- void *new_root_vaddr;
- unsigned long old_root_mfn;
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( gfn == gfn_x(INVALID_GFN) )
- 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));
-
- ASSERT(spin_is_locked(&hd->arch.mapping_lock) && is_hvm_domain(d));
-
- while ( offset >= PTE_PER_TABLE_SIZE )
- {
- /* Allocate and install a new root table.
- * Only upper I/O page table grows, no need to fix next level bits */
- new_root = alloc_amd_iommu_pgtable();
- if ( new_root == NULL )
- {
- AMD_IOMMU_DEBUG("%s Cannot allocate I/O page table\n",
- __func__);
- return -ENOMEM;
- }
-
- new_root_vaddr = __map_domain_page(new_root);
- old_root_mfn = page_to_mfn(old_root);
- set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
- !!IOMMUF_writable, !!IOMMUF_readable);
- level++;
- old_root = new_root;
- offset >>= PTE_PER_TABLE_SHIFT;
- unmap_domain_page(new_root_vaddr);
- }
-
- if ( new_root != NULL )
- {
- hd->arch.paging_mode = level;
- hd->arch.root_table = new_root;
-
- if ( !pcidevs_locked() )
- AMD_IOMMU_DEBUG("%s Try to access pdev_list "
- "without aquiring pcidevs_lock.\n", __func__);
-
- /* Update device table entries using new root table and paging mode */
- for_each_pdev( d, pdev )
- {
- bdf = PCI_BDF2(pdev->bus, pdev->devfn);
- iommu = find_iommu_for_device(pdev->seg, bdf);
- if ( !iommu )
- {
- AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
- return -ENODEV;
- }
-
- spin_lock_irqsave(&iommu->lock, flags);
- do {
- req_id = get_dma_requestor_id(pdev->seg, bdf);
- device_entry = iommu->dev_table.buffer +
- (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
-
- /* valid = 0 only works for dom0 passthrough mode */
- amd_iommu_set_root_page_table((u32 *)device_entry,
- page_to_maddr(hd->arch.root_table),
- d->domain_id,
- hd->arch.paging_mode, 1);
-
- amd_iommu_flush_device(iommu, req_id);
- bdf += pdev->phantom_stride;
- } while ( PCI_DEVFN2(bdf) != pdev->devfn &&
- PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
- spin_unlock_irqrestore(&iommu->lock, flags);
- }
-
- /* For safety, invalidate all entries */
- amd_iommu_flush_all_pages(d);
- }
- return 0;
-}
-
int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
@@ -678,19 +587,6 @@ int amd_iommu_map_page(struct domain *d,
spin_lock(&hd->arch.mapping_lock);
- /* 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) )
- {
- if ( update_paging_mode(d, gfn) )
- {
- spin_unlock(&hd->arch.mapping_lock);
- AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
- domain_crash(d);
- return -EFAULT;
- }
- }
-
if ( iommu_pde_from_gfn(d, gfn, pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -271,11 +271,17 @@ static int amd_iommu_domain_init(struct
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 = is_hvm_domain(d) ?
- IOMMU_PAGING_MODE_LEVEL_2 :
- get_paging_mode(max_page);
+ /*
+ * Choose the number of levels for the IOMMU page tables.
+ * - PV needs 3 or 4, depending on whether there is RAM (including hotplug
+ * RAM) above the 512G boundary.
+ * - HVM could in principle use 3 or 4 depending on how much guest
+ * physical address space we give it, but this isn't known yet so use 4
+ * unilaterally.
+ */
+ hd->arch.paging_mode = is_hvm_domain(d)
+ ? IOMMU_PAGING_MODE_LEVEL_4 : get_paging_mode(get_upper_mfn_bound());
+
return 0;
}
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -577,6 +577,9 @@ int prepare_ring_for_helper(struct domai
struct page_info **_page, void **_va);
void destroy_ring_for_helper(void **_va, struct page_info *page);
+/* Return the upper bound of MFNs, including hotplug memory. */
+unsigned long get_upper_mfn_bound(void);
+
#include <asm/flushtlb.h>
static inline void accumulate_tlbflush(bool *need_tlbflush,
["xsa311-4.9-1.patch" (application/octet-stream)]
From: Jan Beulich <jbeulich@suse.com>
Subject: AMD/IOMMU: don't needlessly trigger errors/crashes when unmapping a page
Unmapping a page which has never been mapped should be a no-op (note how
it already is in case there was no root page table allocated). There's
in particular no need to grow the number of page table levels in use,
and there's also no need to allocate intermediate page tables except
when needing to split a large page.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -456,7 +456,7 @@ static int iommu_merge_pages(struct doma
* page tables.
*/
static int iommu_pde_from_gfn(struct domain *d, unsigned long pfn,
- unsigned long pt_mfn[])
+ unsigned long pt_mfn[], bool map)
{
u64 *pde, *next_table_vaddr;
unsigned long next_table_mfn;
@@ -470,6 +470,13 @@ static int iommu_pde_from_gfn(struct dom
BUG_ON( table == NULL || level < IOMMU_PAGING_MODE_LEVEL_1 ||
level > IOMMU_PAGING_MODE_LEVEL_6 );
+ /*
+ * A frame number past what the current page tables can represent can't
+ * possibly have a mapping.
+ */
+ if ( pfn >> (PTE_PER_TABLE_SHIFT * level) )
+ return 0;
+
next_table_mfn = page_to_mfn(table);
if ( level == IOMMU_PAGING_MODE_LEVEL_1 )
@@ -530,6 +537,9 @@ static int iommu_pde_from_gfn(struct dom
/* Install lower level page table for non-present entries */
else if ( !iommu_is_pte_present((u32*)pde) )
{
+ if ( !map )
+ return 0;
+
if ( next_table_mfn == 0 )
{
table = alloc_amd_iommu_pgtable();
@@ -688,7 +698,7 @@ int amd_iommu_map_page(struct domain *d,
}
}
- if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
+ if ( iommu_pde_from_gfn(d, gfn, pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
@@ -767,23 +777,7 @@ int amd_iommu_unmap_page(struct domain *
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) )
- {
- 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);
- if ( rc != -EADDRNOTAVAIL )
- domain_crash(d);
- return rc;
- }
- }
-
- if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
+ if ( iommu_pde_from_gfn(d, gfn, pt_mfn, false) )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
@@ -791,8 +785,11 @@ int amd_iommu_unmap_page(struct domain *
return -EFAULT;
}
- /* mark PTE as 'page not present' */
- clear_iommu_pte_present(pt_mfn[1], gfn);
+ if ( pt_mfn[1] )
+ {
+ /* Mark PTE as 'page not present'. */
+ clear_iommu_pte_present(pt_mfn[1], gfn);
+ }
/* No further merging in amd_iommu_map_page(), as the logic doesn't cope. */
hd->arch.no_merge = true;
["xsa311-4.9-2.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables
update_paging_mode() has multiple bugs:
1) Booting with iommu=debug will cause it to inform you that that it called
without the pdev_list lock held.
2) When growing by more than a single level, it leaks the newly allocated
table(s) in the case of a further error.
Furthermore, the choice of default level for a domain has issues:
1) All HVM guests grow from 2 to 3 levels during construction because of the
position of the VRAM just below the 4G boundary, so defaulting to 2 is a
waste of effort.
2) The limit for PV guests doesn't take memory hotplug into account, and
isn't dynamic at runtime like HVM guests. This means that a PV guest may
get RAM which it can't map in the IOMMU.
The dynamic height is a property unique to AMD, and adds a substantial
quantity of complexity for what is a marginal performance improvement. Remove
the complexity by removing the dynamic height.
PV guests now get 3 or 4 levels based on any hotplug regions in the host.
This only makes a difference for hardware which previously had all RAM below
the 512G boundary, and a hotplug region above.
HVM guests now get 4 levels (which will be sufficient until 256TB guests
become a thing), because we don't currently have the information to know when
3 would be safe to use.
The overhead of this extra level is not expected to be noticeable. It costs
one page (4k) per domain, and one extra IO-TLB paging structure cache entry
which is very hot and less likely to be evicted.
This is XSA-311.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -7574,6 +7574,17 @@ void write_32bit_pse_identmap(uint32_t *
_PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
}
+unsigned long get_upper_mfn_bound(void)
+{
+ unsigned long max_mfn;
+
+ max_mfn = mem_hotplug ? PFN_DOWN(mem_hotplug) : max_page;
+#ifndef CONFIG_BIGMEM
+ max_mfn = min(max_mfn, 1UL << 32);
+#endif
+ return min(max_mfn, 1UL << (paddr_bits - PAGE_SHIFT)) - 1;
+}
+
/*
* Local variables:
* mode: C
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -569,97 +569,6 @@ static int iommu_pde_from_gfn(struct dom
return 0;
}
-static int update_paging_mode(struct domain *d, unsigned long gfn)
-{
- u16 bdf;
- void *device_entry;
- unsigned int req_id, level, offset;
- unsigned long flags;
- struct pci_dev *pdev;
- struct amd_iommu *iommu = NULL;
- struct page_info *new_root = NULL;
- struct page_info *old_root = NULL;
- void *new_root_vaddr;
- unsigned long old_root_mfn;
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( gfn == gfn_x(INVALID_GFN) )
- 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));
-
- ASSERT(spin_is_locked(&hd->arch.mapping_lock) && is_hvm_domain(d));
-
- while ( offset >= PTE_PER_TABLE_SIZE )
- {
- /* Allocate and install a new root table.
- * Only upper I/O page table grows, no need to fix next level bits */
- new_root = alloc_amd_iommu_pgtable();
- if ( new_root == NULL )
- {
- AMD_IOMMU_DEBUG("%s Cannot allocate I/O page table\n",
- __func__);
- return -ENOMEM;
- }
-
- new_root_vaddr = __map_domain_page(new_root);
- old_root_mfn = page_to_mfn(old_root);
- set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
- !!IOMMUF_writable, !!IOMMUF_readable);
- level++;
- old_root = new_root;
- offset >>= PTE_PER_TABLE_SHIFT;
- unmap_domain_page(new_root_vaddr);
- }
-
- if ( new_root != NULL )
- {
- hd->arch.paging_mode = level;
- hd->arch.root_table = new_root;
-
- if ( !pcidevs_locked() )
- AMD_IOMMU_DEBUG("%s Try to access pdev_list "
- "without aquiring pcidevs_lock.\n", __func__);
-
- /* Update device table entries using new root table and paging mode */
- for_each_pdev( d, pdev )
- {
- bdf = PCI_BDF2(pdev->bus, pdev->devfn);
- iommu = find_iommu_for_device(pdev->seg, bdf);
- if ( !iommu )
- {
- AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
- return -ENODEV;
- }
-
- spin_lock_irqsave(&iommu->lock, flags);
- do {
- req_id = get_dma_requestor_id(pdev->seg, bdf);
- device_entry = iommu->dev_table.buffer +
- (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
-
- /* valid = 0 only works for dom0 passthrough mode */
- amd_iommu_set_root_page_table((u32 *)device_entry,
- page_to_maddr(hd->arch.root_table),
- d->domain_id,
- hd->arch.paging_mode, 1);
-
- amd_iommu_flush_device(iommu, req_id);
- bdf += pdev->phantom_stride;
- } while ( PCI_DEVFN2(bdf) != pdev->devfn &&
- PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
- spin_unlock_irqrestore(&iommu->lock, flags);
- }
-
- /* For safety, invalidate all entries */
- amd_iommu_flush_all_pages(d);
- }
- return 0;
-}
-
int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
@@ -685,19 +594,6 @@ int amd_iommu_map_page(struct domain *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) )
- {
- if ( update_paging_mode(d, gfn) )
- {
- spin_unlock(&hd->arch.mapping_lock);
- AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
- domain_crash(d);
- return -EFAULT;
- }
- }
-
if ( iommu_pde_from_gfn(d, gfn, pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -269,11 +269,17 @@ static int amd_iommu_domain_init(struct
{
struct domain_iommu *hd = dom_iommu(d);
- /* 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 = is_hvm_domain(d) ?
- IOMMU_PAGING_MODE_LEVEL_2 :
- get_paging_mode(max_page);
+ /*
+ * Choose the number of levels for the IOMMU page tables.
+ * - PV needs 3 or 4, depending on whether there is RAM (including hotplug
+ * RAM) above the 512G boundary.
+ * - HVM could in principle use 3 or 4 depending on how much guest
+ * physical address space we give it, but this isn't known yet so use 4
+ * unilaterally.
+ */
+ hd->arch.paging_mode = is_hvm_domain(d)
+ ? IOMMU_PAGING_MODE_LEVEL_4 : get_paging_mode(get_upper_mfn_bound());
+
return 0;
}
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -577,6 +577,9 @@ int prepare_ring_for_helper(struct domai
struct page_info **_page, void **_va);
void destroy_ring_for_helper(void **_va, struct page_info *page);
+/* Return the upper bound of MFNs, including hotplug memory. */
+unsigned long get_upper_mfn_bound(void);
+
#include <asm/flushtlb.h>
static inline void accumulate_tlbflush(bool *need_tlbflush,
["xsa311-4.10-1.patch" (application/octet-stream)]
From: Jan Beulich <jbeulich@suse.com>
Subject: AMD/IOMMU: don't needlessly trigger errors/crashes when unmapping a page
Unmapping a page which has never been mapped should be a no-op (note how
it already is in case there was no root page table allocated). There's
in particular no need to grow the number of page table levels in use,
and there's also no need to allocate intermediate page tables except
when needing to split a large page.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -456,7 +456,7 @@ static int iommu_merge_pages(struct doma
* page tables.
*/
static int iommu_pde_from_gfn(struct domain *d, unsigned long pfn,
- unsigned long pt_mfn[])
+ unsigned long pt_mfn[], bool map)
{
u64 *pde, *next_table_vaddr;
unsigned long next_table_mfn;
@@ -470,6 +470,13 @@ static int iommu_pde_from_gfn(struct dom
BUG_ON( table == NULL || level < IOMMU_PAGING_MODE_LEVEL_1 ||
level > IOMMU_PAGING_MODE_LEVEL_6 );
+ /*
+ * A frame number past what the current page tables can represent can't
+ * possibly have a mapping.
+ */
+ if ( pfn >> (PTE_PER_TABLE_SHIFT * level) )
+ return 0;
+
next_table_mfn = page_to_mfn(table);
if ( level == IOMMU_PAGING_MODE_LEVEL_1 )
@@ -530,6 +537,9 @@ static int iommu_pde_from_gfn(struct dom
/* Install lower level page table for non-present entries */
else if ( !iommu_is_pte_present((u32*)pde) )
{
+ if ( !map )
+ return 0;
+
if ( next_table_mfn == 0 )
{
table = alloc_amd_iommu_pgtable();
@@ -688,7 +698,7 @@ int amd_iommu_map_page(struct domain *d,
}
}
- if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
+ if ( iommu_pde_from_gfn(d, gfn, pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
@@ -767,23 +777,7 @@ int amd_iommu_unmap_page(struct domain *
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) )
- {
- 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);
- if ( rc != -EADDRNOTAVAIL )
- domain_crash(d);
- return rc;
- }
- }
-
- if ( iommu_pde_from_gfn(d, gfn, pt_mfn) || (pt_mfn[1] == 0) )
+ if ( iommu_pde_from_gfn(d, gfn, pt_mfn, false) )
{
spin_unlock(&hd->arch.mapping_lock);
AMD_IOMMU_DEBUG("Invalid IO pagetable entry gfn = %lx\n", gfn);
@@ -791,8 +785,11 @@ int amd_iommu_unmap_page(struct domain *
return -EFAULT;
}
- /* mark PTE as 'page not present' */
- clear_iommu_pte_present(pt_mfn[1], gfn);
+ if ( pt_mfn[1] )
+ {
+ /* Mark PTE as 'page not present'. */
+ clear_iommu_pte_present(pt_mfn[1], gfn);
+ }
/* No further merging in amd_iommu_map_page(), as the logic doesn't cope. */
hd->arch.no_merge = true;
["xsa311-4.10-2.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables
update_paging_mode() has multiple bugs:
1) Booting with iommu=debug will cause it to inform you that that it called
without the pdev_list lock held.
2) When growing by more than a single level, it leaks the newly allocated
table(s) in the case of a further error.
Furthermore, the choice of default level for a domain has issues:
1) All HVM guests grow from 2 to 3 levels during construction because of the
position of the VRAM just below the 4G boundary, so defaulting to 2 is a
waste of effort.
2) The limit for PV guests doesn't take memory hotplug into account, and
isn't dynamic at runtime like HVM guests. This means that a PV guest may
get RAM which it can't map in the IOMMU.
The dynamic height is a property unique to AMD, and adds a substantial
quantity of complexity for what is a marginal performance improvement. Remove
the complexity by removing the dynamic height.
PV guests now get 3 or 4 levels based on any hotplug regions in the host.
This only makes a difference for hardware which previously had all RAM below
the 512G boundary, and a hotplug region above.
HVM guests now get 4 levels (which will be sufficient until 256TB guests
become a thing), because we don't currently have the information to know when
3 would be safe to use.
The overhead of this extra level is not expected to be noticeable. It costs
one page (4k) per domain, and one extra IO-TLB paging structure cache entry
which is very hot and less likely to be evicted.
This is XSA-311.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -569,97 +569,6 @@ static int iommu_pde_from_gfn(struct dom
return 0;
}
-static int update_paging_mode(struct domain *d, unsigned long gfn)
-{
- u16 bdf;
- void *device_entry;
- unsigned int req_id, level, offset;
- unsigned long flags;
- struct pci_dev *pdev;
- struct amd_iommu *iommu = NULL;
- struct page_info *new_root = NULL;
- struct page_info *old_root = NULL;
- void *new_root_vaddr;
- unsigned long old_root_mfn;
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( gfn == gfn_x(INVALID_GFN) )
- 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));
-
- ASSERT(spin_is_locked(&hd->arch.mapping_lock) && is_hvm_domain(d));
-
- while ( offset >= PTE_PER_TABLE_SIZE )
- {
- /* Allocate and install a new root table.
- * Only upper I/O page table grows, no need to fix next level bits */
- new_root = alloc_amd_iommu_pgtable();
- if ( new_root == NULL )
- {
- AMD_IOMMU_DEBUG("%s Cannot allocate I/O page table\n",
- __func__);
- return -ENOMEM;
- }
-
- new_root_vaddr = __map_domain_page(new_root);
- old_root_mfn = page_to_mfn(old_root);
- set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
- !!IOMMUF_writable, !!IOMMUF_readable);
- level++;
- old_root = new_root;
- offset >>= PTE_PER_TABLE_SHIFT;
- unmap_domain_page(new_root_vaddr);
- }
-
- if ( new_root != NULL )
- {
- hd->arch.paging_mode = level;
- hd->arch.root_table = new_root;
-
- if ( !pcidevs_locked() )
- AMD_IOMMU_DEBUG("%s Try to access pdev_list "
- "without aquiring pcidevs_lock.\n", __func__);
-
- /* Update device table entries using new root table and paging mode */
- for_each_pdev( d, pdev )
- {
- bdf = PCI_BDF2(pdev->bus, pdev->devfn);
- iommu = find_iommu_for_device(pdev->seg, bdf);
- if ( !iommu )
- {
- AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
- return -ENODEV;
- }
-
- spin_lock_irqsave(&iommu->lock, flags);
- do {
- req_id = get_dma_requestor_id(pdev->seg, bdf);
- device_entry = iommu->dev_table.buffer +
- (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
-
- /* valid = 0 only works for dom0 passthrough mode */
- amd_iommu_set_root_page_table((u32 *)device_entry,
- page_to_maddr(hd->arch.root_table),
- d->domain_id,
- hd->arch.paging_mode, 1);
-
- amd_iommu_flush_device(iommu, req_id);
- bdf += pdev->phantom_stride;
- } while ( PCI_DEVFN2(bdf) != pdev->devfn &&
- PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
- spin_unlock_irqrestore(&iommu->lock, flags);
- }
-
- /* For safety, invalidate all entries */
- amd_iommu_flush_all_pages(d);
- }
- return 0;
-}
-
int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
@@ -685,19 +594,6 @@ int amd_iommu_map_page(struct domain *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) )
- {
- if ( update_paging_mode(d, gfn) )
- {
- spin_unlock(&hd->arch.mapping_lock);
- AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
- domain_crash(d);
- return -EFAULT;
- }
- }
-
if ( iommu_pde_from_gfn(d, gfn, pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -242,11 +242,17 @@ static int amd_iommu_domain_init(struct
{
struct domain_iommu *hd = dom_iommu(d);
- /* 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 = is_hvm_domain(d) ?
- IOMMU_PAGING_MODE_LEVEL_2 :
- get_paging_mode(max_page);
+ /*
+ * Choose the number of levels for the IOMMU page tables.
+ * - PV needs 3 or 4, depending on whether there is RAM (including hotplug
+ * RAM) above the 512G boundary.
+ * - HVM could in principle use 3 or 4 depending on how much guest
+ * physical address space we give it, but this isn't known yet so use 4
+ * unilaterally.
+ */
+ hd->arch.paging_mode = is_hvm_domain(d)
+ ? IOMMU_PAGING_MODE_LEVEL_4 : get_paging_mode(get_upper_mfn_bound());
+
return 0;
}
["xsa311-4.11.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables
update_paging_mode() has multiple bugs:
1) Booting with iommu=debug will cause it to inform you that that it called
without the pdev_list lock held.
2) When growing by more than a single level, it leaks the newly allocated
table(s) in the case of a further error.
Furthermore, the choice of default level for a domain has issues:
1) All HVM guests grow from 2 to 3 levels during construction because of the
position of the VRAM just below the 4G boundary, so defaulting to 2 is a
waste of effort.
2) The limit for PV guests doesn't take memory hotplug into account, and
isn't dynamic at runtime like HVM guests. This means that a PV guest may
get RAM which it can't map in the IOMMU.
The dynamic height is a property unique to AMD, and adds a substantial
quantity of complexity for what is a marginal performance improvement. Remove
the complexity by removing the dynamic height.
PV guests now get 3 or 4 levels based on any hotplug regions in the host.
This only makes a difference for hardware which previously had all RAM below
the 512G boundary, and a hotplug region above.
HVM guests now get 4 levels (which will be sufficient until 256TB guests
become a thing), because we don't currently have the information to know when
3 would be safe to use.
The overhead of this extra level is not expected to be noticeable. It costs
one page (4k) per domain, and one extra IO-TLB paging structure cache entry
which is very hot and less likely to be evicted.
This is XSA-311.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -569,97 +569,6 @@ static int iommu_pde_from_gfn(struct dom
return 0;
}
-static int update_paging_mode(struct domain *d, unsigned long gfn)
-{
- u16 bdf;
- void *device_entry;
- unsigned int req_id, level, offset;
- unsigned long flags;
- struct pci_dev *pdev;
- struct amd_iommu *iommu = NULL;
- struct page_info *new_root = NULL;
- struct page_info *old_root = NULL;
- void *new_root_vaddr;
- unsigned long old_root_mfn;
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( gfn == gfn_x(INVALID_GFN) )
- 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));
-
- ASSERT(spin_is_locked(&hd->arch.mapping_lock) && is_hvm_domain(d));
-
- while ( offset >= PTE_PER_TABLE_SIZE )
- {
- /* Allocate and install a new root table.
- * Only upper I/O page table grows, no need to fix next level bits */
- new_root = alloc_amd_iommu_pgtable();
- if ( new_root == NULL )
- {
- AMD_IOMMU_DEBUG("%s Cannot allocate I/O page table\n",
- __func__);
- return -ENOMEM;
- }
-
- new_root_vaddr = __map_domain_page(new_root);
- old_root_mfn = mfn_x(page_to_mfn(old_root));
- set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
- !!IOMMUF_writable, !!IOMMUF_readable);
- level++;
- old_root = new_root;
- offset >>= PTE_PER_TABLE_SHIFT;
- unmap_domain_page(new_root_vaddr);
- }
-
- if ( new_root != NULL )
- {
- hd->arch.paging_mode = level;
- hd->arch.root_table = new_root;
-
- if ( !pcidevs_locked() )
- AMD_IOMMU_DEBUG("%s Try to access pdev_list "
- "without aquiring pcidevs_lock.\n", __func__);
-
- /* Update device table entries using new root table and paging mode */
- for_each_pdev( d, pdev )
- {
- bdf = PCI_BDF2(pdev->bus, pdev->devfn);
- iommu = find_iommu_for_device(pdev->seg, bdf);
- if ( !iommu )
- {
- AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
- return -ENODEV;
- }
-
- spin_lock_irqsave(&iommu->lock, flags);
- do {
- req_id = get_dma_requestor_id(pdev->seg, bdf);
- device_entry = iommu->dev_table.buffer +
- (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
-
- /* valid = 0 only works for dom0 passthrough mode */
- amd_iommu_set_root_page_table((u32 *)device_entry,
- page_to_maddr(hd->arch.root_table),
- d->domain_id,
- hd->arch.paging_mode, 1);
-
- amd_iommu_flush_device(iommu, req_id);
- bdf += pdev->phantom_stride;
- } while ( PCI_DEVFN2(bdf) != pdev->devfn &&
- PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
- spin_unlock_irqrestore(&iommu->lock, flags);
- }
-
- /* For safety, invalidate all entries */
- amd_iommu_flush_all_pages(d);
- }
- return 0;
-}
-
int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
@@ -685,19 +594,6 @@ int amd_iommu_map_page(struct domain *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) )
- {
- if ( update_paging_mode(d, gfn) )
- {
- spin_unlock(&hd->arch.mapping_lock);
- AMD_IOMMU_DEBUG("Update page mode failed gfn = %lx\n", gfn);
- domain_crash(d);
- return -EFAULT;
- }
- }
-
if ( iommu_pde_from_gfn(d, gfn, pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -242,11 +242,17 @@ static int amd_iommu_domain_init(struct
{
struct domain_iommu *hd = dom_iommu(d);
- /* 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 = is_hvm_domain(d) ?
- IOMMU_PAGING_MODE_LEVEL_2 :
- get_paging_mode(max_page);
+ /*
+ * Choose the number of levels for the IOMMU page tables.
+ * - PV needs 3 or 4, depending on whether there is RAM (including hotplug
+ * RAM) above the 512G boundary.
+ * - HVM could in principle use 3 or 4 depending on how much guest
+ * physical address space we give it, but this isn't known yet so use 4
+ * unilaterally.
+ */
+ hd->arch.paging_mode = is_hvm_domain(d)
+ ? IOMMU_PAGING_MODE_LEVEL_4 : get_paging_mode(get_upper_mfn_bound());
+
return 0;
}
["xsa311-4.12.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: AMD/IOMMU: Cease using a dynamic height for the IOMMU pagetables
update_paging_mode() has multiple bugs:
1) Booting with iommu=debug will cause it to inform you that that it called
without the pdev_list lock held.
2) When growing by more than a single level, it leaks the newly allocated
table(s) in the case of a further error.
Furthermore, the choice of default level for a domain has issues:
1) All HVM guests grow from 2 to 3 levels during construction because of the
position of the VRAM just below the 4G boundary, so defaulting to 2 is a
waste of effort.
2) The limit for PV guests doesn't take memory hotplug into account, and
isn't dynamic at runtime like HVM guests. This means that a PV guest may
get RAM which it can't map in the IOMMU.
The dynamic height is a property unique to AMD, and adds a substantial
quantity of complexity for what is a marginal performance improvement. Remove
the complexity by removing the dynamic height.
PV guests now get 3 or 4 levels based on any hotplug regions in the host.
This only makes a difference for hardware which previously had all RAM below
the 512G boundary, and a hotplug region above.
HVM guests now get 4 levels (which will be sufficient until 256TB guests
become a thing), because we don't currently have the information to know when
3 would be safe to use.
The overhead of this extra level is not expected to be noticeable. It costs
one page (4k) per domain, and one extra IO-TLB paging structure cache entry
which is very hot and less likely to be evicted.
This is XSA-311.
Reported-by: XXX PERSON <XXX EMAIL>3
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 628aa60230..9a222c95e1 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -455,100 +455,6 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
return 0;
}
-static int update_paging_mode(struct domain *d, unsigned long dfn)
-{
- uint16_t bdf;
- void *device_entry;
- unsigned int req_id, level, offset;
- unsigned long flags;
- struct pci_dev *pdev;
- struct amd_iommu *iommu = NULL;
- struct page_info *new_root = NULL;
- struct page_info *old_root = NULL;
- void *new_root_vaddr;
- unsigned long old_root_mfn;
- struct domain_iommu *hd = dom_iommu(d);
-
- if ( dfn == dfn_x(INVALID_DFN) )
- return -EADDRNOTAVAIL;
- ASSERT(!(dfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
-
- level = hd->arch.paging_mode;
- old_root = hd->arch.root_table;
- offset = dfn >> (PTE_PER_TABLE_SHIFT * (level - 1));
-
- ASSERT(spin_is_locked(&hd->arch.mapping_lock) && is_hvm_domain(d));
-
- while ( offset >= PTE_PER_TABLE_SIZE )
- {
- /* Allocate and install a new root table.
- * Only upper I/O page table grows, no need to fix next level bits */
- new_root = alloc_amd_iommu_pgtable();
- if ( new_root == NULL )
- {
- AMD_IOMMU_DEBUG("%s Cannot allocate I/O page table\n",
- __func__);
- return -ENOMEM;
- }
-
- new_root_vaddr = __map_domain_page(new_root);
- old_root_mfn = mfn_x(page_to_mfn(old_root));
- set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
- !!IOMMUF_writable, !!IOMMUF_readable);
- level++;
- old_root = new_root;
- offset >>= PTE_PER_TABLE_SHIFT;
- unmap_domain_page(new_root_vaddr);
- }
-
- if ( new_root != NULL )
- {
- hd->arch.paging_mode = level;
- hd->arch.root_table = new_root;
-
- if ( !pcidevs_locked() )
- AMD_IOMMU_DEBUG("%s Try to access pdev_list "
- "without aquiring pcidevs_lock.\n", __func__);
-
- /* Update device table entries using new root table and paging mode */
- for_each_pdev( d, pdev )
- {
- if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE )
- continue;
-
- bdf = PCI_BDF2(pdev->bus, pdev->devfn);
- iommu = find_iommu_for_device(pdev->seg, bdf);
- if ( !iommu )
- {
- AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
- return -ENODEV;
- }
-
- spin_lock_irqsave(&iommu->lock, flags);
- do {
- req_id = get_dma_requestor_id(pdev->seg, bdf);
- device_entry = iommu->dev_table.buffer +
- (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
-
- /* valid = 0 only works for dom0 passthrough mode */
- amd_iommu_set_root_page_table((uint32_t *)device_entry,
- page_to_maddr(hd->arch.root_table),
- d->domain_id,
- hd->arch.paging_mode, 1);
-
- amd_iommu_flush_device(iommu, req_id);
- bdf += pdev->phantom_stride;
- } while ( PCI_DEVFN2(bdf) != pdev->devfn &&
- PCI_SLOT(bdf) == PCI_SLOT(pdev->devfn) );
- spin_unlock_irqrestore(&iommu->lock, flags);
- }
-
- /* For safety, invalidate all entries */
- amd_iommu_flush_all_pages(d);
- }
- return 0;
-}
-
int amd_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
unsigned int flags, unsigned int *flush_flags)
{
@@ -573,20 +479,6 @@ int amd_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
return rc;
}
- /* Since HVM domain is initialized with 2 level IO page table,
- * we might need a deeper page table for wider dfn now */
- if ( is_hvm_domain(d) )
- {
- if ( update_paging_mode(d, dfn_x(dfn)) )
- {
- spin_unlock(&hd->arch.mapping_lock);
- AMD_IOMMU_DEBUG("Update page mode failed dfn = %"PRI_dfn"\n",
- dfn_x(dfn));
- domain_crash(d);
- return -EFAULT;
- }
- }
-
if ( iommu_pde_from_dfn(d, dfn_x(dfn), pt_mfn, true) || (pt_mfn[1] == 0) )
{
spin_unlock(&hd->arch.mapping_lock);
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 15c13e1163..57dc2c5f20 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -242,10 +242,17 @@ static int amd_iommu_domain_init(struct domain *d)
{
struct domain_iommu *hd = dom_iommu(d);
- /* 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 = is_hvm_domain(d) ?
- 2 : amd_iommu_get_paging_mode(max_page);
+ /*
+ * Choose the number of levels for the IOMMU page tables.
+ * - PV needs 3 or 4, depending on whether there is RAM (including hotplug
+ * RAM) above the 512G boundary.
+ * - HVM could in principle use 3 or 4 depending on how much guest
+ * physical address space we give it, but this isn't known yet so use 4
+ * unilaterally.
+ */
+ hd->arch.paging_mode = is_hvm_domain(d)
+ ? 4 : amd_iommu_get_paging_mode(get_upper_mfn_bound());
+
return 0;
}
[Attachment #13 (text/plain)]
_______________________________________________
Xen-users mailing list
Xen-users@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-users
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic