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

[Jan Kiszka]

On 2016-04-12 02:02, Peter Xu wrote:
> 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?

The HPET does send MSIs, and I'm not sure how much different the
IOAPIC's message actually is. In any case, modelling it as MSI is
neither adding incorrectness nor making the code more complex (in fact,
the contrary is true!). Last but not least, it would be trivial to
filter out non-PCI MSI sources if we wanted to trace only PCI - because
we need to identify the origin anyway for remapping purposes. So,
explicit hooking looks like the wrong way to me.

Jan