Xen Security Advisory 373 v2 (CVE-2021-28692) - inappropriate x86 IOMMU timeout detection / handling
Xen.org security team <[email protected]> Tue, 08 Jun 2021 17:04:29 +0000
| Newsgroups | gmane.comp.emulators.xen.announce,gmane.comp.security.oss.general |
|---|---|
| Message-ID | <[email protected]> |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2021-28692 / XSA-373
version 2
inappropriate x86 IOMMU timeout detection / handling
UPDATES IN VERSION 2
====================
Public release.
ISSUE DESCRIPTION
=================
IOMMUs process commands issued to them in parallel with the operation
of the CPU(s) issuing such commands. In the current implementation in
Xen, asynchronous notification of the completion of such commands is
not used. Instead, the issuing CPU spin-waits for the completion of
the most recently issued command(s). Some of these waiting loops try
to apply a timeout to fail overly-slow commands. The course of action
upon a perceived timeout actually being detected is inappropriate:
- on Intel hardware guests which did not originally cause the timeout
may be marked as crashed,
- on AMD hardware higher layer callers would not be notified of the
issue, making them continue as if the IOMMU operation succeeded.
IMPACT
======
A malicious guest may be able to elevate its privileges to that of the
host, cause host or guest Denial of Service (DoS), or cause information
leaks.
VULNERABLE SYSTEMS
==================
All Xen versions from at least 3.2 onwards are vulnerable. Earlier
versions have not been inspected.
Only x86 systems with in-use IOMMU hardware are vulnerable. x86 systems
without any IOMMUs in use are not vulnerable. On Arm systems IOMMU /
SMMU use is not security supported.
Only x86 guests which have physical devices passed through to them can
leverage the vulnerability.
MITIGATION
==========
Not passing through physical devices to untrusted guests will avoid
the vulnerability.
CREDITS
=======
This issue was discovered by Igor Druzhinin and Andrew Cooper of Citrix,
and further issues were uncovered by by Jan Beulich of SUSE while trying
to fix the first issue.
RESOLUTION
==========
Applying the appropriate set of attached patches resolves this issue.
Note that patches for released versions are generally prepared to
apply to the stable branches, and may not apply cleanly to the most
recent release tarball. Downstreams are encouraged to update to the
tip of the stable branch before applying these patches.
xsa373/xsa373-?.patch xen-unstable
xsa373/xsa373-4.15-?.patch Xen 4.15.x
xsa373/xsa373-4.14-?.patch Xen 4.14.x
xsa373/xsa373-4.13-?.patch Xen 4.13.x
xsa373/xsa373-4.12-?.patch Xen 4.12.x
xsa373/xsa373-4.11-?.patch Xen 4.11.x
$ sha256sum xsa373* xsa373*/*
2ded01092088735e0d8a0e378a41b772ec0f17ceb7afabc78228670c43407fc2 xsa373.meta
f62df56cd176237521aa2ed4a22b0e893318b85bb0ce3c17bd7fca5282b6105b xsa373/xsa373-1.patch
9eed9566508e116c4da6c201b36fe7e53e98f2daf96cce8ed0a9ca192d783edc xsa373/xsa373-2.patch
ffee9d17e40798c053a67707dd13d7a944e4a53de7bcfe3e146eac7871ca2608 xsa373/xsa373-3.patch
c51bea462222c090ae671f14471ece00724348e6c04e5850f9b91d0b1eceaad8 xsa373/xsa373-4.11-1.patch
9a3b331e404a38c72ec154cefd78f1f67db6f25dcc1bd554b37ff50899ea42ff xsa373/xsa373-4.11-2.patch
dba77bce4e6c88ec43df61e88bd5c8bee6e32c0ff681cbeddc4bceb0ee6c73dd xsa373/xsa373-4.11-3.patch
b1f14e8885e3004de79c5012a1d9278d7a0c39633c5b73cbfda28679f1722c38 xsa373/xsa373-4.11-4.patch
791bccec1e7ba4429a0bafef5fd5a35a68562cee333d0962c70477172493ef3b xsa373/xsa373-4.11-5.patch
cc4e1bcef148dbfc94ada92bef4408c5516cff2cf249e43c5595b1dbffbbc1e4 xsa373/xsa373-4.12-1.patch
12ffdac1526d96c4f1b572360a7f1a0371e8a177cf15228b126c1032de4e8930 xsa373/xsa373-4.12-2.patch
619425ba44f449bf7b0f519040ee579adff0d0293a95e9b0f70c943c02ae22fb xsa373/xsa373-4.12-3.patch
b1f14e8885e3004de79c5012a1d9278d7a0c39633c5b73cbfda28679f1722c38 xsa373/xsa373-4.12-4.patch
96b3dd11d38ca8ca0b2dfe2dfb571045fcda78dbfe416580c9b04c5a8ce5fcef xsa373/xsa373-4.12-5.patch
4add1d05ad2780904ebc89b4d1a93a8f2757b6e9f45b075afce46392ae406b58 xsa373/xsa373-4.13-1.patch
b064324db709078b8ef479df0c31ff3391a506755bfb0186d7d165592d025357 xsa373/xsa373-4.13-2.patch
6fe47fbba0c9d86f48643182d8a7c64ff70a7c8b290b0e93afe1d43d04bed480 xsa373/xsa373-4.13-3.patch
b1f14e8885e3004de79c5012a1d9278d7a0c39633c5b73cbfda28679f1722c38 xsa373/xsa373-4.13-4.patch
96b3dd11d38ca8ca0b2dfe2dfb571045fcda78dbfe416580c9b04c5a8ce5fcef xsa373/xsa373-4.13-5.patch
4add1d05ad2780904ebc89b4d1a93a8f2757b6e9f45b075afce46392ae406b58 xsa373/xsa373-4.14-1.patch
8e61b7dda9ea21a830454e629fd23e3379b73fb230bd04107618e45975e117d1 xsa373/xsa373-4.14-2.patch
a5aa80d8e893c268f171a5e429bfef0c553522f860e3e5132b4bd87d3a73c6b7 xsa373/xsa373-4.14-3.patch
25bfd2b821ae2cc867b8e2d480528ebd435da76cfab766e8106573cf8dc6f36c xsa373/xsa373-4.14-4.patch
162b3f14d15fe5ca2cb659efad6635f3803dde6fa97a6f0f1f7f202d3ea72d94 xsa373/xsa373-4.14-5.patch
4add1d05ad2780904ebc89b4d1a93a8f2757b6e9f45b075afce46392ae406b58 xsa373/xsa373-4.15-1.patch
9eed9566508e116c4da6c201b36fe7e53e98f2daf96cce8ed0a9ca192d783edc xsa373/xsa373-4.15-2.patch
13642541b056ed47129d8143a919bcc81a73797baedc3bd90afeb33f021e6d31 xsa373/xsa373-4.15-3.patch
b2517a7e92c26a818e94ed5133d5aef6ef1d3a7a98f2f5355f1ad6f30baa3ab9 xsa373/xsa373-4.15-4.patch
3ca056796b93cb07ddb7e1dfda98410162382fc56135eb08bc5ff19137d8c427 xsa373/xsa373-4.15-5.patch
b2517a7e92c26a818e94ed5133d5aef6ef1d3a7a98f2f5355f1ad6f30baa3ab9 xsa373/xsa373-4.patch
0b7bb146330f7fdc7c8c331a618307819073654a13d9fe1d0a8b83ab037ae802 xsa373/xsa373-5.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/4UyVfoK9kFAmC/oxIMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZ7oQH/39iA05B0xCxHjYxZJmwplLhtr/RwNt+3zOgsesg
jaG8KMWRobWsfLWpbQdEuWKLQ5kPcK47KBGdFkadbSgNW6ZKeG6iR+HWC04/9uA6
3jjlhyqcdetfGnRUh/EO+4gLEaWxdWegWLWMBqYYp+f9b9lKDp8vyWj5yfzU1FFF
+YOu4bSRnqbY21hapsy2iupbBJugJF1vCLVfMLxQjba8KOjl4bk6cIxx/WgX3FPI
XIH6T+0MtLioCbv7MFaSlfeWoMNjpcimMA8/dmePS6XBtjGX02ahEYSO66lHKk7T
BsrN4QLibAsb8vMb5KjcjGE8ukhrg3AH5EOE950duWF5heQ=
=fAD/
-----END PGP SIGNATURE-----
xsa373.meta
(application/octet-stream, 1.7 KB) - not displayed
xsa373/xsa373-1.patch
(application/octet-stream, 4.7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: size qinval queue dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait descriptor). There can be one such pair of requests pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the queue ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -452,17 +452,9 @@ struct qinval_entry { }q; }; -/* Order of queue invalidation pages(max is 8) */ -#define QINVAL_PAGE_ORDER 2 - -#define QINVAL_ARCH_PAGE_ORDER (QINVAL_PAGE_ORDER + PAGE_SHIFT_4K - PAGE_SHIFT) -#define QINVAL_ARCH_PAGE_NR ( QINVAL_ARCH_PAGE_ORDER < 0 ? \ - 1 : \ - 1 << QINVAL_ARCH_PAGE_ORDER ) - /* Each entry is 16 bytes, so 2^8 entries per page */ #define QINVAL_ENTRY_ORDER ( PAGE_SHIFT - 4 ) -#define QINVAL_ENTRY_NR (1 << (QINVAL_PAGE_ORDER + 8)) +#define QINVAL_MAX_ENTRY_NR (1u << (7 + QINVAL_ENTRY_ORDER)) /* Status data flag */ #define QINVAL_STAT_INIT 0 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -31,6 +31,9 @@ #define VTD_QI_TIMEOUT 1 +static unsigned int __read_mostly qi_pg_order; +static unsigned int __read_mostly qi_entry_nr; + static int __must_check invalidate_sync(struct vtd_iommu *iommu); static void print_qi_regs(const struct vtd_iommu *iommu) @@ -47,7 +50,7 @@ static unsigned int qinval_next_index(st tail >>= QINVAL_INDEX_SHIFT; /* (tail+1 == head) indicates a full queue, wait for HW */ - while ( (tail + 1) % QINVAL_ENTRY_NR == + while ( ((tail + 1) & (qi_entry_nr - 1)) == (dmar_readl(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT) ) cpu_relax(); @@ -60,7 +63,7 @@ static void qinval_update_qtail(struct v /* Need hold register lock when update tail */ ASSERT( spin_is_locked(&iommu->register_lock) ); - val = (index + 1) % QINVAL_ENTRY_NR; + val = (index + 1) & (qi_entry_nr - 1); dmar_writel(iommu->reg, DMAR_IQT_REG, val << QINVAL_INDEX_SHIFT); } @@ -395,8 +398,28 @@ int enable_qinval(struct vtd_iommu *iomm if ( iommu->qinval_maddr == 0 ) { - iommu->qinval_maddr = alloc_pgtable_maddr(QINVAL_ARCH_PAGE_NR, - iommu->node); + if ( !qi_entry_nr ) + { + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait descriptor). There + * can be one such pair of requests pending per CPU. One extra + * entry is needed as the ring is considered full when there's + * only one entry left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= QINVAL_MAX_ENTRY_NR); + qi_pg_order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + (PAGE_SHIFT - + QINVAL_ENTRY_ORDER)); + qi_entry_nr = 1u << (qi_pg_order + QINVAL_ENTRY_ORDER); + + dprintk(XENLOG_INFO VTDPREFIX, + "QI: using %u-entry ring(s)\n", qi_entry_nr); + } + + iommu->qinval_maddr = + alloc_pgtable_maddr(qi_entry_nr >> QINVAL_ENTRY_ORDER, + iommu->node); if ( iommu->qinval_maddr == 0 ) { dprintk(XENLOG_WARNING VTDPREFIX, @@ -410,15 +433,16 @@ int enable_qinval(struct vtd_iommu *iomm spin_lock_irqsave(&iommu->register_lock, flags); - /* Setup Invalidation Queue Address(IQA) register with the - * address of the page we just allocated. QS field at - * bits[2:0] to indicate size of queue is one 4KB page. - * That's 256 entries. Queued Head (IQH) and Queue Tail (IQT) - * registers are automatically reset to 0 with write - * to IQA register. + /* + * Setup Invalidation Queue Address (IQA) register with the address of the + * pages we just allocated. The QS field at bits[2:0] indicates the size + * (page order) of the queue. + * + * Queued Head (IQH) and Queue Tail (IQT) registers are automatically + * reset to 0 with write to IQA register. */ dmar_writeq(iommu->reg, DMAR_IQA_REG, - iommu->qinval_maddr | QINVAL_PAGE_ORDER); + iommu->qinval_maddr | qi_pg_order); dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
xsa373/xsa373-2.patch
(application/octet-stream, 3.9 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: size command buffer dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait command). There can be one such pair of commands pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the command ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu-defs.h +++ b/xen/drivers/passthrough/amd/iommu-defs.h @@ -20,9 +20,6 @@ #ifndef AMD_IOMMU_DEFS_H #define AMD_IOMMU_DEFS_H -/* IOMMU Command Buffer entries: in power of 2 increments, minimum of 256 */ -#define IOMMU_CMD_BUFFER_DEFAULT_ENTRIES 512 - /* IOMMU Event Log entries: in power of 2 increments, minimum of 256 */ #define IOMMU_EVENT_LOG_DEFAULT_ENTRIES 512 @@ -164,8 +161,8 @@ struct amd_iommu_dte { #define IOMMU_CMD_BUFFER_LENGTH_MASK 0x0F000000 #define IOMMU_CMD_BUFFER_LENGTH_SHIFT 24 -#define IOMMU_CMD_BUFFER_ENTRY_SIZE 16 -#define IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE 8 +#define IOMMU_CMD_BUFFER_ENTRY_ORDER 4 +#define IOMMU_CMD_BUFFER_MAX_ENTRIES (1u << 15) #define IOMMU_CMD_OPCODE_MASK 0xF0000000 #define IOMMU_CMD_OPCODE_SHIFT 28 --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -24,7 +24,7 @@ static int queue_iommu_command(struct am { uint32_t tail, head; - tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE; + tail = iommu->cmd_buffer.tail + sizeof(cmd_entry_t); if ( tail == iommu->cmd_buffer.size ) tail = 0; @@ -33,7 +33,7 @@ static int queue_iommu_command(struct am if ( head != tail ) { memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, - cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE); + cmd, sizeof(cmd_entry_t)); iommu->cmd_buffer.tail = tail; return 1; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -118,7 +118,7 @@ static void register_iommu_cmd_buffer_in writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET); power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.size) + - IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE; + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER; entry = 0; iommu_set_addr_hi_to_reg(&entry, addr_hi); @@ -1018,9 +1018,31 @@ static void *__init allocate_ring_buffer static void * __init allocate_cmd_buffer(struct amd_iommu *iommu) { /* allocate 'command buffer' in power of 2 increments of 4K */ + static unsigned int __read_mostly nr_ents; + + if ( !nr_ents ) + { + unsigned int order; + + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait command). There can be + * one such pair of requests pending per CPU. One extra entry is + * needed as the ring is considered full when there's only one entry + * left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= IOMMU_CMD_BUFFER_MAX_ENTRIES); + order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + IOMMU_CMD_BUFFER_ENTRY_ORDER); + nr_ents = 1u << (order + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER); + + AMD_IOMMU_DEBUG("using %u-entry cmd ring(s)\n", nr_ents); + } + + BUILD_BUG_ON(sizeof(cmd_entry_t) != (1u << IOMMU_CMD_BUFFER_ENTRY_ORDER)); + return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), - IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, - "Command Buffer", false); + nr_ents, "Command Buffer", false); } static void * __init allocate_event_log(struct amd_iommu *iommu)
xsa373/xsa373-3.patch
(application/octet-stream, 7.3 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: eliminate flush related timeouts Leaving an in-progress operation pending when it appears to take too long is problematic: If e.g. a QI command completed later, the write to the "poll slot" may instead be understood to signal a subsequently started command's completion. Also our accounting of the timeout period was actually wrong: We included the time it took for the command to actually make it to the front of the queue, which could be heavily affected by guests other than the one for which the flush is being performed. Do away with all timeout detection on all flush related code paths. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Additionally log (once) if qinval_next_index() didn't immediately find an available slot. Together with the earlier change sizing the queue(s) dynamically, we should now have a guarantee that with our fully synchronous model any demand for slots can actually be satisfied. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- TBD: In queue_invalidate_wait() we have the option of processing softirqs every once in a while, as there IRQs aren't off while spinning. This would keep the watchdog happy. I'm not sure though whether we aren't better off if it actually triggered in case we spin for a long time. --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,6 +127,34 @@ do { } \ } while (0) +#define IOMMU_FLUSH_WAIT(what, iommu, offset, op, cond, sts) \ +do { \ + static unsigned int __read_mostly threshold = 1; \ + s_time_t start = NOW(); \ + s_time_t timeout = start + DMAR_OPERATION_TIMEOUT * threshold; \ + \ + for ( ; ; ) \ + { \ + sts = op(iommu->reg, offset); \ + if ( cond ) \ + break; \ + if ( timeout && NOW() > timeout ) \ + { \ + threshold |= threshold << 1; \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush taking too long\n", \ + iommu->index, what); \ + timeout = 0; \ + } \ + cpu_relax(); \ + } \ + \ + if ( !timeout ) \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush took %lums\n", \ + iommu->index, what, (NOW() - start) / 10000000); \ +} while ( false ) + int vtd_hw_check(void); void disable_pmr(struct vtd_iommu *iommu); int is_igd_drhd(struct acpi_drhd_unit *drhd); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -373,8 +373,8 @@ static void iommu_flush_write_buffer(str dmar_writel(iommu->reg, DMAR_GCMD_REG, val | DMA_GCMD_WBF); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, dmar_readl, - !(val & DMA_GSTS_WBFS), val); + IOMMU_FLUSH_WAIT("write buffer", iommu, DMAR_GSTS_REG, dmar_readl, + !(val & DMA_GSTS_WBFS), val); spin_unlock_irqrestore(&iommu->register_lock, flags); } @@ -423,8 +423,8 @@ int vtd_flush_context_reg(struct vtd_iom dmar_writeq(iommu->reg, DMAR_CCMD_REG, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_CCMD_REG, dmar_readq, - !(val & DMA_CCMD_ICC), val); + IOMMU_FLUSH_WAIT("context", iommu, DMAR_CCMD_REG, dmar_readq, + !(val & DMA_CCMD_ICC), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* flush context entry will implicitly flush write buffer */ @@ -501,8 +501,8 @@ int vtd_flush_iotlb_reg(struct vtd_iommu dmar_writeq(iommu->reg, tlb_offset + 8, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, (tlb_offset + 8), dmar_readq, - !(val & DMA_TLB_IVT), val); + IOMMU_FLUSH_WAIT("iotlb", iommu, (tlb_offset + 8), dmar_readq, + !(val & DMA_TLB_IVT), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* check IOTLB invalidation granularity */ --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -29,8 +29,6 @@ #include "extern.h" #include "../ats.h" -#define VTD_QI_TIMEOUT 1 - static unsigned int __read_mostly qi_pg_order; static unsigned int __read_mostly qi_entry_nr; @@ -52,7 +50,11 @@ static unsigned int qinval_next_index(st /* (tail+1 == head) indicates a full queue, wait for HW */ while ( ((tail + 1) & (qi_entry_nr - 1)) == (dmar_readl(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT) ) + { + printk_once(XENLOG_ERR VTDPREFIX " IOMMU#%u: no QI slot available\n", + iommu->index); cpu_relax(); + } return tail; } @@ -172,23 +174,32 @@ static int __must_check queue_invalidate /* Now we don't support interrupt method */ if ( sw ) { - s_time_t timeout; - - /* In case all wait descriptor writes to same addr with same data */ - timeout = NOW() + MILLISECS(flush_dev_iotlb ? - iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT); + static unsigned int __read_mostly threshold = 1; + s_time_t start = NOW(); + s_time_t timeout = start + (flush_dev_iotlb + ? iommu_dev_iotlb_timeout + : 100) * MILLISECS(threshold); while ( ACCESS_ONCE(*this_poll_slot) != QINVAL_STAT_DONE ) { - if ( NOW() > timeout ) + if ( timeout && NOW() > timeout ) { - print_qi_regs(iommu); + threshold |= threshold << 1; printk(XENLOG_WARNING VTDPREFIX - " Queue invalidate wait descriptor timed out\n"); - return -ETIMEDOUT; + " IOMMU#%u: QI%s wait descriptor taking too long\n", + iommu->index, flush_dev_iotlb ? " dev" : ""); + print_qi_regs(iommu); + timeout = 0; } cpu_relax(); } + + if ( !timeout ) + printk(XENLOG_WARNING VTDPREFIX + " IOMMU#%u: QI%s wait descriptor took %lums\n", + iommu->index, flush_dev_iotlb ? " dev" : "", + (NOW() - start) / 10000000); + return 0; }
xsa373/xsa373-4.11-1.patch
(application/octet-stream, 4.6 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: size qinval queue dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait descriptor). There can be one such pair of requests pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the queue ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -447,17 +447,9 @@ struct qinval_entry { }q; }; -/* Order of queue invalidation pages(max is 8) */ -#define QINVAL_PAGE_ORDER 2 - -#define QINVAL_ARCH_PAGE_ORDER (QINVAL_PAGE_ORDER + PAGE_SHIFT_4K - PAGE_SHIFT) -#define QINVAL_ARCH_PAGE_NR ( QINVAL_ARCH_PAGE_ORDER < 0 ? \ - 1 : \ - 1 << QINVAL_ARCH_PAGE_ORDER ) - /* Each entry is 16 bytes, so 2^8 entries per page */ #define QINVAL_ENTRY_ORDER ( PAGE_SHIFT - 4 ) -#define QINVAL_ENTRY_NR (1 << (QINVAL_PAGE_ORDER + 8)) +#define QINVAL_MAX_ENTRY_NR (1u << (7 + QINVAL_ENTRY_ORDER)) /* Status data flag */ #define QINVAL_STAT_INIT 0 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -31,6 +31,9 @@ #define VTD_QI_TIMEOUT 1 +static unsigned int __read_mostly qi_pg_order; +static unsigned int __read_mostly qi_entry_nr; + static int __must_check invalidate_sync(struct iommu *iommu); static void print_qi_regs(struct iommu *iommu) @@ -55,7 +58,7 @@ static unsigned int qinval_next_index(st tail >>= QINVAL_INDEX_SHIFT; /* (tail+1 == head) indicates a full queue, wait for HW */ - while ( ( tail + 1 ) % QINVAL_ENTRY_NR == + while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) cpu_relax(); @@ -68,7 +71,7 @@ static void qinval_update_qtail(struct i /* Need hold register lock when update tail */ ASSERT( spin_is_locked(&iommu->register_lock) ); - val = (index + 1) % QINVAL_ENTRY_NR; + val = (index + 1) & (qi_entry_nr - 1); dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT)); } @@ -417,7 +420,27 @@ int enable_qinval(struct iommu *iommu) if ( qi_ctrl->qinval_maddr == 0 ) { drhd = iommu_to_drhd(iommu); - qi_ctrl->qinval_maddr = alloc_pgtable_maddr(drhd, QINVAL_ARCH_PAGE_NR); + if ( !qi_entry_nr ) + { + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait descriptor). There + * can be one such pair of requests pending per CPU. One extra + * entry is needed as the ring is considered full when there's + * only one entry left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= QINVAL_MAX_ENTRY_NR); + qi_pg_order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + (PAGE_SHIFT - + QINVAL_ENTRY_ORDER)); + qi_entry_nr = 1u << (qi_pg_order + QINVAL_ENTRY_ORDER); + + dprintk(XENLOG_INFO VTDPREFIX, + "QI: using %u-entry ring(s)\n", qi_entry_nr); + } + + qi_ctrl->qinval_maddr = + alloc_pgtable_maddr(drhd, qi_entry_nr >> QINVAL_ENTRY_ORDER); if ( qi_ctrl->qinval_maddr == 0 ) { dprintk(XENLOG_WARNING VTDPREFIX, @@ -431,15 +454,16 @@ int enable_qinval(struct iommu *iommu) spin_lock_irqsave(&iommu->register_lock, flags); - /* Setup Invalidation Queue Address(IQA) register with the - * address of the page we just allocated. QS field at - * bits[2:0] to indicate size of queue is one 4KB page. - * That's 256 entries. Queued Head (IQH) and Queue Tail (IQT) - * registers are automatically reset to 0 with write - * to IQA register. + /* + * Setup Invalidation Queue Address (IQA) register with the address of the + * pages we just allocated. The QS field at bits[2:0] indicates the size + * (page order) of the queue. + * + * Queued Head (IQH) and Queue Tail (IQT) registers are automatically + * reset to 0 with write to IQA register. */ dmar_writeq(iommu->reg, DMAR_IQA_REG, - qi_ctrl->qinval_maddr | QINVAL_PAGE_ORDER); + qi_ctrl->qinval_maddr | qi_pg_order); dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
xsa373/xsa373-4.11-2.patch
(application/octet-stream, 4.3 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: size command buffer dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait command). There can be one such pair of commands pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the command ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -24,8 +24,7 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[]) { - u32 tail, head, *cmd_buffer; - int i; + uint32_t tail, head; tail = iommu->cmd_buffer.tail; if ( ++tail == iommu->cmd_buffer.entries ) @@ -35,12 +34,9 @@ static int queue_iommu_command(struct am IOMMU_CMD_BUFFER_HEAD_OFFSET)); if ( head != tail ) { - cmd_buffer = (u32 *)(iommu->cmd_buffer.buffer + - (iommu->cmd_buffer.tail * - IOMMU_CMD_BUFFER_ENTRY_SIZE)); - - for ( i = 0; i < IOMMU_CMD_BUFFER_U32_PER_ENTRY; i++ ) - cmd_buffer[i] = cmd[i]; + memcpy(iommu->cmd_buffer.buffer + + (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), + cmd, sizeof(cmd_entry_t)); iommu->cmd_buffer.tail = tail; return 1; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -136,7 +136,7 @@ static void register_iommu_cmd_buffer_in writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET); power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.alloc_size) + - IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE; + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER; entry = 0; iommu_set_addr_hi_to_reg(&entry, addr_hi); @@ -1000,9 +1000,31 @@ static void * __init allocate_ring_buffe static void * __init allocate_cmd_buffer(struct amd_iommu *iommu) { /* allocate 'command buffer' in power of 2 increments of 4K */ + static unsigned int __read_mostly nr_ents; + + if ( !nr_ents ) + { + unsigned int order; + + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait command). There can be + * one such pair of requests pending per CPU. One extra entry is + * needed as the ring is considered full when there's only one entry + * left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= IOMMU_CMD_BUFFER_MAX_ENTRIES); + order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + IOMMU_CMD_BUFFER_ENTRY_ORDER); + nr_ents = 1u << (order + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER); + + AMD_IOMMU_DEBUG("using %u-entry cmd ring(s)\n", nr_ents); + } + + BUILD_BUG_ON(sizeof(cmd_entry_t) != (1u << IOMMU_CMD_BUFFER_ENTRY_ORDER)); + return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), - IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, - "Command Buffer"); + nr_ents, "Command Buffer"); } static void * __init allocate_event_log(struct amd_iommu *iommu) --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -20,9 +20,6 @@ #ifndef _ASM_X86_64_AMD_IOMMU_DEFS_H #define _ASM_X86_64_AMD_IOMMU_DEFS_H -/* IOMMU Command Buffer entries: in power of 2 increments, minimum of 256 */ -#define IOMMU_CMD_BUFFER_DEFAULT_ENTRIES 512 - /* IOMMU Event Log entries: in power of 2 increments, minimum of 256 */ #define IOMMU_EVENT_LOG_DEFAULT_ENTRIES 512 @@ -185,9 +182,8 @@ #define IOMMU_CMD_BUFFER_LENGTH_MASK 0x0F000000 #define IOMMU_CMD_BUFFER_LENGTH_SHIFT 24 -#define IOMMU_CMD_BUFFER_ENTRY_SIZE 16 -#define IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE 8 -#define IOMMU_CMD_BUFFER_U32_PER_ENTRY (IOMMU_CMD_BUFFER_ENTRY_SIZE / 4) +#define IOMMU_CMD_BUFFER_ENTRY_ORDER 4 +#define IOMMU_CMD_BUFFER_MAX_ENTRIES (1u << 15) #define IOMMU_CMD_OPCODE_MASK 0xF0000000 #define IOMMU_CMD_OPCODE_SHIFT 28
xsa373/xsa373-4.11-3.patch
(application/octet-stream, 7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: eliminate flush related timeouts Leaving an in-progress operation pending when it appears to take too long is problematic: If e.g. a QI command completed later, the write to the "poll slot" may instead be understood to signal a subsequently started command's completion. Also our accounting of the timeout period was actually wrong: We included the time it took for the command to actually make it to the front of the queue, which could be heavily affected by guests other than the one for which the flush is being performed. Do away with all timeout detection on all flush related code paths. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Additionally log (once) if qinval_next_index() didn't immediately find an available slot. Together with the earlier change sizing the queue(s) dynamically, we should now have a guarantee that with our fully synchronous model any demand for slots can actually be satisfied. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,6 +127,34 @@ do { } \ } while (0) +#define IOMMU_FLUSH_WAIT(what, iommu, offset, op, cond, sts) \ +do { \ + static unsigned int __read_mostly threshold = 1; \ + s_time_t start = NOW(); \ + s_time_t timeout = start + DMAR_OPERATION_TIMEOUT * threshold; \ + \ + for ( ; ; ) \ + { \ + sts = op(iommu->reg, offset); \ + if ( cond ) \ + break; \ + if ( timeout && NOW() > timeout ) \ + { \ + threshold |= threshold << 1; \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush taking too long\n", \ + iommu->index, what); \ + timeout = 0; \ + } \ + cpu_relax(); \ + } \ + \ + if ( !timeout ) \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush took %lums\n", \ + iommu->index, what, (NOW() - start) / 10000000); \ +} while ( false ) + int vtd_hw_check(void); void disable_pmr(struct iommu *iommu); int is_igd_drhd(struct acpi_drhd_unit *drhd); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -357,8 +357,8 @@ static void iommu_flush_write_buffer(str dmar_writel(iommu->reg, DMAR_GCMD_REG, val | DMA_GCMD_WBF); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, dmar_readl, - !(val & DMA_GSTS_WBFS), val); + IOMMU_FLUSH_WAIT("write buffer", iommu, DMAR_GSTS_REG, dmar_readl, + !(val & DMA_GSTS_WBFS), val); spin_unlock_irqrestore(&iommu->register_lock, flags); } @@ -408,8 +408,8 @@ static int __must_check flush_context_re dmar_writeq(iommu->reg, DMAR_CCMD_REG, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_CCMD_REG, dmar_readq, - !(val & DMA_CCMD_ICC), val); + IOMMU_FLUSH_WAIT("context", iommu, DMAR_CCMD_REG, dmar_readq, + !(val & DMA_CCMD_ICC), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* flush context entry will implicitly flush write buffer */ @@ -491,8 +491,8 @@ static int __must_check flush_iotlb_reg( dmar_writeq(iommu->reg, tlb_offset + 8, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, (tlb_offset + 8), dmar_readq, - !(val & DMA_TLB_IVT), val); + IOMMU_FLUSH_WAIT("iotlb", iommu, (tlb_offset + 8), dmar_readq, + !(val & DMA_TLB_IVT), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* check IOTLB invalidation granularity */ --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -29,8 +29,6 @@ #include "extern.h" #include "../ats.h" -#define VTD_QI_TIMEOUT 1 - static unsigned int __read_mostly qi_pg_order; static unsigned int __read_mostly qi_entry_nr; @@ -60,7 +58,11 @@ static unsigned int qinval_next_index(st /* (tail+1 == head) indicates a full queue, wait for HW */ while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) + { + printk_once(XENLOG_ERR VTDPREFIX " IOMMU#%u: no QI slot available\n", + iommu->index); cpu_relax(); + } return tail; } @@ -180,23 +182,32 @@ static int __must_check queue_invalidate /* Now we don't support interrupt method */ if ( sw ) { - s_time_t timeout; - - /* In case all wait descriptor writes to same addr with same data */ - timeout = NOW() + MILLISECS(flush_dev_iotlb ? - iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT); + static unsigned int __read_mostly threshold = 1; + s_time_t start = NOW(); + s_time_t timeout = start + (flush_dev_iotlb + ? iommu_dev_iotlb_timeout + : 100) * MILLISECS(threshold); while ( ACCESS_ONCE(*this_poll_slot) != QINVAL_STAT_DONE ) { - if ( NOW() > timeout ) + if ( timeout && NOW() > timeout ) { - print_qi_regs(iommu); + threshold |= threshold << 1; printk(XENLOG_WARNING VTDPREFIX - " Queue invalidate wait descriptor timed out\n"); - return -ETIMEDOUT; + " IOMMU#%u: QI%s wait descriptor taking too long\n", + iommu->index, flush_dev_iotlb ? " dev" : ""); + print_qi_regs(iommu); + timeout = 0; } cpu_relax(); } + + if ( !timeout ) + printk(XENLOG_WARNING VTDPREFIX + " IOMMU#%u: QI%s wait descriptor took %lums\n", + iommu->index, flush_dev_iotlb ? " dev" : "", + (NOW() - start) / 10000000); + return 0; }
xsa373/xsa373-4.11-4.patch
(application/octet-stream, 2.8 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: wait for command slot to be available No caller cared about send_iommu_command() indicating unavailability of a slot. Hence if a sufficient number prior commands timed out, we did blindly assume that the requested command was submitted to the IOMMU when really it wasn't. This could mean both a hanging system (waiting for a command to complete that was never seen by the IOMMU) or blindly propagating success back to callers, making them believe they're fine to e.g. free previously unmapped pages. Fold the three involved functions into one, add spin waiting for an available slot along the lines of VT-d's qinval_next_index(), and as a consequence drop all error indicator return types/values. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -22,48 +22,36 @@ #include <asm/hvm/svm/amd-iommu-proto.h> #include "../ats.h" -static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[]) +static void send_iommu_command(struct amd_iommu *iommu, + const uint32_t cmd[4]) { - uint32_t tail, head; + uint32_t tail; tail = iommu->cmd_buffer.tail; if ( ++tail == iommu->cmd_buffer.entries ) tail = 0; - head = iommu_get_rb_pointer(readl(iommu->mmio_base + - IOMMU_CMD_BUFFER_HEAD_OFFSET)); - if ( head != tail ) + while ( tail == iommu_get_rb_pointer(readl(iommu->mmio_base + + IOMMU_CMD_BUFFER_HEAD_OFFSET)) ) { - memcpy(iommu->cmd_buffer.buffer + - (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), - cmd, sizeof(cmd_entry_t)); - - iommu->cmd_buffer.tail = tail; - return 1; + printk_once(XENLOG_ERR + "AMD IOMMU %04x:%02x:%02x.%u: no cmd slot available\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf)); + cpu_relax(); } - return 0; -} + memcpy(iommu->cmd_buffer.buffer + + (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), + cmd, sizeof(cmd_entry_t)); -static void commit_iommu_command_buffer(struct amd_iommu *iommu) -{ - u32 tail = 0; + iommu->cmd_buffer.tail = tail; + tail = 0; iommu_set_rb_pointer(&tail, iommu->cmd_buffer.tail); writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET); } -int send_iommu_command(struct amd_iommu *iommu, u32 cmd[]) -{ - if ( queue_iommu_command(iommu, cmd) ) - { - commit_iommu_command_buffer(iommu); - return 1; - } - - return 0; -} - static void flush_command_buffer(struct amd_iommu *iommu) { u32 cmd[4], status;
xsa373/xsa373-4.11-5.patch
(application/octet-stream, 5.5 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: drop command completion timeout First and foremost - such timeouts were not signaled to callers, making them believe they're fine to e.g. free previously unmapped pages. Mirror VT-d's behavior: A fixed number of loop iterations is not a suitable way to detect timeouts in an environment (CPU and bus speeds) independent manner anyway. Furthermore, leaving an in-progress operation pending when it appears to take too long is problematic: If a command completed later, the signaling of its completion may instead be understood to signal a subsequently started command's completion. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Allow callers to specify a non-default timeout bias for this logging, using the same values as VT-d does, which in particular means a (by default) much larger value for device IO TLB invalidation. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -52,10 +52,12 @@ static void send_iommu_command(struct am writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET); } -static void flush_command_buffer(struct amd_iommu *iommu) +static void flush_command_buffer(struct amd_iommu *iommu, + unsigned int timeout_base) { - u32 cmd[4], status; - int loop_count, comp_wait; + uint32_t cmd[4]; + s_time_t start, timeout; + static unsigned int __read_mostly threshold = 1; /* RW1C 'ComWaitInt' in status register */ writel(IOMMU_STATUS_COMP_WAIT_INT_MASK, @@ -71,24 +73,31 @@ static void flush_command_buffer(struct IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]); send_iommu_command(iommu, cmd); - /* Make loop_count long enough for polling completion wait bit */ - loop_count = 1000; - do { - status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - comp_wait = get_field_from_reg_u32(status, - IOMMU_STATUS_COMP_WAIT_INT_MASK, - IOMMU_STATUS_COMP_WAIT_INT_SHIFT); - --loop_count; - } while ( !comp_wait && loop_count ); - - if ( comp_wait ) + start = NOW(); + timeout = start + (timeout_base ?: 100) * MILLISECS(threshold); + while ( !(readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET) & + IOMMU_STATUS_COMP_WAIT_INT_MASK) ) { - /* RW1C 'ComWaitInt' in status register */ - writel(IOMMU_STATUS_COMP_WAIT_INT_MASK, - iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - return; + if ( timeout && NOW() > timeout ) + { + threshold |= threshold << 1; + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait taking too long\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : ""); + timeout = 0; + } + cpu_relax(); } - AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n"); + + if ( !timeout ) + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait took %lums\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : "", + (NOW() - start) / 10000000); } /* Build low level iommu command messages */ @@ -300,7 +309,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con /* send INVALIDATE_IOTLB_PAGES command */ spin_lock_irqsave(&iommu->lock, flags); invalidate_iotlb_pages(iommu, maxpend, 0, queueid, gaddr, req_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, iommu_dev_iotlb_timeout); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -337,7 +346,7 @@ static void _amd_iommu_flush_pages(struc { spin_lock_irqsave(&iommu->lock, flags); invalidate_iommu_pages(iommu, gaddr, dom_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -361,7 +370,7 @@ void amd_iommu_flush_device(struct amd_i ASSERT( spin_is_locked(&iommu->lock) ); invalidate_dev_table_entry(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf) @@ -369,7 +378,7 @@ void amd_iommu_flush_intremap(struct amd ASSERT( spin_is_locked(&iommu->lock) ); invalidate_interrupt_table(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_all_caches(struct amd_iommu *iommu) @@ -377,7 +386,7 @@ void amd_iommu_flush_all_caches(struct a ASSERT( spin_is_locked(&iommu->lock) ); invalidate_iommu_all(iommu); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) @@ -387,7 +396,8 @@ void amd_iommu_send_guest_cmd(struct amd spin_lock_irqsave(&iommu->lock, flags); send_iommu_command(iommu, cmd); - flush_command_buffer(iommu); + /* TBD: Timeout selection may require peeking into cmd[]. */ + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); }
xsa373/xsa373-4.12-1.patch
(application/octet-stream, 4.6 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: size qinval queue dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait descriptor). There can be one such pair of requests pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the queue ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -450,17 +450,9 @@ struct qinval_entry { }q; }; -/* Order of queue invalidation pages(max is 8) */ -#define QINVAL_PAGE_ORDER 2 - -#define QINVAL_ARCH_PAGE_ORDER (QINVAL_PAGE_ORDER + PAGE_SHIFT_4K - PAGE_SHIFT) -#define QINVAL_ARCH_PAGE_NR ( QINVAL_ARCH_PAGE_ORDER < 0 ? \ - 1 : \ - 1 << QINVAL_ARCH_PAGE_ORDER ) - /* Each entry is 16 bytes, so 2^8 entries per page */ #define QINVAL_ENTRY_ORDER ( PAGE_SHIFT - 4 ) -#define QINVAL_ENTRY_NR (1 << (QINVAL_PAGE_ORDER + 8)) +#define QINVAL_MAX_ENTRY_NR (1u << (7 + QINVAL_ENTRY_ORDER)) /* Status data flag */ #define QINVAL_STAT_INIT 0 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -31,6 +31,9 @@ #define VTD_QI_TIMEOUT 1 +static unsigned int __read_mostly qi_pg_order; +static unsigned int __read_mostly qi_entry_nr; + static int __must_check invalidate_sync(struct iommu *iommu); static void print_qi_regs(struct iommu *iommu) @@ -55,7 +58,7 @@ static unsigned int qinval_next_index(st tail >>= QINVAL_INDEX_SHIFT; /* (tail+1 == head) indicates a full queue, wait for HW */ - while ( ( tail + 1 ) % QINVAL_ENTRY_NR == + while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) cpu_relax(); @@ -68,7 +71,7 @@ static void qinval_update_qtail(struct i /* Need hold register lock when update tail */ ASSERT( spin_is_locked(&iommu->register_lock) ); - val = (index + 1) % QINVAL_ENTRY_NR; + val = (index + 1) & (qi_entry_nr - 1); dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT)); } @@ -417,7 +420,27 @@ int enable_qinval(struct iommu *iommu) if ( qi_ctrl->qinval_maddr == 0 ) { drhd = iommu_to_drhd(iommu); - qi_ctrl->qinval_maddr = alloc_pgtable_maddr(drhd, QINVAL_ARCH_PAGE_NR); + if ( !qi_entry_nr ) + { + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait descriptor). There + * can be one such pair of requests pending per CPU. One extra + * entry is needed as the ring is considered full when there's + * only one entry left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= QINVAL_MAX_ENTRY_NR); + qi_pg_order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + (PAGE_SHIFT - + QINVAL_ENTRY_ORDER)); + qi_entry_nr = 1u << (qi_pg_order + QINVAL_ENTRY_ORDER); + + dprintk(XENLOG_INFO VTDPREFIX, + "QI: using %u-entry ring(s)\n", qi_entry_nr); + } + + qi_ctrl->qinval_maddr = + alloc_pgtable_maddr(drhd, qi_entry_nr >> QINVAL_ENTRY_ORDER); if ( qi_ctrl->qinval_maddr == 0 ) { dprintk(XENLOG_WARNING VTDPREFIX, @@ -431,15 +454,16 @@ int enable_qinval(struct iommu *iommu) spin_lock_irqsave(&iommu->register_lock, flags); - /* Setup Invalidation Queue Address(IQA) register with the - * address of the page we just allocated. QS field at - * bits[2:0] to indicate size of queue is one 4KB page. - * That's 256 entries. Queued Head (IQH) and Queue Tail (IQT) - * registers are automatically reset to 0 with write - * to IQA register. + /* + * Setup Invalidation Queue Address (IQA) register with the address of the + * pages we just allocated. The QS field at bits[2:0] indicates the size + * (page order) of the queue. + * + * Queued Head (IQH) and Queue Tail (IQT) registers are automatically + * reset to 0 with write to IQA register. */ dmar_writeq(iommu->reg, DMAR_IQA_REG, - qi_ctrl->qinval_maddr | QINVAL_PAGE_ORDER); + qi_ctrl->qinval_maddr | qi_pg_order); dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
xsa373/xsa373-4.12-2.patch
(application/octet-stream, 3.8 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: size command buffer dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait command). There can be one such pair of commands pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the command ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -35,8 +35,8 @@ static int queue_iommu_command(struct am if ( head != tail ) { memcpy(iommu->cmd_buffer.buffer + - (iommu->cmd_buffer.tail * IOMMU_CMD_BUFFER_ENTRY_SIZE), - cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE); + (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), + cmd, sizeof(cmd_entry_t)); iommu->cmd_buffer.tail = tail; return 1; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -136,7 +136,7 @@ static void register_iommu_cmd_buffer_in writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET); power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.alloc_size) + - IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE; + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER; entry = 0; iommu_set_addr_hi_to_reg(&entry, addr_hi); @@ -1000,9 +1000,31 @@ static void * __init allocate_ring_buffe static void * __init allocate_cmd_buffer(struct amd_iommu *iommu) { /* allocate 'command buffer' in power of 2 increments of 4K */ + static unsigned int __read_mostly nr_ents; + + if ( !nr_ents ) + { + unsigned int order; + + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait command). There can be + * one such pair of requests pending per CPU. One extra entry is + * needed as the ring is considered full when there's only one entry + * left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= IOMMU_CMD_BUFFER_MAX_ENTRIES); + order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + IOMMU_CMD_BUFFER_ENTRY_ORDER); + nr_ents = 1u << (order + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER); + + AMD_IOMMU_DEBUG("using %u-entry cmd ring(s)\n", nr_ents); + } + + BUILD_BUG_ON(sizeof(cmd_entry_t) != (1u << IOMMU_CMD_BUFFER_ENTRY_ORDER)); + return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), - IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, - "Command Buffer"); + nr_ents, "Command Buffer"); } static void * __init allocate_event_log(struct amd_iommu *iommu) --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -20,9 +20,6 @@ #ifndef _ASM_X86_64_AMD_IOMMU_DEFS_H #define _ASM_X86_64_AMD_IOMMU_DEFS_H -/* IOMMU Command Buffer entries: in power of 2 increments, minimum of 256 */ -#define IOMMU_CMD_BUFFER_DEFAULT_ENTRIES 512 - /* IOMMU Event Log entries: in power of 2 increments, minimum of 256 */ #define IOMMU_EVENT_LOG_DEFAULT_ENTRIES 512 @@ -184,8 +181,8 @@ #define IOMMU_CMD_BUFFER_LENGTH_MASK 0x0F000000 #define IOMMU_CMD_BUFFER_LENGTH_SHIFT 24 -#define IOMMU_CMD_BUFFER_ENTRY_SIZE 16 -#define IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE 8 +#define IOMMU_CMD_BUFFER_ENTRY_ORDER 4 +#define IOMMU_CMD_BUFFER_MAX_ENTRIES (1u << 15) #define IOMMU_CMD_OPCODE_MASK 0xF0000000 #define IOMMU_CMD_OPCODE_SHIFT 28
xsa373/xsa373-4.12-3.patch
(application/octet-stream, 7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: eliminate flush related timeouts Leaving an in-progress operation pending when it appears to take too long is problematic: If e.g. a QI command completed later, the write to the "poll slot" may instead be understood to signal a subsequently started command's completion. Also our accounting of the timeout period was actually wrong: We included the time it took for the command to actually make it to the front of the queue, which could be heavily affected by guests other than the one for which the flush is being performed. Do away with all timeout detection on all flush related code paths. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Additionally log (once) if qinval_next_index() didn't immediately find an available slot. Together with the earlier change sizing the queue(s) dynamically, we should now have a guarantee that with our fully synchronous model any demand for slots can actually be satisfied. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,6 +127,34 @@ do { } \ } while (0) +#define IOMMU_FLUSH_WAIT(what, iommu, offset, op, cond, sts) \ +do { \ + static unsigned int __read_mostly threshold = 1; \ + s_time_t start = NOW(); \ + s_time_t timeout = start + DMAR_OPERATION_TIMEOUT * threshold; \ + \ + for ( ; ; ) \ + { \ + sts = op(iommu->reg, offset); \ + if ( cond ) \ + break; \ + if ( timeout && NOW() > timeout ) \ + { \ + threshold |= threshold << 1; \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush taking too long\n", \ + iommu->index, what); \ + timeout = 0; \ + } \ + cpu_relax(); \ + } \ + \ + if ( !timeout ) \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush took %lums\n", \ + iommu->index, what, (NOW() - start) / 10000000); \ +} while ( false ) + int vtd_hw_check(void); void disable_pmr(struct iommu *iommu); int is_igd_drhd(struct acpi_drhd_unit *drhd); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -357,8 +357,8 @@ static void iommu_flush_write_buffer(str dmar_writel(iommu->reg, DMAR_GCMD_REG, val | DMA_GCMD_WBF); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, dmar_readl, - !(val & DMA_GSTS_WBFS), val); + IOMMU_FLUSH_WAIT("write buffer", iommu, DMAR_GSTS_REG, dmar_readl, + !(val & DMA_GSTS_WBFS), val); spin_unlock_irqrestore(&iommu->register_lock, flags); } @@ -408,8 +408,8 @@ int vtd_flush_context_reg(void *_iommu, dmar_writeq(iommu->reg, DMAR_CCMD_REG, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_CCMD_REG, dmar_readq, - !(val & DMA_CCMD_ICC), val); + IOMMU_FLUSH_WAIT("context", iommu, DMAR_CCMD_REG, dmar_readq, + !(val & DMA_CCMD_ICC), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* flush context entry will implicitly flush write buffer */ @@ -491,8 +491,8 @@ int vtd_flush_iotlb_reg(void *_iommu, ui dmar_writeq(iommu->reg, tlb_offset + 8, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, (tlb_offset + 8), dmar_readq, - !(val & DMA_TLB_IVT), val); + IOMMU_FLUSH_WAIT("iotlb", iommu, (tlb_offset + 8), dmar_readq, + !(val & DMA_TLB_IVT), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* check IOTLB invalidation granularity */ --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -29,8 +29,6 @@ #include "extern.h" #include "../ats.h" -#define VTD_QI_TIMEOUT 1 - static unsigned int __read_mostly qi_pg_order; static unsigned int __read_mostly qi_entry_nr; @@ -60,7 +58,11 @@ static unsigned int qinval_next_index(st /* (tail+1 == head) indicates a full queue, wait for HW */ while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) + { + printk_once(XENLOG_ERR VTDPREFIX " IOMMU#%u: no QI slot available\n", + iommu->index); cpu_relax(); + } return tail; } @@ -180,23 +182,32 @@ static int __must_check queue_invalidate /* Now we don't support interrupt method */ if ( sw ) { - s_time_t timeout; - - /* In case all wait descriptor writes to same addr with same data */ - timeout = NOW() + MILLISECS(flush_dev_iotlb ? - iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT); + static unsigned int __read_mostly threshold = 1; + s_time_t start = NOW(); + s_time_t timeout = start + (flush_dev_iotlb + ? iommu_dev_iotlb_timeout + : 100) * MILLISECS(threshold); while ( ACCESS_ONCE(*this_poll_slot) != QINVAL_STAT_DONE ) { - if ( NOW() > timeout ) + if ( timeout && NOW() > timeout ) { - print_qi_regs(iommu); + threshold |= threshold << 1; printk(XENLOG_WARNING VTDPREFIX - " Queue invalidate wait descriptor timed out\n"); - return -ETIMEDOUT; + " IOMMU#%u: QI%s wait descriptor taking too long\n", + iommu->index, flush_dev_iotlb ? " dev" : ""); + print_qi_regs(iommu); + timeout = 0; } cpu_relax(); } + + if ( !timeout ) + printk(XENLOG_WARNING VTDPREFIX + " IOMMU#%u: QI%s wait descriptor took %lums\n", + iommu->index, flush_dev_iotlb ? " dev" : "", + (NOW() - start) / 10000000); + return 0; }
xsa373/xsa373-4.12-4.patch
(application/octet-stream, 2.8 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: wait for command slot to be available No caller cared about send_iommu_command() indicating unavailability of a slot. Hence if a sufficient number prior commands timed out, we did blindly assume that the requested command was submitted to the IOMMU when really it wasn't. This could mean both a hanging system (waiting for a command to complete that was never seen by the IOMMU) or blindly propagating success back to callers, making them believe they're fine to e.g. free previously unmapped pages. Fold the three involved functions into one, add spin waiting for an available slot along the lines of VT-d's qinval_next_index(), and as a consequence drop all error indicator return types/values. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -22,48 +22,36 @@ #include <asm/hvm/svm/amd-iommu-proto.h> #include "../ats.h" -static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[]) +static void send_iommu_command(struct amd_iommu *iommu, + const uint32_t cmd[4]) { - uint32_t tail, head; + uint32_t tail; tail = iommu->cmd_buffer.tail; if ( ++tail == iommu->cmd_buffer.entries ) tail = 0; - head = iommu_get_rb_pointer(readl(iommu->mmio_base + - IOMMU_CMD_BUFFER_HEAD_OFFSET)); - if ( head != tail ) + while ( tail == iommu_get_rb_pointer(readl(iommu->mmio_base + + IOMMU_CMD_BUFFER_HEAD_OFFSET)) ) { - memcpy(iommu->cmd_buffer.buffer + - (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), - cmd, sizeof(cmd_entry_t)); - - iommu->cmd_buffer.tail = tail; - return 1; + printk_once(XENLOG_ERR + "AMD IOMMU %04x:%02x:%02x.%u: no cmd slot available\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf)); + cpu_relax(); } - return 0; -} + memcpy(iommu->cmd_buffer.buffer + + (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), + cmd, sizeof(cmd_entry_t)); -static void commit_iommu_command_buffer(struct amd_iommu *iommu) -{ - u32 tail = 0; + iommu->cmd_buffer.tail = tail; + tail = 0; iommu_set_rb_pointer(&tail, iommu->cmd_buffer.tail); writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET); } -int send_iommu_command(struct amd_iommu *iommu, u32 cmd[]) -{ - if ( queue_iommu_command(iommu, cmd) ) - { - commit_iommu_command_buffer(iommu); - return 1; - } - - return 0; -} - static void flush_command_buffer(struct amd_iommu *iommu) { u32 cmd[4], status;
xsa373/xsa373-4.12-5.patch
(application/octet-stream, 5.5 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: drop command completion timeout First and foremost - such timeouts were not signaled to callers, making them believe they're fine to e.g. free previously unmapped pages. Mirror VT-d's behavior: A fixed number of loop iterations is not a suitable way to detect timeouts in an environment (CPU and bus speeds) independent manner anyway. Furthermore, leaving an in-progress operation pending when it appears to take too long is problematic: If a command completed later, the signaling of its completion may instead be understood to signal a subsequently started command's completion. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Allow callers to specify a non-default timeout bias for this logging, using the same values as VT-d does, which in particular means a (by default) much larger value for device IO TLB invalidation. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -52,10 +52,12 @@ static void send_iommu_command(struct am writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET); } -static void flush_command_buffer(struct amd_iommu *iommu) +static void flush_command_buffer(struct amd_iommu *iommu, + unsigned int timeout_base) { - u32 cmd[4], status; - int loop_count, comp_wait; + uint32_t cmd[4]; + s_time_t start, timeout; + static unsigned int __read_mostly threshold = 1; /* RW1C 'ComWaitInt' in status register */ writel(IOMMU_STATUS_COMP_WAIT_INT_MASK, @@ -71,24 +73,31 @@ static void flush_command_buffer(struct IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]); send_iommu_command(iommu, cmd); - /* Make loop_count long enough for polling completion wait bit */ - loop_count = 1000; - do { - status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - comp_wait = get_field_from_reg_u32(status, - IOMMU_STATUS_COMP_WAIT_INT_MASK, - IOMMU_STATUS_COMP_WAIT_INT_SHIFT); - --loop_count; - } while ( !comp_wait && loop_count ); - - if ( comp_wait ) + start = NOW(); + timeout = start + (timeout_base ?: 100) * MILLISECS(threshold); + while ( !(readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET) & + IOMMU_STATUS_COMP_WAIT_INT_MASK) ) { - /* RW1C 'ComWaitInt' in status register */ - writel(IOMMU_STATUS_COMP_WAIT_INT_MASK, - iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - return; + if ( timeout && NOW() > timeout ) + { + threshold |= threshold << 1; + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait taking too long\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : ""); + timeout = 0; + } + cpu_relax(); } - AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n"); + + if ( !timeout ) + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait took %lums\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : "", + (NOW() - start) / 10000000); } /* Build low level iommu command messages */ @@ -300,7 +309,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con /* send INVALIDATE_IOTLB_PAGES command */ spin_lock_irqsave(&iommu->lock, flags); invalidate_iotlb_pages(iommu, maxpend, 0, queueid, daddr, req_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, iommu_dev_iotlb_timeout); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -337,7 +346,7 @@ static void _amd_iommu_flush_pages(struc { spin_lock_irqsave(&iommu->lock, flags); invalidate_iommu_pages(iommu, daddr, dom_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -361,7 +370,7 @@ void amd_iommu_flush_device(struct amd_i ASSERT( spin_is_locked(&iommu->lock) ); invalidate_dev_table_entry(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf) @@ -369,7 +378,7 @@ void amd_iommu_flush_intremap(struct amd ASSERT( spin_is_locked(&iommu->lock) ); invalidate_interrupt_table(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_all_caches(struct amd_iommu *iommu) @@ -377,7 +386,7 @@ void amd_iommu_flush_all_caches(struct a ASSERT( spin_is_locked(&iommu->lock) ); invalidate_iommu_all(iommu); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) @@ -387,7 +396,8 @@ void amd_iommu_send_guest_cmd(struct amd spin_lock_irqsave(&iommu->lock, flags); send_iommu_command(iommu, cmd); - flush_command_buffer(iommu); + /* TBD: Timeout selection may require peeking into cmd[]. */ + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); }
xsa373/xsa373-4.13-1.patch
(application/octet-stream, 4.7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: size qinval queue dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait descriptor). There can be one such pair of requests pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the queue ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -450,17 +450,9 @@ struct qinval_entry { }q; }; -/* Order of queue invalidation pages(max is 8) */ -#define QINVAL_PAGE_ORDER 2 - -#define QINVAL_ARCH_PAGE_ORDER (QINVAL_PAGE_ORDER + PAGE_SHIFT_4K - PAGE_SHIFT) -#define QINVAL_ARCH_PAGE_NR ( QINVAL_ARCH_PAGE_ORDER < 0 ? \ - 1 : \ - 1 << QINVAL_ARCH_PAGE_ORDER ) - /* Each entry is 16 bytes, so 2^8 entries per page */ #define QINVAL_ENTRY_ORDER ( PAGE_SHIFT - 4 ) -#define QINVAL_ENTRY_NR (1 << (QINVAL_PAGE_ORDER + 8)) +#define QINVAL_MAX_ENTRY_NR (1u << (7 + QINVAL_ENTRY_ORDER)) /* Status data flag */ #define QINVAL_STAT_INIT 0 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -31,6 +31,9 @@ #define VTD_QI_TIMEOUT 1 +static unsigned int __read_mostly qi_pg_order; +static unsigned int __read_mostly qi_entry_nr; + static int __must_check invalidate_sync(struct vtd_iommu *iommu); static void print_qi_regs(struct vtd_iommu *iommu) @@ -55,7 +58,7 @@ static unsigned int qinval_next_index(st tail >>= QINVAL_INDEX_SHIFT; /* (tail+1 == head) indicates a full queue, wait for HW */ - while ( ( tail + 1 ) % QINVAL_ENTRY_NR == + while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) cpu_relax(); @@ -68,7 +71,7 @@ static void qinval_update_qtail(struct v /* Need hold register lock when update tail */ ASSERT( spin_is_locked(&iommu->register_lock) ); - val = (index + 1) % QINVAL_ENTRY_NR; + val = (index + 1) & (qi_entry_nr - 1); dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT)); } @@ -403,8 +406,28 @@ int enable_qinval(struct vtd_iommu *iomm if ( iommu->qinval_maddr == 0 ) { - iommu->qinval_maddr = alloc_pgtable_maddr(QINVAL_ARCH_PAGE_NR, - iommu->node); + if ( !qi_entry_nr ) + { + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait descriptor). There + * can be one such pair of requests pending per CPU. One extra + * entry is needed as the ring is considered full when there's + * only one entry left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= QINVAL_MAX_ENTRY_NR); + qi_pg_order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + (PAGE_SHIFT - + QINVAL_ENTRY_ORDER)); + qi_entry_nr = 1u << (qi_pg_order + QINVAL_ENTRY_ORDER); + + dprintk(XENLOG_INFO VTDPREFIX, + "QI: using %u-entry ring(s)\n", qi_entry_nr); + } + + iommu->qinval_maddr = + alloc_pgtable_maddr(qi_entry_nr >> QINVAL_ENTRY_ORDER, + iommu->node); if ( iommu->qinval_maddr == 0 ) { dprintk(XENLOG_WARNING VTDPREFIX, @@ -418,15 +441,16 @@ int enable_qinval(struct vtd_iommu *iomm spin_lock_irqsave(&iommu->register_lock, flags); - /* Setup Invalidation Queue Address(IQA) register with the - * address of the page we just allocated. QS field at - * bits[2:0] to indicate size of queue is one 4KB page. - * That's 256 entries. Queued Head (IQH) and Queue Tail (IQT) - * registers are automatically reset to 0 with write - * to IQA register. + /* + * Setup Invalidation Queue Address (IQA) register with the address of the + * pages we just allocated. The QS field at bits[2:0] indicates the size + * (page order) of the queue. + * + * Queued Head (IQH) and Queue Tail (IQT) registers are automatically + * reset to 0 with write to IQA register. */ dmar_writeq(iommu->reg, DMAR_IQA_REG, - iommu->qinval_maddr | QINVAL_PAGE_ORDER); + iommu->qinval_maddr | qi_pg_order); dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
xsa373/xsa373-4.13-2.patch
(application/octet-stream, 3.8 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: size command buffer dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait command). There can be one such pair of commands pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the command ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -35,8 +35,8 @@ static int queue_iommu_command(struct am if ( head != tail ) { memcpy(iommu->cmd_buffer.buffer + - (iommu->cmd_buffer.tail * IOMMU_CMD_BUFFER_ENTRY_SIZE), - cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE); + (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), + cmd, sizeof(cmd_entry_t)); iommu->cmd_buffer.tail = tail; return 1; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -125,7 +125,7 @@ static void register_iommu_cmd_buffer_in writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET); power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.alloc_size) + - IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE; + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER; entry = 0; iommu_set_addr_hi_to_reg(&entry, addr_hi); @@ -1050,9 +1050,31 @@ static void *__init allocate_ring_buffer static void * __init allocate_cmd_buffer(struct amd_iommu *iommu) { /* allocate 'command buffer' in power of 2 increments of 4K */ + static unsigned int __read_mostly nr_ents; + + if ( !nr_ents ) + { + unsigned int order; + + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait command). There can be + * one such pair of requests pending per CPU. One extra entry is + * needed as the ring is considered full when there's only one entry + * left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= IOMMU_CMD_BUFFER_MAX_ENTRIES); + order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + IOMMU_CMD_BUFFER_ENTRY_ORDER); + nr_ents = 1u << (order + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER); + + AMD_IOMMU_DEBUG("using %u-entry cmd ring(s)\n", nr_ents); + } + + BUILD_BUG_ON(sizeof(cmd_entry_t) != (1u << IOMMU_CMD_BUFFER_ENTRY_ORDER)); + return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), - IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, - "Command Buffer", false); + nr_ents, "Command Buffer", false); } static void * __init allocate_event_log(struct amd_iommu *iommu) --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -20,9 +20,6 @@ #ifndef _ASM_X86_64_AMD_IOMMU_DEFS_H #define _ASM_X86_64_AMD_IOMMU_DEFS_H -/* IOMMU Command Buffer entries: in power of 2 increments, minimum of 256 */ -#define IOMMU_CMD_BUFFER_DEFAULT_ENTRIES 512 - /* IOMMU Event Log entries: in power of 2 increments, minimum of 256 */ #define IOMMU_EVENT_LOG_DEFAULT_ENTRIES 512 @@ -168,8 +165,8 @@ struct amd_iommu_dte { #define IOMMU_CMD_BUFFER_LENGTH_MASK 0x0F000000 #define IOMMU_CMD_BUFFER_LENGTH_SHIFT 24 -#define IOMMU_CMD_BUFFER_ENTRY_SIZE 16 -#define IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE 8 +#define IOMMU_CMD_BUFFER_ENTRY_ORDER 4 +#define IOMMU_CMD_BUFFER_MAX_ENTRIES (1u << 15) #define IOMMU_CMD_OPCODE_MASK 0xF0000000 #define IOMMU_CMD_OPCODE_SHIFT 28
xsa373/xsa373-4.13-3.patch
(application/octet-stream, 7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: eliminate flush related timeouts Leaving an in-progress operation pending when it appears to take too long is problematic: If e.g. a QI command completed later, the write to the "poll slot" may instead be understood to signal a subsequently started command's completion. Also our accounting of the timeout period was actually wrong: We included the time it took for the command to actually make it to the front of the queue, which could be heavily affected by guests other than the one for which the flush is being performed. Do away with all timeout detection on all flush related code paths. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Additionally log (once) if qinval_next_index() didn't immediately find an available slot. Together with the earlier change sizing the queue(s) dynamically, we should now have a guarantee that with our fully synchronous model any demand for slots can actually be satisfied. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,6 +127,34 @@ do { } \ } while (0) +#define IOMMU_FLUSH_WAIT(what, iommu, offset, op, cond, sts) \ +do { \ + static unsigned int __read_mostly threshold = 1; \ + s_time_t start = NOW(); \ + s_time_t timeout = start + DMAR_OPERATION_TIMEOUT * threshold; \ + \ + for ( ; ; ) \ + { \ + sts = op(iommu->reg, offset); \ + if ( cond ) \ + break; \ + if ( timeout && NOW() > timeout ) \ + { \ + threshold |= threshold << 1; \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush taking too long\n", \ + iommu->index, what); \ + timeout = 0; \ + } \ + cpu_relax(); \ + } \ + \ + if ( !timeout ) \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush took %lums\n", \ + iommu->index, what, (NOW() - start) / 10000000); \ +} while ( false ) + int vtd_hw_check(void); void disable_pmr(struct vtd_iommu *iommu); int is_igd_drhd(struct acpi_drhd_unit *drhd); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -320,8 +320,8 @@ static void iommu_flush_write_buffer(str dmar_writel(iommu->reg, DMAR_GCMD_REG, val | DMA_GCMD_WBF); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, dmar_readl, - !(val & DMA_GSTS_WBFS), val); + IOMMU_FLUSH_WAIT("write buffer", iommu, DMAR_GSTS_REG, dmar_readl, + !(val & DMA_GSTS_WBFS), val); spin_unlock_irqrestore(&iommu->register_lock, flags); } @@ -370,8 +370,8 @@ int vtd_flush_context_reg(struct vtd_iom dmar_writeq(iommu->reg, DMAR_CCMD_REG, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_CCMD_REG, dmar_readq, - !(val & DMA_CCMD_ICC), val); + IOMMU_FLUSH_WAIT("context", iommu, DMAR_CCMD_REG, dmar_readq, + !(val & DMA_CCMD_ICC), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* flush context entry will implicitly flush write buffer */ @@ -448,8 +448,8 @@ int vtd_flush_iotlb_reg(struct vtd_iommu dmar_writeq(iommu->reg, tlb_offset + 8, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, (tlb_offset + 8), dmar_readq, - !(val & DMA_TLB_IVT), val); + IOMMU_FLUSH_WAIT("iotlb", iommu, (tlb_offset + 8), dmar_readq, + !(val & DMA_TLB_IVT), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* check IOTLB invalidation granularity */ --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -29,8 +29,6 @@ #include "extern.h" #include "../ats.h" -#define VTD_QI_TIMEOUT 1 - static unsigned int __read_mostly qi_pg_order; static unsigned int __read_mostly qi_entry_nr; @@ -60,7 +58,11 @@ static unsigned int qinval_next_index(st /* (tail+1 == head) indicates a full queue, wait for HW */ while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) + { + printk_once(XENLOG_ERR VTDPREFIX " IOMMU#%u: no QI slot available\n", + iommu->index); cpu_relax(); + } return tail; } @@ -180,23 +182,32 @@ static int __must_check queue_invalidate /* Now we don't support interrupt method */ if ( sw ) { - s_time_t timeout; - - /* In case all wait descriptor writes to same addr with same data */ - timeout = NOW() + MILLISECS(flush_dev_iotlb ? - iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT); + static unsigned int __read_mostly threshold = 1; + s_time_t start = NOW(); + s_time_t timeout = start + (flush_dev_iotlb + ? iommu_dev_iotlb_timeout + : 100) * MILLISECS(threshold); while ( ACCESS_ONCE(*this_poll_slot) != QINVAL_STAT_DONE ) { - if ( NOW() > timeout ) + if ( timeout && NOW() > timeout ) { - print_qi_regs(iommu); + threshold |= threshold << 1; printk(XENLOG_WARNING VTDPREFIX - " Queue invalidate wait descriptor timed out\n"); - return -ETIMEDOUT; + " IOMMU#%u: QI%s wait descriptor taking too long\n", + iommu->index, flush_dev_iotlb ? " dev" : ""); + print_qi_regs(iommu); + timeout = 0; } cpu_relax(); } + + if ( !timeout ) + printk(XENLOG_WARNING VTDPREFIX + " IOMMU#%u: QI%s wait descriptor took %lums\n", + iommu->index, flush_dev_iotlb ? " dev" : "", + (NOW() - start) / 10000000); + return 0; }
xsa373/xsa373-4.13-4.patch
(application/octet-stream, 2.8 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: wait for command slot to be available No caller cared about send_iommu_command() indicating unavailability of a slot. Hence if a sufficient number prior commands timed out, we did blindly assume that the requested command was submitted to the IOMMU when really it wasn't. This could mean both a hanging system (waiting for a command to complete that was never seen by the IOMMU) or blindly propagating success back to callers, making them believe they're fine to e.g. free previously unmapped pages. Fold the three involved functions into one, add spin waiting for an available slot along the lines of VT-d's qinval_next_index(), and as a consequence drop all error indicator return types/values. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -22,48 +22,36 @@ #include <asm/hvm/svm/amd-iommu-proto.h> #include "../ats.h" -static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[]) +static void send_iommu_command(struct amd_iommu *iommu, + const uint32_t cmd[4]) { - uint32_t tail, head; + uint32_t tail; tail = iommu->cmd_buffer.tail; if ( ++tail == iommu->cmd_buffer.entries ) tail = 0; - head = iommu_get_rb_pointer(readl(iommu->mmio_base + - IOMMU_CMD_BUFFER_HEAD_OFFSET)); - if ( head != tail ) + while ( tail == iommu_get_rb_pointer(readl(iommu->mmio_base + + IOMMU_CMD_BUFFER_HEAD_OFFSET)) ) { - memcpy(iommu->cmd_buffer.buffer + - (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), - cmd, sizeof(cmd_entry_t)); - - iommu->cmd_buffer.tail = tail; - return 1; + printk_once(XENLOG_ERR + "AMD IOMMU %04x:%02x:%02x.%u: no cmd slot available\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf)); + cpu_relax(); } - return 0; -} + memcpy(iommu->cmd_buffer.buffer + + (iommu->cmd_buffer.tail * sizeof(cmd_entry_t)), + cmd, sizeof(cmd_entry_t)); -static void commit_iommu_command_buffer(struct amd_iommu *iommu) -{ - u32 tail = 0; + iommu->cmd_buffer.tail = tail; + tail = 0; iommu_set_rb_pointer(&tail, iommu->cmd_buffer.tail); writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET); } -int send_iommu_command(struct amd_iommu *iommu, u32 cmd[]) -{ - if ( queue_iommu_command(iommu, cmd) ) - { - commit_iommu_command_buffer(iommu); - return 1; - } - - return 0; -} - static void flush_command_buffer(struct amd_iommu *iommu) { u32 cmd[4], status;
xsa373/xsa373-4.13-5.patch
(application/octet-stream, 5.5 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: drop command completion timeout First and foremost - such timeouts were not signaled to callers, making them believe they're fine to e.g. free previously unmapped pages. Mirror VT-d's behavior: A fixed number of loop iterations is not a suitable way to detect timeouts in an environment (CPU and bus speeds) independent manner anyway. Furthermore, leaving an in-progress operation pending when it appears to take too long is problematic: If a command completed later, the signaling of its completion may instead be understood to signal a subsequently started command's completion. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Allow callers to specify a non-default timeout bias for this logging, using the same values as VT-d does, which in particular means a (by default) much larger value for device IO TLB invalidation. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -52,10 +52,12 @@ static void send_iommu_command(struct am writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET); } -static void flush_command_buffer(struct amd_iommu *iommu) +static void flush_command_buffer(struct amd_iommu *iommu, + unsigned int timeout_base) { - u32 cmd[4], status; - int loop_count, comp_wait; + uint32_t cmd[4]; + s_time_t start, timeout; + static unsigned int __read_mostly threshold = 1; /* RW1C 'ComWaitInt' in status register */ writel(IOMMU_STATUS_COMP_WAIT_INT_MASK, @@ -71,24 +73,31 @@ static void flush_command_buffer(struct IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]); send_iommu_command(iommu, cmd); - /* Make loop_count long enough for polling completion wait bit */ - loop_count = 1000; - do { - status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - comp_wait = get_field_from_reg_u32(status, - IOMMU_STATUS_COMP_WAIT_INT_MASK, - IOMMU_STATUS_COMP_WAIT_INT_SHIFT); - --loop_count; - } while ( !comp_wait && loop_count ); - - if ( comp_wait ) + start = NOW(); + timeout = start + (timeout_base ?: 100) * MILLISECS(threshold); + while ( !(readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET) & + IOMMU_STATUS_COMP_WAIT_INT_MASK) ) { - /* RW1C 'ComWaitInt' in status register */ - writel(IOMMU_STATUS_COMP_WAIT_INT_MASK, - iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - return; + if ( timeout && NOW() > timeout ) + { + threshold |= threshold << 1; + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait taking too long\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : ""); + timeout = 0; + } + cpu_relax(); } - AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n"); + + if ( !timeout ) + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait took %lums\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : "", + (NOW() - start) / 10000000); } /* Build low level iommu command messages */ @@ -300,7 +309,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con /* send INVALIDATE_IOTLB_PAGES command */ spin_lock_irqsave(&iommu->lock, flags); invalidate_iotlb_pages(iommu, maxpend, 0, queueid, daddr, req_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, iommu_dev_iotlb_timeout); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -337,7 +346,7 @@ static void _amd_iommu_flush_pages(struc { spin_lock_irqsave(&iommu->lock, flags); invalidate_iommu_pages(iommu, daddr, dom_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -361,7 +370,7 @@ void amd_iommu_flush_device(struct amd_i ASSERT( spin_is_locked(&iommu->lock) ); invalidate_dev_table_entry(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf) @@ -369,7 +378,7 @@ void amd_iommu_flush_intremap(struct amd ASSERT( spin_is_locked(&iommu->lock) ); invalidate_interrupt_table(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_all_caches(struct amd_iommu *iommu) @@ -377,7 +386,7 @@ void amd_iommu_flush_all_caches(struct a ASSERT( spin_is_locked(&iommu->lock) ); invalidate_iommu_all(iommu); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) @@ -387,7 +396,8 @@ void amd_iommu_send_guest_cmd(struct amd spin_lock_irqsave(&iommu->lock, flags); send_iommu_command(iommu, cmd); - flush_command_buffer(iommu); + /* TBD: Timeout selection may require peeking into cmd[]. */ + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); }
xsa373/xsa373-4.14-1.patch
(application/octet-stream, 4.7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: size qinval queue dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait descriptor). There can be one such pair of requests pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the queue ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -450,17 +450,9 @@ struct qinval_entry { }q; }; -/* Order of queue invalidation pages(max is 8) */ -#define QINVAL_PAGE_ORDER 2 - -#define QINVAL_ARCH_PAGE_ORDER (QINVAL_PAGE_ORDER + PAGE_SHIFT_4K - PAGE_SHIFT) -#define QINVAL_ARCH_PAGE_NR ( QINVAL_ARCH_PAGE_ORDER < 0 ? \ - 1 : \ - 1 << QINVAL_ARCH_PAGE_ORDER ) - /* Each entry is 16 bytes, so 2^8 entries per page */ #define QINVAL_ENTRY_ORDER ( PAGE_SHIFT - 4 ) -#define QINVAL_ENTRY_NR (1 << (QINVAL_PAGE_ORDER + 8)) +#define QINVAL_MAX_ENTRY_NR (1u << (7 + QINVAL_ENTRY_ORDER)) /* Status data flag */ #define QINVAL_STAT_INIT 0 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -31,6 +31,9 @@ #define VTD_QI_TIMEOUT 1 +static unsigned int __read_mostly qi_pg_order; +static unsigned int __read_mostly qi_entry_nr; + static int __must_check invalidate_sync(struct vtd_iommu *iommu); static void print_qi_regs(struct vtd_iommu *iommu) @@ -55,7 +58,7 @@ static unsigned int qinval_next_index(st tail >>= QINVAL_INDEX_SHIFT; /* (tail+1 == head) indicates a full queue, wait for HW */ - while ( ( tail + 1 ) % QINVAL_ENTRY_NR == + while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) cpu_relax(); @@ -68,7 +71,7 @@ static void qinval_update_qtail(struct v /* Need hold register lock when update tail */ ASSERT( spin_is_locked(&iommu->register_lock) ); - val = (index + 1) % QINVAL_ENTRY_NR; + val = (index + 1) & (qi_entry_nr - 1); dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT)); } @@ -403,8 +406,28 @@ int enable_qinval(struct vtd_iommu *iomm if ( iommu->qinval_maddr == 0 ) { - iommu->qinval_maddr = alloc_pgtable_maddr(QINVAL_ARCH_PAGE_NR, - iommu->node); + if ( !qi_entry_nr ) + { + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait descriptor). There + * can be one such pair of requests pending per CPU. One extra + * entry is needed as the ring is considered full when there's + * only one entry left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= QINVAL_MAX_ENTRY_NR); + qi_pg_order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + (PAGE_SHIFT - + QINVAL_ENTRY_ORDER)); + qi_entry_nr = 1u << (qi_pg_order + QINVAL_ENTRY_ORDER); + + dprintk(XENLOG_INFO VTDPREFIX, + "QI: using %u-entry ring(s)\n", qi_entry_nr); + } + + iommu->qinval_maddr = + alloc_pgtable_maddr(qi_entry_nr >> QINVAL_ENTRY_ORDER, + iommu->node); if ( iommu->qinval_maddr == 0 ) { dprintk(XENLOG_WARNING VTDPREFIX, @@ -418,15 +441,16 @@ int enable_qinval(struct vtd_iommu *iomm spin_lock_irqsave(&iommu->register_lock, flags); - /* Setup Invalidation Queue Address(IQA) register with the - * address of the page we just allocated. QS field at - * bits[2:0] to indicate size of queue is one 4KB page. - * That's 256 entries. Queued Head (IQH) and Queue Tail (IQT) - * registers are automatically reset to 0 with write - * to IQA register. + /* + * Setup Invalidation Queue Address (IQA) register with the address of the + * pages we just allocated. The QS field at bits[2:0] indicates the size + * (page order) of the queue. + * + * Queued Head (IQH) and Queue Tail (IQT) registers are automatically + * reset to 0 with write to IQA register. */ dmar_writeq(iommu->reg, DMAR_IQA_REG, - iommu->qinval_maddr | QINVAL_PAGE_ORDER); + iommu->qinval_maddr | qi_pg_order); dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
xsa373/xsa373-4.14-2.patch
(application/octet-stream, 3.9 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: size command buffer dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait command). There can be one such pair of commands pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the command ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu-defs.h +++ b/xen/drivers/passthrough/amd/iommu-defs.h @@ -20,9 +20,6 @@ #ifndef AMD_IOMMU_DEFS_H #define AMD_IOMMU_DEFS_H -/* IOMMU Command Buffer entries: in power of 2 increments, minimum of 256 */ -#define IOMMU_CMD_BUFFER_DEFAULT_ENTRIES 512 - /* IOMMU Event Log entries: in power of 2 increments, minimum of 256 */ #define IOMMU_EVENT_LOG_DEFAULT_ENTRIES 512 @@ -164,8 +161,8 @@ struct amd_iommu_dte { #define IOMMU_CMD_BUFFER_LENGTH_MASK 0x0F000000 #define IOMMU_CMD_BUFFER_LENGTH_SHIFT 24 -#define IOMMU_CMD_BUFFER_ENTRY_SIZE 16 -#define IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE 8 +#define IOMMU_CMD_BUFFER_ENTRY_ORDER 4 +#define IOMMU_CMD_BUFFER_MAX_ENTRIES (1u << 15) #define IOMMU_CMD_OPCODE_MASK 0xF0000000 #define IOMMU_CMD_OPCODE_SHIFT 28 --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -24,7 +24,7 @@ static int queue_iommu_command(struct am { uint32_t tail, head; - tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE; + tail = iommu->cmd_buffer.tail + sizeof(cmd_entry_t); if ( tail == iommu->cmd_buffer.size ) tail = 0; @@ -33,7 +33,7 @@ static int queue_iommu_command(struct am if ( head != tail ) { memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, - cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE); + cmd, sizeof(cmd_entry_t)); iommu->cmd_buffer.tail = tail; return 1; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -118,7 +118,7 @@ static void register_iommu_cmd_buffer_in writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET); power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.size) + - IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE; + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER; entry = 0; iommu_set_addr_hi_to_reg(&entry, addr_hi); @@ -1022,9 +1022,31 @@ static void *__init allocate_ring_buffer static void * __init allocate_cmd_buffer(struct amd_iommu *iommu) { /* allocate 'command buffer' in power of 2 increments of 4K */ + static unsigned int __read_mostly nr_ents; + + if ( !nr_ents ) + { + unsigned int order; + + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait command). There can be + * one such pair of requests pending per CPU. One extra entry is + * needed as the ring is considered full when there's only one entry + * left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= IOMMU_CMD_BUFFER_MAX_ENTRIES); + order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + IOMMU_CMD_BUFFER_ENTRY_ORDER); + nr_ents = 1u << (order + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER); + + AMD_IOMMU_DEBUG("using %u-entry cmd ring(s)\n", nr_ents); + } + + BUILD_BUG_ON(sizeof(cmd_entry_t) != (1u << IOMMU_CMD_BUFFER_ENTRY_ORDER)); + return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), - IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, - "Command Buffer", false); + nr_ents, "Command Buffer", false); } static void * __init allocate_event_log(struct amd_iommu *iommu)
xsa373/xsa373-4.14-3.patch
(application/octet-stream, 7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: eliminate flush related timeouts Leaving an in-progress operation pending when it appears to take too long is problematic: If e.g. a QI command completed later, the write to the "poll slot" may instead be understood to signal a subsequently started command's completion. Also our accounting of the timeout period was actually wrong: We included the time it took for the command to actually make it to the front of the queue, which could be heavily affected by guests other than the one for which the flush is being performed. Do away with all timeout detection on all flush related code paths. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Additionally log (once) if qinval_next_index() didn't immediately find an available slot. Together with the earlier change sizing the queue(s) dynamically, we should now have a guarantee that with our fully synchronous model any demand for slots can actually be satisfied. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,6 +127,34 @@ do { } \ } while (0) +#define IOMMU_FLUSH_WAIT(what, iommu, offset, op, cond, sts) \ +do { \ + static unsigned int __read_mostly threshold = 1; \ + s_time_t start = NOW(); \ + s_time_t timeout = start + DMAR_OPERATION_TIMEOUT * threshold; \ + \ + for ( ; ; ) \ + { \ + sts = op(iommu->reg, offset); \ + if ( cond ) \ + break; \ + if ( timeout && NOW() > timeout ) \ + { \ + threshold |= threshold << 1; \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush taking too long\n", \ + iommu->index, what); \ + timeout = 0; \ + } \ + cpu_relax(); \ + } \ + \ + if ( !timeout ) \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush took %lums\n", \ + iommu->index, what, (NOW() - start) / 10000000); \ +} while ( false ) + int vtd_hw_check(void); void disable_pmr(struct vtd_iommu *iommu); int is_igd_drhd(struct acpi_drhd_unit *drhd); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -326,8 +326,8 @@ static void iommu_flush_write_buffer(str dmar_writel(iommu->reg, DMAR_GCMD_REG, val | DMA_GCMD_WBF); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, dmar_readl, - !(val & DMA_GSTS_WBFS), val); + IOMMU_FLUSH_WAIT("write buffer", iommu, DMAR_GSTS_REG, dmar_readl, + !(val & DMA_GSTS_WBFS), val); spin_unlock_irqrestore(&iommu->register_lock, flags); } @@ -376,8 +376,8 @@ int vtd_flush_context_reg(struct vtd_iom dmar_writeq(iommu->reg, DMAR_CCMD_REG, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_CCMD_REG, dmar_readq, - !(val & DMA_CCMD_ICC), val); + IOMMU_FLUSH_WAIT("context", iommu, DMAR_CCMD_REG, dmar_readq, + !(val & DMA_CCMD_ICC), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* flush context entry will implicitly flush write buffer */ @@ -454,8 +454,8 @@ int vtd_flush_iotlb_reg(struct vtd_iommu dmar_writeq(iommu->reg, tlb_offset + 8, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, (tlb_offset + 8), dmar_readq, - !(val & DMA_TLB_IVT), val); + IOMMU_FLUSH_WAIT("iotlb", iommu, (tlb_offset + 8), dmar_readq, + !(val & DMA_TLB_IVT), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* check IOTLB invalidation granularity */ --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -29,8 +29,6 @@ #include "extern.h" #include "../ats.h" -#define VTD_QI_TIMEOUT 1 - static unsigned int __read_mostly qi_pg_order; static unsigned int __read_mostly qi_entry_nr; @@ -60,7 +58,11 @@ static unsigned int qinval_next_index(st /* (tail+1 == head) indicates a full queue, wait for HW */ while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) + { + printk_once(XENLOG_ERR VTDPREFIX " IOMMU#%u: no QI slot available\n", + iommu->index); cpu_relax(); + } return tail; } @@ -180,23 +182,32 @@ static int __must_check queue_invalidate /* Now we don't support interrupt method */ if ( sw ) { - s_time_t timeout; - - /* In case all wait descriptor writes to same addr with same data */ - timeout = NOW() + MILLISECS(flush_dev_iotlb ? - iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT); + static unsigned int __read_mostly threshold = 1; + s_time_t start = NOW(); + s_time_t timeout = start + (flush_dev_iotlb + ? iommu_dev_iotlb_timeout + : 100) * MILLISECS(threshold); while ( ACCESS_ONCE(*this_poll_slot) != QINVAL_STAT_DONE ) { - if ( NOW() > timeout ) + if ( timeout && NOW() > timeout ) { - print_qi_regs(iommu); + threshold |= threshold << 1; printk(XENLOG_WARNING VTDPREFIX - " Queue invalidate wait descriptor timed out\n"); - return -ETIMEDOUT; + " IOMMU#%u: QI%s wait descriptor taking too long\n", + iommu->index, flush_dev_iotlb ? " dev" : ""); + print_qi_regs(iommu); + timeout = 0; } cpu_relax(); } + + if ( !timeout ) + printk(XENLOG_WARNING VTDPREFIX + " IOMMU#%u: QI%s wait descriptor took %lums\n", + iommu->index, flush_dev_iotlb ? " dev" : "", + (NOW() - start) / 10000000); + return 0; }
xsa373/xsa373-4.14-4.patch
(application/octet-stream, 2.7 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: wait for command slot to be available No caller cared about send_iommu_command() indicating unavailability of a slot. Hence if a sufficient number prior commands timed out, we did blindly assume that the requested command was submitted to the IOMMU when really it wasn't. This could mean both a hanging system (waiting for a command to complete that was never seen by the IOMMU) or blindly propagating success back to callers, making them believe they're fine to e.g. free previously unmapped pages. Fold the three involved functions into one, add spin waiting for an available slot along the lines of VT-d's qinval_next_index(), and as a consequence drop all error indicator return types/values. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -20,43 +20,32 @@ #include "iommu.h" #include "../ats.h" -static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[]) +static void send_iommu_command(struct amd_iommu *iommu, + const uint32_t cmd[4]) { - uint32_t tail, head; + uint32_t tail; tail = iommu->cmd_buffer.tail + sizeof(cmd_entry_t); if ( tail == iommu->cmd_buffer.size ) tail = 0; - head = readl(iommu->mmio_base + - IOMMU_CMD_BUFFER_HEAD_OFFSET) & IOMMU_RING_BUFFER_PTR_MASK; - if ( head != tail ) + while ( tail == (readl(iommu->mmio_base + + IOMMU_CMD_BUFFER_HEAD_OFFSET) & + IOMMU_RING_BUFFER_PTR_MASK) ) { - memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, - cmd, sizeof(cmd_entry_t)); - - iommu->cmd_buffer.tail = tail; - return 1; + printk_once(XENLOG_ERR + "AMD IOMMU %04x:%02x:%02x.%u: no cmd slot available\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf)); + cpu_relax(); } - return 0; -} - -static void commit_iommu_command_buffer(struct amd_iommu *iommu) -{ - writel(iommu->cmd_buffer.tail, - iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); -} + memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, + cmd, sizeof(cmd_entry_t)); -static int send_iommu_command(struct amd_iommu *iommu, u32 cmd[]) -{ - if ( queue_iommu_command(iommu, cmd) ) - { - commit_iommu_command_buffer(iommu); - return 1; - } + iommu->cmd_buffer.tail = tail; - return 0; + writel(tail, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); } static void flush_command_buffer(struct amd_iommu *iommu)
xsa373/xsa373-4.14-5.patch
(application/octet-stream, 5.3 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: drop command completion timeout First and foremost - such timeouts were not signaled to callers, making them believe they're fine to e.g. free previously unmapped pages. Mirror VT-d's behavior: A fixed number of loop iterations is not a suitable way to detect timeouts in an environment (CPU and bus speeds) independent manner anyway. Furthermore, leaving an in-progress operation pending when it appears to take too long is problematic: If a command completed later, the signaling of its completion may instead be understood to signal a subsequently started command's completion. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Allow callers to specify a non-default timeout bias for this logging, using the same values as VT-d does, which in particular means a (by default) much larger value for device IO TLB invalidation. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -48,10 +48,12 @@ static void send_iommu_command(struct am writel(tail, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); } -static void flush_command_buffer(struct amd_iommu *iommu) +static void flush_command_buffer(struct amd_iommu *iommu, + unsigned int timeout_base) { - unsigned int cmd[4], status, loop_count; - bool comp_wait; + uint32_t cmd[4]; + s_time_t start, timeout; + static unsigned int __read_mostly threshold = 1; /* RW1C 'ComWaitInt' in status register */ writel(IOMMU_STATUS_COMP_WAIT_INT, @@ -67,22 +69,31 @@ static void flush_command_buffer(struct IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]); send_iommu_command(iommu, cmd); - /* Make loop_count long enough for polling completion wait bit */ - loop_count = 1000; - do { - status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT; - --loop_count; - } while ( !comp_wait && loop_count ); - - if ( comp_wait ) + start = NOW(); + timeout = start + (timeout_base ?: 100) * MILLISECS(threshold); + while ( !(readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET) & + IOMMU_STATUS_COMP_WAIT_INT) ) { - /* RW1C 'ComWaitInt' in status register */ - writel(IOMMU_STATUS_COMP_WAIT_INT, - iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - return; + if ( timeout && NOW() > timeout ) + { + threshold |= threshold << 1; + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait taking too long\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : ""); + timeout = 0; + } + cpu_relax(); } - AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n"); + + if ( !timeout ) + printk(XENLOG_WARNING + "AMD IOMMU %04x:%02x:%02x.%u: %scompletion wait took %lums\n", + iommu->seg, PCI_BUS(iommu->bdf), + PCI_SLOT(iommu->bdf), PCI_FUNC(iommu->bdf), + timeout_base ? "iotlb " : "", + (NOW() - start) / 10000000); } /* Build low level iommu command messages */ @@ -294,7 +305,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con /* send INVALIDATE_IOTLB_PAGES command */ spin_lock_irqsave(&iommu->lock, flags); invalidate_iotlb_pages(iommu, maxpend, 0, queueid, daddr, req_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, iommu_dev_iotlb_timeout); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -331,7 +342,7 @@ static void _amd_iommu_flush_pages(struc { spin_lock_irqsave(&iommu->lock, flags); invalidate_iommu_pages(iommu, daddr, dom_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -355,7 +366,7 @@ void amd_iommu_flush_device(struct amd_i ASSERT( spin_is_locked(&iommu->lock) ); invalidate_dev_table_entry(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf) @@ -363,7 +374,7 @@ void amd_iommu_flush_intremap(struct amd ASSERT( spin_is_locked(&iommu->lock) ); invalidate_interrupt_table(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_all_caches(struct amd_iommu *iommu) @@ -371,7 +382,7 @@ void amd_iommu_flush_all_caches(struct a ASSERT( spin_is_locked(&iommu->lock) ); invalidate_iommu_all(iommu); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) @@ -381,7 +392,8 @@ void amd_iommu_send_guest_cmd(struct amd spin_lock_irqsave(&iommu->lock, flags); send_iommu_command(iommu, cmd); - flush_command_buffer(iommu); + /* TBD: Timeout selection may require peeking into cmd[]. */ + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); }
xsa373/xsa373-4.15-1.patch
(application/octet-stream, 4.7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: size qinval queue dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait descriptor). There can be one such pair of requests pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the queue ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/iommu.h +++ b/xen/drivers/passthrough/vtd/iommu.h @@ -450,17 +450,9 @@ struct qinval_entry { }q; }; -/* Order of queue invalidation pages(max is 8) */ -#define QINVAL_PAGE_ORDER 2 - -#define QINVAL_ARCH_PAGE_ORDER (QINVAL_PAGE_ORDER + PAGE_SHIFT_4K - PAGE_SHIFT) -#define QINVAL_ARCH_PAGE_NR ( QINVAL_ARCH_PAGE_ORDER < 0 ? \ - 1 : \ - 1 << QINVAL_ARCH_PAGE_ORDER ) - /* Each entry is 16 bytes, so 2^8 entries per page */ #define QINVAL_ENTRY_ORDER ( PAGE_SHIFT - 4 ) -#define QINVAL_ENTRY_NR (1 << (QINVAL_PAGE_ORDER + 8)) +#define QINVAL_MAX_ENTRY_NR (1u << (7 + QINVAL_ENTRY_ORDER)) /* Status data flag */ #define QINVAL_STAT_INIT 0 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -31,6 +31,9 @@ #define VTD_QI_TIMEOUT 1 +static unsigned int __read_mostly qi_pg_order; +static unsigned int __read_mostly qi_entry_nr; + static int __must_check invalidate_sync(struct vtd_iommu *iommu); static void print_qi_regs(struct vtd_iommu *iommu) @@ -55,7 +58,7 @@ static unsigned int qinval_next_index(st tail >>= QINVAL_INDEX_SHIFT; /* (tail+1 == head) indicates a full queue, wait for HW */ - while ( ( tail + 1 ) % QINVAL_ENTRY_NR == + while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) cpu_relax(); @@ -68,7 +71,7 @@ static void qinval_update_qtail(struct v /* Need hold register lock when update tail */ ASSERT( spin_is_locked(&iommu->register_lock) ); - val = (index + 1) % QINVAL_ENTRY_NR; + val = (index + 1) & (qi_entry_nr - 1); dmar_writeq(iommu->reg, DMAR_IQT_REG, (val << QINVAL_INDEX_SHIFT)); } @@ -403,8 +406,28 @@ int enable_qinval(struct vtd_iommu *iomm if ( iommu->qinval_maddr == 0 ) { - iommu->qinval_maddr = alloc_pgtable_maddr(QINVAL_ARCH_PAGE_NR, - iommu->node); + if ( !qi_entry_nr ) + { + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait descriptor). There + * can be one such pair of requests pending per CPU. One extra + * entry is needed as the ring is considered full when there's + * only one entry left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= QINVAL_MAX_ENTRY_NR); + qi_pg_order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + (PAGE_SHIFT - + QINVAL_ENTRY_ORDER)); + qi_entry_nr = 1u << (qi_pg_order + QINVAL_ENTRY_ORDER); + + dprintk(XENLOG_INFO VTDPREFIX, + "QI: using %u-entry ring(s)\n", qi_entry_nr); + } + + iommu->qinval_maddr = + alloc_pgtable_maddr(qi_entry_nr >> QINVAL_ENTRY_ORDER, + iommu->node); if ( iommu->qinval_maddr == 0 ) { dprintk(XENLOG_WARNING VTDPREFIX, @@ -418,15 +441,16 @@ int enable_qinval(struct vtd_iommu *iomm spin_lock_irqsave(&iommu->register_lock, flags); - /* Setup Invalidation Queue Address(IQA) register with the - * address of the page we just allocated. QS field at - * bits[2:0] to indicate size of queue is one 4KB page. - * That's 256 entries. Queued Head (IQH) and Queue Tail (IQT) - * registers are automatically reset to 0 with write - * to IQA register. + /* + * Setup Invalidation Queue Address (IQA) register with the address of the + * pages we just allocated. The QS field at bits[2:0] indicates the size + * (page order) of the queue. + * + * Queued Head (IQH) and Queue Tail (IQT) registers are automatically + * reset to 0 with write to IQA register. */ dmar_writeq(iommu->reg, DMAR_IQA_REG, - iommu->qinval_maddr | QINVAL_PAGE_ORDER); + iommu->qinval_maddr | qi_pg_order); dmar_writeq(iommu->reg, DMAR_IQT_REG, 0);
xsa373/xsa373-4.15-2.patch
(application/octet-stream, 3.9 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: size command buffer dynamically With the present synchronous model, we need two slots for every operation (the operation itself and a wait command). There can be one such pair of commands pending per CPU. To ensure that under all normal circumstances a slot is always available when one is requested, size the command ring according to the number of present CPUs. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu-defs.h +++ b/xen/drivers/passthrough/amd/iommu-defs.h @@ -20,9 +20,6 @@ #ifndef AMD_IOMMU_DEFS_H #define AMD_IOMMU_DEFS_H -/* IOMMU Command Buffer entries: in power of 2 increments, minimum of 256 */ -#define IOMMU_CMD_BUFFER_DEFAULT_ENTRIES 512 - /* IOMMU Event Log entries: in power of 2 increments, minimum of 256 */ #define IOMMU_EVENT_LOG_DEFAULT_ENTRIES 512 @@ -164,8 +161,8 @@ struct amd_iommu_dte { #define IOMMU_CMD_BUFFER_LENGTH_MASK 0x0F000000 #define IOMMU_CMD_BUFFER_LENGTH_SHIFT 24 -#define IOMMU_CMD_BUFFER_ENTRY_SIZE 16 -#define IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE 8 +#define IOMMU_CMD_BUFFER_ENTRY_ORDER 4 +#define IOMMU_CMD_BUFFER_MAX_ENTRIES (1u << 15) #define IOMMU_CMD_OPCODE_MASK 0xF0000000 #define IOMMU_CMD_OPCODE_SHIFT 28 --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -24,7 +24,7 @@ static int queue_iommu_command(struct am { uint32_t tail, head; - tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE; + tail = iommu->cmd_buffer.tail + sizeof(cmd_entry_t); if ( tail == iommu->cmd_buffer.size ) tail = 0; @@ -33,7 +33,7 @@ static int queue_iommu_command(struct am if ( head != tail ) { memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, - cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE); + cmd, sizeof(cmd_entry_t)); iommu->cmd_buffer.tail = tail; return 1; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -118,7 +118,7 @@ static void register_iommu_cmd_buffer_in writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET); power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.size) + - IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE; + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER; entry = 0; iommu_set_addr_hi_to_reg(&entry, addr_hi); @@ -1018,9 +1018,31 @@ static void *__init allocate_ring_buffer static void * __init allocate_cmd_buffer(struct amd_iommu *iommu) { /* allocate 'command buffer' in power of 2 increments of 4K */ + static unsigned int __read_mostly nr_ents; + + if ( !nr_ents ) + { + unsigned int order; + + /* + * With the present synchronous model, we need two slots for every + * operation (the operation itself and a wait command). There can be + * one such pair of requests pending per CPU. One extra entry is + * needed as the ring is considered full when there's only one entry + * left. + */ + BUILD_BUG_ON(CONFIG_NR_CPUS * 2 >= IOMMU_CMD_BUFFER_MAX_ENTRIES); + order = get_order_from_bytes((num_present_cpus() * 2 + 1) << + IOMMU_CMD_BUFFER_ENTRY_ORDER); + nr_ents = 1u << (order + PAGE_SHIFT - IOMMU_CMD_BUFFER_ENTRY_ORDER); + + AMD_IOMMU_DEBUG("using %u-entry cmd ring(s)\n", nr_ents); + } + + BUILD_BUG_ON(sizeof(cmd_entry_t) != (1u << IOMMU_CMD_BUFFER_ENTRY_ORDER)); + return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), - IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, - "Command Buffer", false); + nr_ents, "Command Buffer", false); } static void * __init allocate_event_log(struct amd_iommu *iommu)
xsa373/xsa373-4.15-3.patch
(application/octet-stream, 7 KB)
From: Jan Beulich <[email protected]> Subject: VT-d: eliminate flush related timeouts Leaving an in-progress operation pending when it appears to take too long is problematic: If e.g. a QI command completed later, the write to the "poll slot" may instead be understood to signal a subsequently started command's completion. Also our accounting of the timeout period was actually wrong: We included the time it took for the command to actually make it to the front of the queue, which could be heavily affected by guests other than the one for which the flush is being performed. Do away with all timeout detection on all flush related code paths. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Additionally log (once) if qinval_next_index() didn't immediately find an available slot. Together with the earlier change sizing the queue(s) dynamically, we should now have a guarantee that with our fully synchronous model any demand for slots can actually be satisfied. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,6 +127,34 @@ do { } \ } while (0) +#define IOMMU_FLUSH_WAIT(what, iommu, offset, op, cond, sts) \ +do { \ + static unsigned int __read_mostly threshold = 1; \ + s_time_t start = NOW(); \ + s_time_t timeout = start + DMAR_OPERATION_TIMEOUT * threshold; \ + \ + for ( ; ; ) \ + { \ + sts = op(iommu->reg, offset); \ + if ( cond ) \ + break; \ + if ( timeout && NOW() > timeout ) \ + { \ + threshold |= threshold << 1; \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush taking too long\n", \ + iommu->index, what); \ + timeout = 0; \ + } \ + cpu_relax(); \ + } \ + \ + if ( !timeout ) \ + printk(XENLOG_WARNING VTDPREFIX \ + " IOMMU#%u: %s flush took %lums\n", \ + iommu->index, what, (NOW() - start) / 10000000); \ +} while ( false ) + int vtd_hw_check(void); void disable_pmr(struct vtd_iommu *iommu); int is_igd_drhd(struct acpi_drhd_unit *drhd); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -373,8 +373,8 @@ static void iommu_flush_write_buffer(str dmar_writel(iommu->reg, DMAR_GCMD_REG, val | DMA_GCMD_WBF); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_GSTS_REG, dmar_readl, - !(val & DMA_GSTS_WBFS), val); + IOMMU_FLUSH_WAIT("write buffer", iommu, DMAR_GSTS_REG, dmar_readl, + !(val & DMA_GSTS_WBFS), val); spin_unlock_irqrestore(&iommu->register_lock, flags); } @@ -423,8 +423,8 @@ int vtd_flush_context_reg(struct vtd_iom dmar_writeq(iommu->reg, DMAR_CCMD_REG, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, DMAR_CCMD_REG, dmar_readq, - !(val & DMA_CCMD_ICC), val); + IOMMU_FLUSH_WAIT("context", iommu, DMAR_CCMD_REG, dmar_readq, + !(val & DMA_CCMD_ICC), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* flush context entry will implicitly flush write buffer */ @@ -501,8 +501,8 @@ int vtd_flush_iotlb_reg(struct vtd_iommu dmar_writeq(iommu->reg, tlb_offset + 8, val); /* Make sure hardware complete it */ - IOMMU_WAIT_OP(iommu, (tlb_offset + 8), dmar_readq, - !(val & DMA_TLB_IVT), val); + IOMMU_FLUSH_WAIT("iotlb", iommu, (tlb_offset + 8), dmar_readq, + !(val & DMA_TLB_IVT), val); spin_unlock_irqrestore(&iommu->register_lock, flags); /* check IOTLB invalidation granularity */ --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -29,8 +29,6 @@ #include "extern.h" #include "../ats.h" -#define VTD_QI_TIMEOUT 1 - static unsigned int __read_mostly qi_pg_order; static unsigned int __read_mostly qi_entry_nr; @@ -60,7 +58,11 @@ static unsigned int qinval_next_index(st /* (tail+1 == head) indicates a full queue, wait for HW */ while ( ((tail + 1) & (qi_entry_nr - 1)) == ( dmar_readq(iommu->reg, DMAR_IQH_REG) >> QINVAL_INDEX_SHIFT ) ) + { + printk_once(XENLOG_ERR VTDPREFIX " IOMMU#%u: no QI slot available\n", + iommu->index); cpu_relax(); + } return tail; } @@ -180,23 +182,32 @@ static int __must_check queue_invalidate /* Now we don't support interrupt method */ if ( sw ) { - s_time_t timeout; - - /* In case all wait descriptor writes to same addr with same data */ - timeout = NOW() + MILLISECS(flush_dev_iotlb ? - iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT); + static unsigned int __read_mostly threshold = 1; + s_time_t start = NOW(); + s_time_t timeout = start + (flush_dev_iotlb + ? iommu_dev_iotlb_timeout + : 100) * MILLISECS(threshold); while ( ACCESS_ONCE(*this_poll_slot) != QINVAL_STAT_DONE ) { - if ( NOW() > timeout ) + if ( timeout && NOW() > timeout ) { - print_qi_regs(iommu); + threshold |= threshold << 1; printk(XENLOG_WARNING VTDPREFIX - " Queue invalidate wait descriptor timed out\n"); - return -ETIMEDOUT; + " IOMMU#%u: QI%s wait descriptor taking too long\n", + iommu->index, flush_dev_iotlb ? " dev" : ""); + print_qi_regs(iommu); + timeout = 0; } cpu_relax(); } + + if ( !timeout ) + printk(XENLOG_WARNING VTDPREFIX + " IOMMU#%u: QI%s wait descriptor took %lums\n", + iommu->index, flush_dev_iotlb ? " dev" : "", + (NOW() - start) / 10000000); + return 0; }
xsa373/xsa373-4.15-4.patch
(application/octet-stream, 2.6 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: wait for command slot to be available No caller cared about send_iommu_command() indicating unavailability of a slot. Hence if a sufficient number prior commands timed out, we did blindly assume that the requested command was submitted to the IOMMU when really it wasn't. This could mean both a hanging system (waiting for a command to complete that was never seen by the IOMMU) or blindly propagating success back to callers, making them believe they're fine to e.g. free previously unmapped pages. Fold the three involved functions into one, add spin waiting for an available slot along the lines of VT-d's qinval_next_index(), and as a consequence drop all error indicator return types/values. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -20,43 +20,30 @@ #include "iommu.h" #include "../ats.h" -static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[]) +static void send_iommu_command(struct amd_iommu *iommu, + const uint32_t cmd[4]) { - uint32_t tail, head; + uint32_t tail; tail = iommu->cmd_buffer.tail + sizeof(cmd_entry_t); if ( tail == iommu->cmd_buffer.size ) tail = 0; - head = readl(iommu->mmio_base + - IOMMU_CMD_BUFFER_HEAD_OFFSET) & IOMMU_RING_BUFFER_PTR_MASK; - if ( head != tail ) + while ( tail == (readl(iommu->mmio_base + + IOMMU_CMD_BUFFER_HEAD_OFFSET) & + IOMMU_RING_BUFFER_PTR_MASK) ) { - memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, - cmd, sizeof(cmd_entry_t)); - - iommu->cmd_buffer.tail = tail; - return 1; + printk_once(XENLOG_ERR "AMD IOMMU %pp: no cmd slot available\n", + &PCI_SBDF2(iommu->seg, iommu->bdf)); + cpu_relax(); } - return 0; -} - -static void commit_iommu_command_buffer(struct amd_iommu *iommu) -{ - writel(iommu->cmd_buffer.tail, - iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); -} + memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, + cmd, sizeof(cmd_entry_t)); -static int send_iommu_command(struct amd_iommu *iommu, u32 cmd[]) -{ - if ( queue_iommu_command(iommu, cmd) ) - { - commit_iommu_command_buffer(iommu); - return 1; - } + iommu->cmd_buffer.tail = tail; - return 0; + writel(tail, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); } static void flush_command_buffer(struct amd_iommu *iommu)
xsa373/xsa373-4.15-5.patch
(application/octet-stream, 5.2 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: drop command completion timeout First and foremost - such timeouts were not signaled to callers, making them believe they're fine to e.g. free previously unmapped pages. Mirror VT-d's behavior: A fixed number of loop iterations is not a suitable way to detect timeouts in an environment (CPU and bus speeds) independent manner anyway. Furthermore, leaving an in-progress operation pending when it appears to take too long is problematic: If a command completed later, the signaling of its completion may instead be understood to signal a subsequently started command's completion. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Allow callers to specify a non-default timeout bias for this logging, using the same values as VT-d does, which in particular means a (by default) much larger value for device IO TLB invalidation. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -46,10 +46,12 @@ static void send_iommu_command(struct am writel(tail, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); } -static void flush_command_buffer(struct amd_iommu *iommu) +static void flush_command_buffer(struct amd_iommu *iommu, + unsigned int timeout_base) { - unsigned int cmd[4], status, loop_count; - bool comp_wait; + uint32_t cmd[4]; + s_time_t start, timeout; + static unsigned int __read_mostly threshold = 1; /* RW1C 'ComWaitInt' in status register */ writel(IOMMU_STATUS_COMP_WAIT_INT, @@ -65,22 +67,29 @@ static void flush_command_buffer(struct IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]); send_iommu_command(iommu, cmd); - /* Make loop_count long enough for polling completion wait bit */ - loop_count = 1000; - do { - status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT; - --loop_count; - } while ( !comp_wait && loop_count ); - - if ( comp_wait ) + start = NOW(); + timeout = start + (timeout_base ?: 100) * MILLISECS(threshold); + while ( !(readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET) & + IOMMU_STATUS_COMP_WAIT_INT) ) { - /* RW1C 'ComWaitInt' in status register */ - writel(IOMMU_STATUS_COMP_WAIT_INT, - iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - return; + if ( timeout && NOW() > timeout ) + { + threshold |= threshold << 1; + printk(XENLOG_WARNING + "AMD IOMMU %pp: %scompletion wait taking too long\n", + &PCI_SBDF2(iommu->seg, iommu->bdf), + timeout_base ? "iotlb " : ""); + timeout = 0; + } + cpu_relax(); } - AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n"); + + if ( !timeout ) + printk(XENLOG_WARNING + "AMD IOMMU %pp: %scompletion wait took %lums\n", + &PCI_SBDF2(iommu->seg, iommu->bdf), + timeout_base ? "iotlb " : "", + (NOW() - start) / 10000000); } /* Build low level iommu command messages */ @@ -291,7 +300,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con /* send INVALIDATE_IOTLB_PAGES command */ spin_lock_irqsave(&iommu->lock, flags); invalidate_iotlb_pages(iommu, maxpend, 0, queueid, daddr, req_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, iommu_dev_iotlb_timeout); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -328,7 +337,7 @@ static void _amd_iommu_flush_pages(struc { spin_lock_irqsave(&iommu->lock, flags); invalidate_iommu_pages(iommu, daddr, dom_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -352,7 +361,7 @@ void amd_iommu_flush_device(struct amd_i ASSERT( spin_is_locked(&iommu->lock) ); invalidate_dev_table_entry(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf) @@ -360,7 +369,7 @@ void amd_iommu_flush_intremap(struct amd ASSERT( spin_is_locked(&iommu->lock) ); invalidate_interrupt_table(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_all_caches(struct amd_iommu *iommu) @@ -368,7 +377,7 @@ void amd_iommu_flush_all_caches(struct a ASSERT( spin_is_locked(&iommu->lock) ); invalidate_iommu_all(iommu); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) @@ -378,7 +387,8 @@ void amd_iommu_send_guest_cmd(struct amd spin_lock_irqsave(&iommu->lock, flags); send_iommu_command(iommu, cmd); - flush_command_buffer(iommu); + /* TBD: Timeout selection may require peeking into cmd[]. */ + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); }
xsa373/xsa373-4.patch
(application/octet-stream, 2.6 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: wait for command slot to be available No caller cared about send_iommu_command() indicating unavailability of a slot. Hence if a sufficient number prior commands timed out, we did blindly assume that the requested command was submitted to the IOMMU when really it wasn't. This could mean both a hanging system (waiting for a command to complete that was never seen by the IOMMU) or blindly propagating success back to callers, making them believe they're fine to e.g. free previously unmapped pages. Fold the three involved functions into one, add spin waiting for an available slot along the lines of VT-d's qinval_next_index(), and as a consequence drop all error indicator return types/values. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -20,43 +20,30 @@ #include "iommu.h" #include "../ats.h" -static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[]) +static void send_iommu_command(struct amd_iommu *iommu, + const uint32_t cmd[4]) { - uint32_t tail, head; + uint32_t tail; tail = iommu->cmd_buffer.tail + sizeof(cmd_entry_t); if ( tail == iommu->cmd_buffer.size ) tail = 0; - head = readl(iommu->mmio_base + - IOMMU_CMD_BUFFER_HEAD_OFFSET) & IOMMU_RING_BUFFER_PTR_MASK; - if ( head != tail ) + while ( tail == (readl(iommu->mmio_base + + IOMMU_CMD_BUFFER_HEAD_OFFSET) & + IOMMU_RING_BUFFER_PTR_MASK) ) { - memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, - cmd, sizeof(cmd_entry_t)); - - iommu->cmd_buffer.tail = tail; - return 1; + printk_once(XENLOG_ERR "AMD IOMMU %pp: no cmd slot available\n", + &PCI_SBDF2(iommu->seg, iommu->bdf)); + cpu_relax(); } - return 0; -} - -static void commit_iommu_command_buffer(struct amd_iommu *iommu) -{ - writel(iommu->cmd_buffer.tail, - iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); -} + memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail, + cmd, sizeof(cmd_entry_t)); -static int send_iommu_command(struct amd_iommu *iommu, u32 cmd[]) -{ - if ( queue_iommu_command(iommu, cmd) ) - { - commit_iommu_command_buffer(iommu); - return 1; - } + iommu->cmd_buffer.tail = tail; - return 0; + writel(tail, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); } static void flush_command_buffer(struct amd_iommu *iommu)
xsa373/xsa373-5.patch
(application/octet-stream, 5.5 KB)
From: Jan Beulich <[email protected]> Subject: AMD/IOMMU: drop command completion timeout First and foremost - such timeouts were not signaled to callers, making them believe they're fine to e.g. free previously unmapped pages. Mirror VT-d's behavior: A fixed number of loop iterations is not a suitable way to detect timeouts in an environment (CPU and bus speeds) independent manner anyway. Furthermore, leaving an in-progress operation pending when it appears to take too long is problematic: If a command completed later, the signaling of its completion may instead be understood to signal a subsequently started command's completion. Log excessively long processing times (with a progressive threshold) to have some indication of problems in this area. Allow callers to specify a non-default timeout bias for this logging, using the same values as VT-d does, which in particular means a (by default) much larger value for device IO TLB invalidation. This is part of XSA-373 / CVE-2021-28692. Signed-off-by: Jan Beulich <[email protected]> Reviewed-by: Paul Durrant <[email protected]> --- v2: Avoid leading blanks in log messages. --- TBD: As long as the spinning happens under lock, 100ms may be too large a granularity. But I've left it similar to VT-d as subsequently I'm intending to re-work the locking such that the spinning will start only after dropping the lock, like VT-d QI also does. --- a/xen/drivers/passthrough/amd/iommu_cmd.c +++ b/xen/drivers/passthrough/amd/iommu_cmd.c @@ -46,10 +46,12 @@ static void send_iommu_command(struct am writel(tail, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET); } -static void flush_command_buffer(struct amd_iommu *iommu) +static void flush_command_buffer(struct amd_iommu *iommu, + unsigned int timeout_base) { - unsigned int cmd[4], status, loop_count; - bool comp_wait; + uint32_t cmd[4]; + s_time_t start, timeout; + static unsigned int __read_mostly threshold = 1; /* RW1C 'ComWaitInt' in status register */ writel(IOMMU_STATUS_COMP_WAIT_INT, @@ -65,22 +67,29 @@ static void flush_command_buffer(struct IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]); send_iommu_command(iommu, cmd); - /* Make loop_count long enough for polling completion wait bit */ - loop_count = 1000; - do { - status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT; - --loop_count; - } while ( !comp_wait && loop_count ); - - if ( comp_wait ) + start = NOW(); + timeout = start + (timeout_base ?: 100) * MILLISECS(threshold); + while ( !(readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET) & + IOMMU_STATUS_COMP_WAIT_INT) ) { - /* RW1C 'ComWaitInt' in status register */ - writel(IOMMU_STATUS_COMP_WAIT_INT, - iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); - return; + if ( timeout && NOW() > timeout ) + { + threshold |= threshold << 1; + printk(XENLOG_WARNING + "AMD IOMMU %pp: %scompletion wait taking too long\n", + &PCI_SBDF2(iommu->seg, iommu->bdf), + timeout_base ? "iotlb " : ""); + timeout = 0; + } + cpu_relax(); } - AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n"); + + if ( !timeout ) + printk(XENLOG_WARNING + "AMD IOMMU %pp: %scompletion wait took %lums\n", + &PCI_SBDF2(iommu->seg, iommu->bdf), + timeout_base ? "iotlb " : "", + (NOW() - start) / 10000000); } /* Build low level iommu command messages */ @@ -291,7 +300,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con /* send INVALIDATE_IOTLB_PAGES command */ spin_lock_irqsave(&iommu->lock, flags); invalidate_iotlb_pages(iommu, maxpend, 0, queueid, daddr, req_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, iommu_dev_iotlb_timeout); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -328,7 +337,7 @@ static void _amd_iommu_flush_pages(struc { spin_lock_irqsave(&iommu->lock, flags); invalidate_iommu_pages(iommu, daddr, dom_id, order); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); } @@ -352,7 +361,7 @@ void amd_iommu_flush_device(struct amd_i ASSERT( spin_is_locked(&iommu->lock) ); invalidate_dev_table_entry(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf) @@ -360,7 +369,7 @@ void amd_iommu_flush_intremap(struct amd ASSERT( spin_is_locked(&iommu->lock) ); invalidate_interrupt_table(iommu, bdf); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_flush_all_caches(struct amd_iommu *iommu) @@ -368,7 +377,7 @@ void amd_iommu_flush_all_caches(struct a ASSERT( spin_is_locked(&iommu->lock) ); invalidate_iommu_all(iommu); - flush_command_buffer(iommu); + flush_command_buffer(iommu, 0); } void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]) @@ -378,7 +387,8 @@ void amd_iommu_send_guest_cmd(struct amd spin_lock_irqsave(&iommu->lock, flags); send_iommu_command(iommu, cmd); - flush_command_buffer(iommu); + /* TBD: Timeout selection may require peeking into cmd[]. */ + flush_command_buffer(iommu, 0); spin_unlock_irqrestore(&iommu->lock, flags); }