Xen project Mailing List

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

From: "Jan Beulich" <JBeulich@xxxxxxxx>

Date: Mon, 10 Jun 2013 13:41:12 +0100

Cc: Tim Deegan <tim@xxxxxxx>, suravee.suthikulpanit@xxxxxxx

Delivery-date: Mon, 10 Jun 2013 12:41:36 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

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. Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx> With iommu_interrupt_handler() properly having got switched its readl() from status to control register, the subsequent writel() needed to be switched too (and the RW1C comment there was bogus). Some of the cleanup went too far - undone. Further, with iommu_interrupt_handler() now actually disabling the interrupt sources, they also need to get re-enabled by the tasklet once it finished processing the respective log. Finally, guest write emulation to the status register needs to be done with the RW1C (and RO for all other bits) semantics in mind too. Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> --- v4: Clear _only_ the respective overflow bit in iommu_reset_log(). Fix logic error in adjustment to guest_iommu_mmio_write64(). (Both pointed out by Tim - Thanks!) --- 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 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(IOMMU_STATUS_COMP_WAIT_INT_MASK, + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); /* send an empty COMPLETION_WAIT command to flush command buffer */ cmd[3] = cmd[2] = 0; @@ -103,9 +101,9 @@ static void flush_command_buffer(struct 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(IOMMU_STATUS_COMP_WAIT_INT_MASK, + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); return; } AMD_IOMMU_DEBUG("Warning: ComWaitInt bit did not assert!\n"); --- a/xen/drivers/passthrough/amd/iommu_guest.c +++ b/xen/drivers/passthrough/amd/iommu_guest.c @@ -754,7 +754,14 @@ static void guest_iommu_mmio_write64(str u64_to_reg(&iommu->ppr_log.reg_tail, val); break; case IOMMU_STATUS_MMIO_OFFSET: - u64_to_reg(&iommu->reg_status, val); + val &= IOMMU_STATUS_EVENT_OVERFLOW_MASK | + IOMMU_STATUS_EVENT_LOG_INT_MASK | + IOMMU_STATUS_COMP_WAIT_INT_MASK | + IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK | + IOMMU_STATUS_PPR_LOG_INT_MASK | + IOMMU_STATUS_GAPIC_LOG_OVERFLOW_MASK | + IOMMU_STATUS_GAPIC_LOG_INT_MASK; + u64_to_reg(&iommu->reg_status, reg_to_u64(iommu->reg_status) & ~val); break; default: --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -344,13 +344,13 @@ static void set_iommu_ppr_log_control(st writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET); iommu_set_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT); - iommu_set_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT); + iommu_set_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT); iommu_set_bit(&entry, IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT); } else { iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT); - iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT); + iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT); iommu_clear_bit(&entry, IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT); } @@ -410,7 +410,7 @@ static void iommu_reset_log(struct amd_i void (*ctrl_func)(struct amd_iommu *iommu, int)) { u32 entry; - int log_run, run_bit, of_bit; + int log_run, run_bit; int loop_count = 1000; BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log))); @@ -419,10 +419,6 @@ static void iommu_reset_log(struct amd_i IOMMU_STATUS_EVENT_LOG_RUN_SHIFT : IOMMU_STATUS_PPR_LOG_RUN_SHIFT; - of_bit = ( log == &iommu->event_log ) ? - IOMMU_STATUS_EVENT_OVERFLOW_SHIFT : - IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT; - /* wait until EventLogRun bit = 0 */ do { entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); @@ -439,9 +435,10 @@ static void iommu_reset_log(struct amd_i ctrl_func(iommu, IOMMU_CONTROL_DISABLED); - /*clear overflow bit */ - iommu_clear_bit(&entry, of_bit); - writel(entry, iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); + /* RW1C overflow bit */ + writel(log == &iommu->event_log ? IOMMU_STATUS_EVENT_OVERFLOW_MASK + : IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK, + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); /*reset event log base address */ log->head = 0; @@ -619,14 +616,18 @@ static void iommu_check_event_log(struct /*check event overflow */ entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); + /* RW1C interrupt status bit */ + writel(IOMMU_STATUS_EVENT_LOG_INT_MASK, + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); + 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); + else + { + entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET); + iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT); + writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET); + } spin_unlock_irqrestore(&iommu->lock, flags); } @@ -689,14 +690,18 @@ static void iommu_check_ppr_log(struct a /*check event overflow */ entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); + /* RW1C interrupt status bit */ + writel(IOMMU_STATUS_PPR_LOG_INT_MASK, + iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET); + 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); + else + { + entry = readl(iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET); + iommu_set_bit(&entry, IOMMU_CONTROL_PPR_LOG_INT_SHIFT); + writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET); + } spin_unlock_irqrestore(&iommu->lock, flags); } @@ -733,11 +738,14 @@ static void iommu_interrupt_handler(int 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); - writel(entry, iommu->mmio_base+IOMMU_STATUS_MMIO_OFFSET); + /* + * 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); + writel(entry, iommu->mmio_base + IOMMU_CONTROL_MMIO_OFFSET); spin_unlock_irqrestore(&iommu->lock, flags); --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -336,14 +336,13 @@ #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_PPR_LOG_ENABLE_SHIFT 13 +#define IOMMU_CONTROL_PPR_LOG_INT_SHIFT 14 +#define IOMMU_CONTROL_PPR_ENABLE_SHIFT 15 +#define IOMMU_CONTROL_GT_ENABLE_SHIFT 16 #define IOMMU_CONTROL_RESTART_MASK 0x80000000 #define IOMMU_CONTROL_RESTART_SHIFT 31 -#define IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT 13 -#define IOMMU_CONTROL_PPR_INT_SHIFT 14 -#define IOMMU_CONTROL_PPR_ENABLE_SHIFT 15 -#define IOMMU_CONTROL_GT_ENABLE_SHIFT 16 - /* Exclusion Register */ #define IOMMU_EXCLUSION_BASE_LOW_OFFSET 0x20 #define IOMMU_EXCLUSION_BASE_HIGH_OFFSET 0x24 @@ -395,9 +394,18 @@ #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_PPR_LOG_OVERFLOW_MASK 0x00000020 #define IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT 5 +#define IOMMU_STATUS_PPR_LOG_INT_MASK 0x00000040 #define IOMMU_STATUS_PPR_LOG_INT_SHIFT 6 +#define IOMMU_STATUS_PPR_LOG_RUN_MASK 0x00000080 #define IOMMU_STATUS_PPR_LOG_RUN_SHIFT 7 +#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW_MASK 0x00000100 +#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW_SHIFT 8 +#define IOMMU_STATUS_GAPIC_LOG_INT_MASK 0x00000200 +#define IOMMU_STATUS_GAPIC_LOG_INT_SHIFT 9 +#define IOMMU_STATUS_GAPIC_LOG_RUN_MASK 0x00000400 +#define IOMMU_STATUS_GAPIC_LOG_RUN_SHIFT 10 /* I/O Page Table */ #define IOMMU_PAGE_TABLE_ENTRY_SIZE 8

_______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.