Re: [Xen-devel] [Patch RFC 02/13] vt-d: Register MSI for async invalidation completion interrupt.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.09.15 at 15:23, <quan.xu@xxxxxxxxx> wrote:
> +/* IOMMU Queued Invalidation(QI). */
> +static void _qi_msi_unmask(struct iommu *iommu)
> +{
> +    u32 sts;
> +    unsigned long flags;
> +
> +    /* Clear IM bit of DMAR_IECTL_REG. */
> +    spin_lock_irqsave(&iommu->register_lock, flags);
> +    sts = dmar_readl(iommu->reg, DMAR_IECTL_REG);
> +    sts &= ~DMA_IECTL_IM;
> +    dmar_writel(iommu->reg, DMAR_IECTL_REG, sts);
> +    spin_unlock_irqrestore(&iommu->register_lock, flags);
> +}

I think rather than duplicating all this code from the fault interrupt
you should instead re-factor to make the original usable for both
purposes. Afaics the differences really are just the register and
bit locations.

> +static void _do_iommu_qi(struct iommu *iommu)
> +{
> +}

???

> +static void qi_msi_unmask(struct irq_desc *desc)
> +{
> +    _qi_msi_unmask(desc->action->dev_id);
> +}
> +
> +static void qi_msi_mask(struct irq_desc *desc)
> +{
> +    _qi_msi_mask(desc->action->dev_id);
> +}

These wrappers look pretty pointless.

> +static void qi_msi_end(struct irq_desc *desc, u8 vector)
> +{
> +    ack_APIC_irq();
> +}

Why is there, other than for its fault counterpart, no unmask
operation here?

> @@ -1123,6 +1243,7 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>          return -ENOMEM;
>  
>      iommu->msi.irq = -1; /* No irq assigned yet. */
> +    iommu->qi_msi.irq = -1; /* No irq assigned yet. */

Which suggests that (perhaps in patch 1) the existing field should be
renamed to e.g. fe_msi.

> @@ -1985,6 +2109,9 @@ static void adjust_irq_affinity(struct acpi_drhd_unit 
> *drhd)
>           cpumask_intersects(&node_to_cpumask(node), cpumask) )
>          cpumask = &node_to_cpumask(node);
>      dma_msi_set_affinity(irq_to_desc(drhd->iommu->msi.irq), cpumask);
> +
> +    if ( ats_enabled )
> +        qi_msi_set_affinity(irq_to_desc(drhd->iommu->qi_msi.irq), cpumask);
>  }
>  
>  int adjust_vtd_irq_affinities(void)
> @@ -2183,6 +2310,11 @@ int __init intel_vtd_setup(void)
>  
>          ret = iommu_set_interrupt(drhd, &dma_msi_type, "dmar", 
> &drhd->iommu->msi,
>                                    iommu_page_fault);
> +        if ( ats_enabled )
> +            ret = iommu_set_interrupt(drhd, &qi_msi_type, "qi",
> +                                      &drhd->iommu->qi_msi,
> +                                      iommu_qi_completion);
> +

Would there be any harm from leaving out most/all of these
ats_enabled conditionals, despite right now that code only intended
to be used for ATS invalidations? I.e. wouldn't it just be that the
interrupt never triggers?

> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -47,6 +47,11 @@
>  #define    DMAR_IQH_REG    0x80    /* invalidation queue head */
>  #define    DMAR_IQT_REG    0x88    /* invalidation queue tail */
>  #define    DMAR_IQA_REG    0x90    /* invalidation queue addr */
> +#define    DMAR_IECTL_REG  0xA0    /* invalidation event contrl register */
> +#define    DMAR_IEDATA_REG 0xA4    /* invalidation event data register */
> +#define    DMAR_IEADDR_REG 0xA8    /* invalidation event address register */
> +#define    DMAR_IEUADDR_REG 0xAC   /* invalidation event upper address 
> register */
> +#define    DMAR_ICS_REG    0x9C    /* invalidation completion status 
> register */

Numerically ordered please.

Jan


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