Re: [Qemu-devel] [PATCH v2 12/13] intel_iommu: ioapic: IR support for emulated IOAPIC

[Peter Xu]

On Mon, Apr 11, 2016 at 10:22:18PM -0700, Jan Kiszka wrote:
> On 2016-04-11 02:19, Peter Xu wrote:
> > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> > index 378e663..d963d45 100644
> > --- a/hw/intc/ioapic.c
> > +++ b/hw/intc/ioapic.c
> > @@ -57,6 +57,8 @@ static void ioapic_service(IOAPICCommonState *s)
> >      uint64_t entry;
> >      uint8_t dest;
> >      uint8_t dest_mode;
> > +    IntelIOMMUState *iommu = s->iommu;
> > +    VTDIrq irq = {0};
> >  
> >      for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> >          mask = 1 << i;
> > @@ -65,11 +67,33 @@ static void ioapic_service(IOAPICCommonState *s)
> >  
> >              entry = s->ioredtbl[i];
> >              if (!(entry & IOAPIC_LVT_MASKED)) {
> > -                trig_mode = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
> > -                dest = entry >> IOAPIC_LVT_DEST_SHIFT;
> > -                dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
> > -                delivery_mode =
> > -                    (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & 
> > IOAPIC_DM_MASK;
> > +
> > +                if (iommu && iommu->intr_enabled) {
> > +                    /*
> > +                    * Interrupt remapping is enabled in owner IOMMU,
> > +                    * we need to fetch the real IRQ information via
> > +                    * IRTE of the root mapping table
> > +                    */
> > +                    if (vtd_interrupt_remap_ioapic(iommu, &entry, &irq)) {
> 
> VT-d is only one of the possible IOMMUs on x86. Please introduce a
> generic interface.
> 
> Look at Rita's and my patches: they translate the IOAPIC (and HPET...)
> interrupts into MSI messages that are then - in a generic way -
> intercepted by the respective IOMMU or directly dispatched to the APICs.
> We may no longer need new memory regions for this, thanks to the region
> attributes, but we also need no hard-coded hooks here.

Yes, I should consider other x86 platforms like AMD. Thanks to point
out. It seems that there are many places in the patchset that lacks
thorough consideration about this. Will try to fix them in next
version.

Regarding to the above MSI solution: I'd say it is a good way to
hide everything else behind.  However, since we introduced one extra
layer (MSI) which actually does not exist, not sure there would be
problem too.  Also, I feel it a little bit hacky if we "create" one
MSI out of the air...  For example, if someone tries to capture MSIs
from QEMU inside in the APIC memory writes, he will see something he
cannot explain if he never knows this hack's there.  Considering the
above, I would prefer hooks, or better to provide a callback (a
function pointer that others like AMD can override) to do the
translation.  How do you think?

Thanks.

-- peterx