Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH] AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log()
@ 2020-02-14 18:55 Andrew Cooper
  2020-02-17 16:18 ` Jan Beulich
  0 siblings, 1 reply; 2+ messages in thread
From: Andrew Cooper @ 2020-02-14 18:55 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

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-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [Xen-devel] [PATCH] AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log()
  2020-02-14 18:55 [Xen-devel] [PATCH] AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log() Andrew Cooper
@ 2020-02-17 16:18 ` Jan Beulich
  0 siblings, 0 replies; 2+ messages in thread
From: Jan Beulich @ 2020-02-17 16:18 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné

On 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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 18:55 [Xen-devel] [PATCH] AMD/IOMMU: Common the #732/#733 errata handling in iommu_read_log() Andrew Cooper
2020-02-17 16:18 ` Jan Beulich

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git