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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.15 at 10:22, <quan.xu@xxxxxxxxx> wrote:
>> > >>> 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.

Why?

> It takes much effort to modify the other arm/amd/x86 code.

Sure, and this certainly should be avoided.

>> > +static void _do_iommu_qi(struct iommu *iommu) { }
>> 
>> ???
>> 
> 
> Now it still has no knowledge about ATS.

But what's the function good for then? Can't it be added when it
needs to have a body?

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

I don't follow - in analogy to handling of the Fault Event Control
Register, unmasking would mean to clear IM, not IWC. The
question really is more about symmetry, i.e. if the answer is
"we don't need this because IM ought to be clear anyway", then
you'd also need to answer why the fault event case needs it
(which I think is pretty clear - it needs to undo what the
corresponding .ack method did).

Jan


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