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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

>> >>> On 29.09.2015 at 16:57 <JBeulich@xxxxxxxx> wrote:
> >>> 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.
> 

hw_irq_controller is a common data structure for arm/amd/x86.
For reusing these function, I should redefine the function pointers 
.set_affinity / . startup .etc.
It takes much effort to modify the other arm/amd/x86 code.


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

Now it still has no knowledge about ATS.


> > +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.
> 

I will modify it in next version.


> > +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?
> 


 In my design: I try to optimize it in interrupt handler(or associated tasklet):
Check the IP field at the end of interrupt handler, if it is Set, try to scan 
the domain list again, instead of clear IM to cause another Interrupt.
For the logic of IWC/IP/IM as below:

                       Interrupt condition (An invalidation wait descriptor 
with Interrupt Flag(IF) field Set completed.)
                            ||
                             v
           ----------------------(IWC) ----------------------
           (IWC is Set)                     (IWC is not Set)
               ||                              ||
               V                              ||
(Not treated as an new interrupt condition)         ||
                                               ||
                                               V
                                             (Set IWC / IP)
                                               ||
                                               V
                         ------------------------------(IM)---------------------
                       (IM is Set)                               (IM not Set)
                        ||                                        ||
                        ||                                        V
                        ||                    (cause Interrupt message / then 
hardware clear IP)
                        ||
                        V
   (interrupt is held pending, clearing IM to cause interrupt message)
  

And, If you clear IWC, the IP is cleared.


> > @@ -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.
> 

I'd better rename the existing in next patch set. otherwise it is confusing.



> > @@ -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?
> 

No harm.
It is no use if the ATS is not enabled now.
It is only be triggered when an invalidation wait descriptor with Interrupt 
Flag(IF) field Set completed


> > --- 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.
> 

Got it.


Quan






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