|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2 V2] iommu/amd: Fix logic for clearing the IOMMU interrupt bits
>>> On 10.06.13 at 07:05, <suravee.suthikulpanit@xxxxxxx> wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>
> The IOMMU interrupt bits in the IOMMU status registers are
> "read-only, and write-1-to-clear (RW1C). 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.
>
> This patch also, clean up #define masks as Jan has suggested.
This went to far, and imo in the wrong direction - the mask values
are what ultimately should stay, since the shift values can be
computed from them. And this cleanup should really be done only
when [gs]et_field_in_reg_u32() get the redundant shift parameter
eliminated (i.e. in a separate, post-4.3 patch).
But as said already in the response to Tim's reply on patch 2 - this
patch also had a few other issues, so I'm going to reply with a v3
once I'm done with the fixing/cleanup.
Jan
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
> V2 changes:
> - Additional fixes as pointed out by Jan.
> - Clean up unnecessary #define mask as suggested by Jan.
>
> xen/drivers/passthrough/amd/iommu_cmd.c | 18 +++--
> xen/drivers/passthrough/amd/iommu_init.c | 31 ++++-----
> xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 95
> +++++++++++---------------
> 3 files changed, 63 insertions(+), 81 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c
> b/xen/drivers/passthrough/amd/iommu_cmd.c
> index 4c60149..f0283d4 100644
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -75,11 +75,9 @@ static void flush_command_buffer(struct amd_iommu *iommu)
> u32 cmd[4], status;
> int loop_count, comp_wait;
>
> - /* clear 'ComWaitInt' in status register (WIC) */
> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, 0,
> - IOMMU_STATUS_COMP_WAIT_INT_MASK,
> - IOMMU_STATUS_COMP_WAIT_INT_SHIFT, &status);
> - writel(status, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> + /* RW1C 'ComWaitInt' in status register */
> + writel((1 << IOMMU_STATUS_COMP_WAIT_INT_SHIFT),
> + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>
> /* send an empty COMPLETION_WAIT command to flush command buffer */
> cmd[3] = cmd[2] = 0;
> @@ -96,16 +94,16 @@ static void flush_command_buffer(struct amd_iommu *iommu)
> 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);
> + (1 << IOMMU_STATUS_COMP_WAIT_INT_SHIFT),
> + IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
> --loop_count;
> } while ( !comp_wait && loop_count );
>
> if ( comp_wait )
> {
> - /* clear 'ComWaitInt' in status register (WIC) */
> - status &= IOMMU_STATUS_COMP_WAIT_INT_MASK;
> - writel(status, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> + /* RW1C 'ComWaitInt' in status register */
> + writel((1 << IOMMU_STATUS_COMP_WAIT_INT_SHIFT),
> + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> return;
> }
> AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n");
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c
> b/xen/drivers/passthrough/amd/iommu_init.c
> index a939c73..a85c63f 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -439,8 +439,8 @@ static void iommu_reset_log(struct amd_iommu *iommu,
>
> ctrl_func(iommu, IOMMU_CONTROL_DISABLED);
>
> - /*clear overflow bit */
> - iommu_clear_bit(&entry, of_bit);
> + /* RW1C overflow bit */
> + iommu_set_bit(&entry, of_bit);
> writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>
> /*reset event log base address */
> @@ -622,11 +622,9 @@ static void iommu_check_event_log(struct amd_iommu
> *iommu)
> if ( iommu_get_bit(entry, IOMMU_STATUS_EVENT_OVERFLOW_SHIFT) )
> 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);
> + /* RW1C interrupt status bit */
> + writel((1 << IOMMU_STATUS_EVENT_LOG_INT_SHIFT),
> + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>
> spin_unlock_irqrestore(&iommu->lock, flags);
> }
> @@ -692,11 +690,9 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
> if ( iommu_get_bit(entry, IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT) )
> 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);
> + /* RW1C interrupt status bit */
> + writel((1 << IOMMU_STATUS_PPR_LOG_INT_SHIFT),
> + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>
> spin_unlock_irqrestore(&iommu->lock, flags);
> }
> @@ -733,10 +729,13 @@ static void iommu_interrupt_handler(int irq, void
> *dev_id,
>
> spin_lock_irqsave(&iommu->lock, flags);
>
> - /* Silence interrupts from both event and PPR logging */
> - entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> - iommu_clear_bit(&entry, IOMMU_STATUS_EVENT_LOG_INT_SHIFT);
> - iommu_clear_bit(&entry, IOMMU_STATUS_PPR_LOG_INT_SHIFT);
> + /* Silence interrupts from both event and PPR
> + * by clearing the enable logging bits in the
> + * control register */
> + entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET);
> + iommu_clear_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
> + iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT);
> + /* RW1C status bit */
> writel(entry, 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..2f2d740 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -313,31 +313,21 @@
> #define IOMMU_LOG_ENTRY_TIMEOUT 1000
>
> /* Control Register */
> -#define IOMMU_CONTROL_MMIO_OFFSET 0x18
> -#define IOMMU_CONTROL_TRANSLATION_ENABLE_MASK 0x00000001
> -#define IOMMU_CONTROL_TRANSLATION_ENABLE_SHIFT 0
> -#define IOMMU_CONTROL_HT_TUNNEL_TRANSLATION_MASK 0x00000002
> -#define IOMMU_CONTROL_HT_TUNNEL_TRANSLATION_SHIFT 1
> -#define IOMMU_CONTROL_EVENT_LOG_ENABLE_MASK 0x00000004
> -#define IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT 2
> -#define IOMMU_CONTROL_EVENT_LOG_INT_MASK 0x00000008
> -#define IOMMU_CONTROL_EVENT_LOG_INT_SHIFT 3
> -#define IOMMU_CONTROL_COMP_WAIT_INT_MASK 0x00000010
> -#define IOMMU_CONTROL_COMP_WAIT_INT_SHIFT 4
> -#define IOMMU_CONTROL_INVALIDATION_TIMEOUT_MASK 0x000000E0
> -#define IOMMU_CONTROL_INVALIDATION_TIMEOUT_SHIFT 5
> -#define IOMMU_CONTROL_PASS_POSTED_WRITE_MASK 0x00000100
> -#define IOMMU_CONTROL_PASS_POSTED_WRITE_SHIFT 8
> -#define IOMMU_CONTROL_RESP_PASS_POSTED_WRITE_MASK 0x00000200
> -#define IOMMU_CONTROL_RESP_PASS_POSTED_WRITE_SHIFT 9
> -#define IOMMU_CONTROL_COHERENT_MASK 0x00000400
> -#define IOMMU_CONTROL_COHERENT_SHIFT 10
> -#define IOMMU_CONTROL_ISOCHRONOUS_MASK 0x00000800
> -#define IOMMU_CONTROL_ISOCHRONOUS_SHIFT 11
> -#define IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_MASK 0x00001000
> -#define IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT 12
> -#define IOMMU_CONTROL_RESTART_MASK 0x80000000
> -#define IOMMU_CONTROL_RESTART_SHIFT 31
> +#define IOMMU_CONTROL_MMIO_OFFSET 0x18
> +#define IOMMU_CONTROL_TRANSLATION_ENABLE_SHIFT 0
> +#define IOMMU_CONTROL_HT_TUNNEL_TRANSLATION_SHIFT 1
> +#define IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT 2
> +#define IOMMU_CONTROL_EVENT_LOG_INT_SHIFT 3
> +#define IOMMU_CONTROL_COMP_WAIT_INT_SHIFT 4
> +#define IOMMU_CONTROL_INVALIDATION_TIMEOUT_SHIFT 5
> +#define IOMMU_CONTROL_PASS_POSTED_WRITE_SHIFT 8
> +#define IOMMU_CONTROL_RESP_PASS_POSTED_WRITE_SHIFT 9
> +#define IOMMU_CONTROL_COHERENT_SHIFT 10
> +#define IOMMU_CONTROL_ISOCHRONOUS_SHIFT 11
> +#define IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT 12
> +#define IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT 13
> +#define IOMMU_CONTROL_PPR_LOG_INT_SHIFT 14
> +#define IOMMU_CONTROL_RESTART_SHIFT 31
>
> #define IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT 13
> #define IOMMU_CONTROL_PPR_INT_SHIFT 14
> @@ -363,38 +353,33 @@
> #define IOMMU_EXCLUSION_LIMIT_HIGH_SHIFT 0
>
> /* Extended Feature Register*/
> -#define IOMMU_EXT_FEATURE_MMIO_OFFSET 0x30
> -#define IOMMU_EXT_FEATURE_PREFSUP_SHIFT 0x0
> -#define IOMMU_EXT_FEATURE_PPRSUP_SHIFT 0x1
> -#define IOMMU_EXT_FEATURE_XTSUP_SHIFT 0x2
> -#define IOMMU_EXT_FEATURE_NXSUP_SHIFT 0x3
> -#define IOMMU_EXT_FEATURE_GTSUP_SHIFT 0x4
> -#define IOMMU_EXT_FEATURE_IASUP_SHIFT 0x6
> -#define IOMMU_EXT_FEATURE_GASUP_SHIFT 0x7
> -#define IOMMU_EXT_FEATURE_HESUP_SHIFT 0x8
> -#define IOMMU_EXT_FEATURE_PCSUP_SHIFT 0x9
> -#define IOMMU_EXT_FEATURE_HATS_SHIFT 0x10
> -#define IOMMU_EXT_FEATURE_HATS_MASK 0x00000C00
> -#define IOMMU_EXT_FEATURE_GATS_SHIFT 0x12
> -#define IOMMU_EXT_FEATURE_GATS_MASK 0x00003000
> -#define IOMMU_EXT_FEATURE_GLXSUP_SHIFT 0x14
> -#define IOMMU_EXT_FEATURE_GLXSUP_MASK 0x0000C000
> -
> -#define IOMMU_EXT_FEATURE_PASMAX_SHIFT 0x0
> -#define IOMMU_EXT_FEATURE_PASMAX_MASK 0x0000001F
> +#define IOMMU_EXT_FEATURE_MMIO_OFFSET 0x30
> +#define IOMMU_EXT_FEATURE_PREFSUP_SHIFT 0x0
> +#define IOMMU_EXT_FEATURE_PPRSUP_SHIFT 0x1
> +#define IOMMU_EXT_FEATURE_XTSUP_SHIFT 0x2
> +#define IOMMU_EXT_FEATURE_NXSUP_SHIFT 0x3
> +#define IOMMU_EXT_FEATURE_GTSUP_SHIFT 0x4
> +#define IOMMU_EXT_FEATURE_IASUP_SHIFT 0x6
> +#define IOMMU_EXT_FEATURE_GASUP_SHIFT 0x7
> +#define IOMMU_EXT_FEATURE_HESUP_SHIFT 0x8
> +#define IOMMU_EXT_FEATURE_PCSUP_SHIFT 0x9
> +#define IOMMU_EXT_FEATURE_HATS_SHIFT 0x10
> +#define IOMMU_EXT_FEATURE_HATS_MASK 0x00000C00
> +#define IOMMU_EXT_FEATURE_GATS_SHIFT 0x12
> +#define IOMMU_EXT_FEATURE_GATS_MASK 0x00003000
> +#define IOMMU_EXT_FEATURE_GLXSUP_SHIFT 0x14
> +#define IOMMU_EXT_FEATURE_GLXSUP_MASK 0x0000C000
> +
> +#define IOMMU_EXT_FEATURE_PASMAX_SHIFT 0x0
> +#define IOMMU_EXT_FEATURE_PASMAX_MASK 0x0000001F
>
> /* 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_LOG_INT_SHIFT 1
> -#define IOMMU_STATUS_COMP_WAIT_INT_MASK 0x00000004
> -#define IOMMU_STATUS_COMP_WAIT_INT_SHIFT 2
> -#define IOMMU_STATUS_EVENT_LOG_RUN_MASK 0x00000008
> -#define IOMMU_STATUS_EVENT_LOG_RUN_SHIFT 3
> -#define IOMMU_STATUS_CMD_BUFFER_RUN_MASK 0x00000010
> -#define IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT 4
> +#define IOMMU_STATUS_MMIO_OFFSET 0x2020
> +#define IOMMU_STATUS_EVENT_OVERFLOW_SHIFT 0
> +#define IOMMU_STATUS_EVENT_LOG_INT_SHIFT 1
> +#define IOMMU_STATUS_COMP_WAIT_INT_SHIFT 2
> +#define IOMMU_STATUS_EVENT_LOG_RUN_SHIFT 3
> +#define IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT 4
> #define IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT 5
> #define IOMMU_STATUS_PPR_LOG_INT_SHIFT 6
> #define IOMMU_STATUS_PPR_LOG_RUN_SHIFT 7
> --
> 1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |