Re: [Xen-devel] [Patch RFC 05/13] vt-d: Clear the IWC field of Invalidation Event Control Register in

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.09.15 at 15:23, <quan.xu@xxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1070,6 +1070,27 @@ static hw_irq_controller dma_msi_type = {
>  };
>  
>  /* IOMMU Queued Invalidation(QI). */
> +static void qi_clear_iwc(struct iommu *iommu)
> +{
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&iommu->register_lock, flags);
> +    dmar_writel(iommu->reg, DMAR_ICS_REG, RW1CS);

RW1CS is definitely not a suitable name for this (or any other) bit.
The manual and the title call the bit IWC, i.e. maybe QI_ICS_IWC?
Also I don't think you can validly assume you can blindly write
zeroes to the other bits, but considering the RW1CS nature of this
bit it's also not clear whether blindly writing back what you read
would be a good idea.

> +static int _qi_msi_ip(struct iommu *iommu)
> +{
> +    u32 sts;
> +    unsigned long flags;
> +
> +    /* Get IP bit of DMAR_IECTL_REG. */
> +    spin_lock_irqsave(&iommu->register_lock, flags);
> +    sts = dmar_readl(iommu->reg, DMAR_IECTL_REG);
> +    spin_unlock_irqrestore(&iommu->register_lock, flags);
> +    return (sts & DMA_IECTL_IP);
> +}

The function appears to be meant to return a boolean, i.e. its
return type should be bool_t and the return statement should
use !!.

> @@ -1101,6 +1122,14 @@ static void _do_iommu_qi(struct iommu *iommu)
>      unsigned long nr_dom, i;
>      struct domain *d = NULL;
>  
> +scan_again:

Labels indented by at least on space please. Even better would be if
this was written without goto.

> @@ -1120,6 +1149,28 @@ static void _do_iommu_qi(struct iommu *iommu)
>          }
>          i = find_next_bit(iommu->domid_bitmap, nr_dom, i+1);
>      }
> +
> +    /*
> +     * IP is interrupt pending and the 30 bit of Invalidation Event Control
> +     * Register. The IP field is kept Set by hardware while the interrupt
> +     * message is held pending. The IP field is cleared by hardware as soon
> +     * as the interrupt message pending condition  is serviced. IP could be
> +     * cleard due to either:
> +     *
> +     * - Clear IM field in the Invalidation Event Control Register. A QI
> +     *   interrupt is generated along with clearing the IP field.
> +     * - Clear IWC field in the Invalidateion Coompletion Status register.
> +     *
> +     * If the Ip is Set, scan agian, instead of generating another interrupt.
> +     */
> +    if ( _qi_msi_ip(iommu) )
> +        goto scan_again;
> +
> +    /*
> +     * No masking of QI interrupt. when a QI interrupt event condition is
> +     * detected, hardware issues an interrupt message.
> +     */
> +    _qi_msi_unmask(iommu);

Isn't this what actually belongs in the flow end handler?

> @@ -1154,6 +1205,14 @@ static void qi_msi_mask(struct irq_desc *desc)
>  
>  static unsigned int qi_msi_startup(struct irq_desc *desc)
>  {
> +    struct iommu *iommu = desc->action->dev_id;
> +
> +    /*
> +     * If the IWC field in the Invalidation Completion Status register was 
> already
> +     * Set at the time of setting this field, it is not treated as a new 
> interrupt
> +     * condition.
> +     */
> +    qi_clear_iwc(iommu);

Things not related directly to interrupt control don't belong in flow
handlers.

Jan


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