[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.