Xen devel - Apr 2013 - [PATCH 1/2] iommu/amd: Fix logic for clearing the IOMMU interrupt bits

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

The IOMMU interrupt bits in the IOMMU status registers are
cleared when writing 1.  Therefore, the existing logic which reads
the register, set the bit, and then writing back the values
could accidentally clear certain bits if it has been set.

The correct logic would just be writing only the value which only
set the interrupt bits, and leave the rest to zeros.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/iommu_init.c     |   12 ++++--------
 xen/include/asm-x86/hvm/svm/amd-iommu-defs.h |   13 ++++++++-----
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c
b/xen/drivers/passthrough/amd/iommu_init.c
index 73d9ce4..f1af9de 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -623,10 +623,8 @@ static void iommu_check_event_log(struct amd_iommu *iommu)
         iommu_reset_log(iommu, &iommu->event_log,
set_iommu_event_log_control);
 
     /* reset interrupt status bit */
-    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-    iommu_set_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
-
-    writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    writel(IOMMU_STATUS_EVENT_LOG_INT_MASK, 
+        iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
 
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
@@ -693,10 +691,8 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
         iommu_reset_log(iommu, &iommu->ppr_log,
set_iommu_ppr_log_control);
 
     /* reset interrupt status bit */
-    entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
-    iommu_set_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
-
-    writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
+    writel(IOMMU_STATUS_PPR_LOG_INT_MASK, 
+        iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
 
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
index d2176d0..3e161a5 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -385,19 +385,22 @@
 
 /* Status Register*/
 #define IOMMU_STATUS_MMIO_OFFSET		0x2020
-#define IOMMU_STATUS_EVENT_OVERFLOW_MASK	0x00000001
 #define IOMMU_STATUS_EVENT_OVERFLOW_SHIFT	0
-#define IOMMU_STATUS_EVENT_LOG_INT_MASK		0x00000002
+#define IOMMU_STATUS_EVENT_OVERFLOW_MASK	(1 <<
IOMMU_STATUS_EVENT_OVERFLOW_SHIFT)
 #define IOMMU_STATUS_EVENT_LOG_INT_SHIFT	1
-#define IOMMU_STATUS_COMP_WAIT_INT_MASK		0x00000004
+#define IOMMU_STATUS_EVENT_LOG_INT_MASK		(1 <<
IOMMU_STATUS_EVENT_LOG_INT_SHIFT)
 #define IOMMU_STATUS_COMP_WAIT_INT_SHIFT	2
-#define IOMMU_STATUS_EVENT_LOG_RUN_MASK		0x00000008
+#define IOMMU_STATUS_COMP_WAIT_INT_MASK		(1 <<
IOMMU_STATUS_COMP_WAIT_INT_SHIFT)
 #define IOMMU_STATUS_EVENT_LOG_RUN_SHIFT	3
-#define IOMMU_STATUS_CMD_BUFFER_RUN_MASK	0x00000010
+#define IOMMU_STATUS_EVENT_LOG_RUN_MASK		(1 <<
IOMMU_STATUS_EVENT_LOG_RUN_SHIFT)
 #define IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT	4
+#define IOMMU_STATUS_CMD_BUFFER_RUN_MASK	(1 <<
IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT)
 #define IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT     5
+#define IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK	(1 <<
IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT)
 #define IOMMU_STATUS_PPR_LOG_INT_SHIFT          6
+#define IOMMU_STATUS_PPR_LOG_INT_MASK           (1 <<
IOMMU_STATUS_PPR_LOG_INT_SHIFT)
 #define IOMMU_STATUS_PPR_LOG_RUN_SHIFT          7
+#define IOMMU_STATUS_PPR_LOG_RUN_MASK		(1 <<
IOMMU_STATUS_PPR_LOG_RUN_SHIFT)
 
 /* I/O Page Table */
 #define IOMMU_PAGE_TABLE_ENTRY_SIZE	8
-- 
1.7.10.4

>>> On 18.04.13 at 20:41, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -385,19 +385,22 @@
>  
>  /* Status Register*/
>  #define IOMMU_STATUS_MMIO_OFFSET		0x2020
> -#define IOMMU_STATUS_EVENT_OVERFLOW_MASK	0x00000001
>  #define IOMMU_STATUS_EVENT_OVERFLOW_SHIFT	0
> -#define IOMMU_STATUS_EVENT_LOG_INT_MASK		0x00000002
> +#define IOMMU_STATUS_EVENT_OVERFLOW_MASK	(1 << 
> IOMMU_STATUS_EVENT_OVERFLOW_SHIFT)
>  #define IOMMU_STATUS_EVENT_LOG_INT_SHIFT	1
> -#define IOMMU_STATUS_COMP_WAIT_INT_MASK		0x00000004
> +#define IOMMU_STATUS_EVENT_LOG_INT_MASK		(1 << 
> IOMMU_STATUS_EVENT_LOG_INT_SHIFT)
>  #define IOMMU_STATUS_COMP_WAIT_INT_SHIFT	2
> -#define IOMMU_STATUS_EVENT_LOG_RUN_MASK		0x00000008
> +#define IOMMU_STATUS_COMP_WAIT_INT_MASK		(1 << 
> IOMMU_STATUS_COMP_WAIT_INT_SHIFT)
>  #define IOMMU_STATUS_EVENT_LOG_RUN_SHIFT	3
> -#define IOMMU_STATUS_CMD_BUFFER_RUN_MASK	0x00000010
> +#define IOMMU_STATUS_EVENT_LOG_RUN_MASK		(1 << 
> IOMMU_STATUS_EVENT_LOG_RUN_SHIFT)
>  #define IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT	4
> +#define IOMMU_STATUS_CMD_BUFFER_RUN_MASK	(1 << 
> IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT)
>  #define IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT     5
> +#define IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK	(1 << 
> IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT)
>  #define IOMMU_STATUS_PPR_LOG_INT_SHIFT          6
> +#define IOMMU_STATUS_PPR_LOG_INT_MASK           (1 << 
> IOMMU_STATUS_PPR_LOG_INT_SHIFT)
>  #define IOMMU_STATUS_PPR_LOG_RUN_SHIFT          7
> +#define IOMMU_STATUS_PPR_LOG_RUN_MASK		(1 <<
IOMMU_STATUS_PPR_LOG_RUN_SHIFT)
I''ll take this as is, but mid term (post-4.3) most if not all of these
_SHIFT/_MASK pairs should be reduced to just one of them,
generally the _MASK one (as the shift value can be easily
reconstructed from the mask even when the mask is multiple bits
wide).

Jan

>>> On 18.04.13 at 20:41, <suravee.suthikulpanit@amd.com> wrote:
> The IOMMU interrupt bits in the IOMMU status registers are
> cleared when writing 1.  Therefore, the existing logic which reads
> the register, set the bit, and then writing back the values
> could accidentally clear certain bits if it has been set.
> 
> The correct logic would just be writing only the value which only
> set the interrupt bits, and leave the rest to zeros.
So looking through the status register accesses, I would assume
that guest_iommu_mmio_write64() is broken too? In that it
clearly doesn''t implement the RW1C behavior for the interrupt
and overflow bits?

Further (as indicated above), the overflow bits being RW1C too,
doesn''t iommu_reset_log() need a similar fix
(iommu_set_bit(&entry, of_bit) instead of
iommu_clear_bit(&entry, of_bit))?

And, perhaps even more importantly, iommu_interrupt_handler()
also ought to use iommu_set_bit()? Or wait - isn''t this touching
the wrong bits altogether? Logically I would expect interrupt
enable bits to be cleared here, i.e. IOMMU_CONTROL_* to be
used throughout this function instead of IOMMU_STATUS_*.

Jan

On 4/19/2013 3:18 AM, Jan Beulich wrote:
>>>> On 18.04.13 at 20:41, <suravee.suthikulpanit@amd.com>
wrote:
>> The IOMMU interrupt bits in the IOMMU status registers are
>> cleared when writing 1.  Therefore, the existing logic which reads
>> the register, set the bit, and then writing back the values
>> could accidentally clear certain bits if it has been set.
>>
>> The correct logic would just be writing only the value which only
>> set the interrupt bits, and leave the rest to zeros.
> So looking through the status register accesses, I would assume
> that guest_iommu_mmio_write64() is broken too? In that it
> clearly doesn''t implement the RW1C behavior for the interrupt
> and overflow bits?
Actually, I''m not quite sure why hvm guest would be writing to IOMMU 
mmio.  I don''t actually see AMD IOMMU is exposed to the hvm guest. 
However, isn''t this function is supposed to be passing on the value 
intended to be written from guest?  In the sense, if the guest is trying 
to set the bit to 1, the function will just do what it''s been told to 
write to the register?
>
> Further (as indicated above), the overflow bits being RW1C too,
> doesn''t iommu_reset_log() need a similar fix
> (iommu_set_bit(&entry, of_bit) instead of
> iommu_clear_bit(&entry, of_bit))?
>
> And, perhaps even more importantly, iommu_interrupt_handler()
> also ought to use iommu_set_bit()? Or wait - isn''t this touching
> the wrong bits altogether? Logically I would expect interrupt
> enable bits to be cleared here, i.e. IOMMU_CONTROL_* to be
> used throughout this function instead of IOMMU_STATUS_*.
>
> Jan
>
Thanks for catching all these several places where RW1C is not properly 
implemented.  I''ll submit a new patch.

Suravee

>>> On 23.04.13 at 02:53, Suravee Suthikulanit
<suravee.suthikulpanit@amd.com> wrote:
> On 4/19/2013 3:18 AM, Jan Beulich wrote:
>>>>> On 18.04.13 at 20:41, <suravee.suthikulpanit@amd.com>
wrote:
>>> The IOMMU interrupt bits in the IOMMU status registers are
>>> cleared when writing 1.  Therefore, the existing logic which reads
>>> the register, set the bit, and then writing back the values
>>> could accidentally clear certain bits if it has been set.
>>>
>>> The correct logic would just be writing only the value which only
>>> set the interrupt bits, and leave the rest to zeros.
>> So looking through the status register accesses, I would assume
>> that guest_iommu_mmio_write64() is broken too? In that it
>> clearly doesn''t implement the RW1C behavior for the interrupt
>> and overflow bits?
> Actually, I''m not quite sure why hvm guest would be writing to
IOMMU
> mmio.  I don''t actually see AMD IOMMU is exposed to the hvm guest.
> However, isn''t this function is supposed to be passing on the
value
> intended to be written from guest?  In the sense, if the guest is trying 
> to set the bit to 1, the function will just do what it''s been told
to
> write to the register?
This is a virtual IOMMU after all. And hence the emulation code
ought to do what real hardware would in the same situation.

Jan