There is no need to have both helpers implement the same workaround. The size
and layout of the the Event and PPR logs (and others for that matter) share a
lot of commonality.
Use MASK_EXTR() to locate the code field, and use ACCESS_ONCE() rather than
barrier() to prevent hoisting of the repeated read.
Avoid unnecessary zeroing by only clobbering the 'code' field - this alone is
sufficient to spot the errata when the rings wrap.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/drivers/passthrough/amd/iommu_init.c | 80 ++++++++++++--------------------
1 file changed, 29 insertions(+), 51 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index c42b608f07..5de5315d8b 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -300,7 +300,7 @@ static int iommu_read_log(struct amd_iommu *iommu,
unsigned int entry_size,
void (*parse_func)(struct amd_iommu *, u32 *))
{
- u32 tail, *entry, tail_offest, head_offset;
+ unsigned int tail, tail_offest, head_offset;
BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
@@ -319,11 +319,36 @@ static int iommu_read_log(struct amd_iommu *iommu,
while ( tail != log->head )
{
- /* read event log entry */
- entry = log->buffer + log->head;
+ uint32_t *entry = log->buffer + log->head;
+ unsigned int count = 0;
+
+ /* Event and PPR logs have their code field in the same position. */
+ unsigned int code = MASK_EXTR(entry[1], IOMMU_EVENT_CODE_MASK);
+
+ /*
+ * Workaround for errata #732, #733:
+ *
+ * It can happen that the tail pointer is updated before the actual
+ * entry got written. As suggested by RevGuide, we initialize the
+ * buffer to all zeros and clear entries after processing them.
+ */
+ while ( unlikely(code == 0) )
+ {
+ if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
+ {
+ AMD_IOMMU_DEBUG("AMD-Vi: No entry written to %s Log\n",
+ log == &iommu->event_log ? "Event" : "PPR");
+ return 0;
+ }
+ udelay(1);
+ code = MASK_EXTR(ACCESS_ONCE(entry[1]), IOMMU_EVENT_CODE_MASK);
+ }
parse_func(iommu, entry);
+ /* Clear 'code' to be able to spot the erratum when the ring wraps. */
+ ACCESS_ONCE(entry[1]) = 0;
+
log->head += entry_size;
if ( log->head == log->size )
log->head = 0;
@@ -503,7 +528,6 @@ static hw_irq_controller iommu_x2apic_type = {
static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
{
u32 code;
- int count = 0;
static const char *const event_str[] = {
#define EVENT_STR(name) [IOMMU_EVENT_##name - 1] = #name
EVENT_STR(ILLEGAL_DEV_TABLE_ENTRY),
@@ -521,25 +545,6 @@ static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
IOMMU_EVENT_CODE_SHIFT);
- /*
- * Workaround for erratum 732:
- * It can happen that the tail pointer is updated before the actual entry
- * got written. As suggested by RevGuide, we initialize the event log
- * buffer to all zeros and clear event log entries after processing them.
- */
- while ( code == 0 )
- {
- if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
- {
- AMD_IOMMU_DEBUG("AMD-Vi: No event written to log\n");
- return;
- }
- udelay(1);
- barrier(); /* Prevent hoisting of the entry[] read. */
- code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
- IOMMU_EVENT_CODE_SHIFT);
- }
-
/* Look up the symbolic name for code. */
if ( code <= ARRAY_SIZE(event_str) )
code_str = event_str[code - 1];
@@ -575,8 +580,6 @@ static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
else
printk(XENLOG_ERR "%s %08x %08x %08x %08x\n",
code_str, entry[0], entry[1], entry[2], entry[3]);
-
- memset(entry, 0, IOMMU_EVENT_LOG_ENTRY_SIZE);
}
static void iommu_check_event_log(struct amd_iommu *iommu)
@@ -627,31 +630,8 @@ void parse_ppr_log_entry(struct amd_iommu *iommu, u32 entry[])
{
u16 device_id;
- u8 bus, devfn, code;
+ u8 bus, devfn;
struct pci_dev *pdev;
- int count = 0;
-
- code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
- IOMMU_PPR_LOG_CODE_SHIFT);
-
- /*
- * Workaround for erratum 733:
- * It can happen that the tail pointer is updated before the actual entry
- * got written. As suggested by RevGuide, we initialize the event log
- * buffer to all zeros and clear ppr log entries after processing them.
- */
- while ( code == 0 )
- {
- if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
- {
- AMD_IOMMU_DEBUG("AMD-Vi: No ppr written to log\n");
- return;
- }
- udelay(1);
- barrier(); /* Prevent hoisting of the entry[] read. */
- code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
- IOMMU_PPR_LOG_CODE_SHIFT);
- }
/* here device_id is physical value */
device_id = iommu_get_devid_from_cmd(entry[0]);
@@ -664,8 +644,6 @@ void parse_ppr_log_entry(struct amd_iommu *iommu, u32 entry[])
if ( pdev )
guest_iommu_add_ppr_log(pdev->domain, entry);
-
- memset(entry, 0, IOMMU_PPR_LOG_ENTRY_SIZE);
}
static void iommu_check_ppr_log(struct amd_iommu *iommu)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 14.02.2020 19:55, Andrew Cooper wrote:
> There is no need to have both helpers implement the same workaround. The size
> and layout of the the Event and PPR logs (and others for that matter) share a
> lot of commonality.
>
> Use MASK_EXTR() to locate the code field, and use ACCESS_ONCE() rather than
> barrier() to prevent hoisting of the repeated read.
>
> Avoid unnecessary zeroing by only clobbering the 'code' field - this alone is
> sufficient to spot the errata when the rings wrap.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one remark / adjustment request:
> @@ -319,11 +319,36 @@ static int iommu_read_log(struct amd_iommu *iommu,
>
> while ( tail != log->head )
> {
> - /* read event log entry */
> - entry = log->buffer + log->head;
> + uint32_t *entry = log->buffer + log->head;
> + unsigned int count = 0;
> +
> + /* Event and PPR logs have their code field in the same position. */
> + unsigned int code = MASK_EXTR(entry[1], IOMMU_EVENT_CODE_MASK);
> +
> + /*
> + * Workaround for errata #732, #733:
> + *
> + * It can happen that the tail pointer is updated before the actual
> + * entry got written. As suggested by RevGuide, we initialize the
> + * buffer to all zeros and clear entries after processing them.
I don't think "clear entries" is applicable anymore with ...
> + */
> + while ( unlikely(code == 0) )
> + {
> + if ( unlikely(++count == IOMMU_LOG_ENTRY_TIMEOUT) )
> + {
> + AMD_IOMMU_DEBUG("AMD-Vi: No entry written to %s Log\n",
> + log == &iommu->event_log ? "Event" : "PPR");
> + return 0;
> + }
> + udelay(1);
> + code = MASK_EXTR(ACCESS_ONCE(entry[1]), IOMMU_EVENT_CODE_MASK);
> + }
>
> parse_func(iommu, entry);
>
> + /* Clear 'code' to be able to spot the erratum when the ring wraps. */
> + ACCESS_ONCE(entry[1]) = 0;
... this. Perhaps at least add "sufficiently"?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.