[v4,00/16] IOMMU: Enable interrupt remapping for Intel IOMMU

Commit Message

Paolo Bonzini May 9, 2016, 11:58 a.m. UTC

On 26/04/2016 09:34, Peter Xu wrote:
> +/*
> + * This is to satisfy the hack in Linux kernel. One hack of it is to
> + * simulate clearing the Remote IRR bit of IOAPIC entry using the
> + * following:
> + *
> + * "For IO-APIC's with EOI register, we use that to do an explicit EOI.
> + * Otherwise, we simulate the EOI message manually by changing the trigger
> + * mode to edge and then back to level, with RTE being masked during
> + * this."
> + *
> + * (See linux kernel __eoi_ioapic_pin() comment in commit c0205701)
> + *
> + * This is based on the assumption that, Remote IRR bit will be
> + * cleared by IOAPIC hardware for edge-triggered interrupts (I
> + * believe that's what the IOAPIC version 0x1X hardware does). So
> + * if we are emulating it, we'd better do it the same here, so that
> + * the guest kernel hack will work as well on QEMU.
> + *
> + * Without this, level-triggered interrupts in IR mode might fail to
> + * work correctly.
> + */
> +static inline void
> +ioapic_fix_edge_remote_irr(uint64_t *entry)
> +{
> +    if (*entry & IOAPIC_LVT_TRIGGER_MODE) {
> +        /* Level triggered interrupts, make sure remote IRR is zero */
> +        *entry &= ~((uint64_t)IOAPIC_LVT_REMOTE_IRR);
> +    }
> +}
> +
>  static void
>  ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
>                   unsigned int size)
> @@ -314,6 +344,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
>                      s->ioredtbl[index] &= ~0xffffffffULL;
>                      s->ioredtbl[index] |= val;
>                  }
> +                ioapic_fix_edge_remote_irr(&s->ioredtbl[index]);
>                  ioapic_service(s);
>              }
>          }

Is this enough too?


Thanks,

Paolo

Comments

Peter Xu May 10, 2016, 6:09 a.m. UTC | #1

On Mon, May 09, 2016 at 01:58:48PM +0200, Paolo Bonzini wrote:
> Is this enough too?
> 
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 378e663..2443a35 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -72,6 +72,7 @@ static void ioapic_service(IOAPICCommonState *s)
>                      (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
>                  if (trig_mode == IOAPIC_TRIGGER_EDGE) {
>                      s->irr &= ~mask;
> +                    s->ioredtbl[i] &= ~IOAPIC_LVT_REMOTE_IRR;
>                  } else {
>                      coalesce = s->ioredtbl[i] & IOAPIC_LVT_REMOTE_IRR;
>                      s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;

I gave it a quick shot on this but still got the warning. :(

I _guess_ the problem is: the above change is in the "if" block of
(s->irr & mask), when the kernel plays the trick of EOI, the irq
should be pulled down already by the device (or say, irr bit is
cleared). So it does not go into this "if" block.

-- peterx

Paolo Bonzini May 10, 2016, 8:58 a.m. UTC | #2

On 10/05/2016 08:09, Peter Xu wrote:
> On Mon, May 09, 2016 at 01:58:48PM +0200, Paolo Bonzini wrote:
>> Is this enough too?
>>
>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
>> index 378e663..2443a35 100644
>> --- a/hw/intc/ioapic.c
>> +++ b/hw/intc/ioapic.c
>> @@ -72,6 +72,7 @@ static void ioapic_service(IOAPICCommonState *s)
>>                      (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
>>                  if (trig_mode == IOAPIC_TRIGGER_EDGE) {
>>                      s->irr &= ~mask;
>> +                    s->ioredtbl[i] &= ~IOAPIC_LVT_REMOTE_IRR;
>>                  } else {
>>                      coalesce = s->ioredtbl[i] & IOAPIC_LVT_REMOTE_IRR;
>>                      s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;
> 
> I gave it a quick shot on this but still got the warning. :(
> 
> I _guess_ the problem is: the above change is in the "if" block of
> (s->irr & mask), when the kernel plays the trick of EOI, the irq
> should be pulled down already by the device (or say, irr bit is
> cleared). So it does not go into this "if" block.

No problem; feel free to send the other patch separately and I'll take
care of merging it.  Otherwise mst can merge it too.

Paolo

Peter Xu May 10, 2016, 10:10 a.m. UTC | #3

On Tue, May 10, 2016 at 10:58:02AM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/05/2016 08:09, Peter Xu wrote:
> > On Mon, May 09, 2016 at 01:58:48PM +0200, Paolo Bonzini wrote:
> >> Is this enough too?
> >>
> >> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> >> index 378e663..2443a35 100644
> >> --- a/hw/intc/ioapic.c
> >> +++ b/hw/intc/ioapic.c
> >> @@ -72,6 +72,7 @@ static void ioapic_service(IOAPICCommonState *s)
> >>                      (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
> >>                  if (trig_mode == IOAPIC_TRIGGER_EDGE) {
> >>                      s->irr &= ~mask;
> >> +                    s->ioredtbl[i] &= ~IOAPIC_LVT_REMOTE_IRR;
> >>                  } else {
> >>                      coalesce = s->ioredtbl[i] & IOAPIC_LVT_REMOTE_IRR;
> >>                      s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;
> > 
> > I gave it a quick shot on this but still got the warning. :(
> > 
> > I _guess_ the problem is: the above change is in the "if" block of
> > (s->irr & mask), when the kernel plays the trick of EOI, the irq
> > should be pulled down already by the device (or say, irr bit is
> > cleared). So it does not go into this "if" block.
> 
> No problem; feel free to send the other patch separately and I'll take
> care of merging it.  Otherwise mst can merge it too.

Indeed these two patches are totally independent from the IOMMU
content. I'll send them seperately then. :)

Thanks,

-- peterx

Patch

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 378e663..2443a35 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -72,6 +72,7 @@  static void ioapic_service(IOAPICCommonState *s)
                     (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
                 if (trig_mode == IOAPIC_TRIGGER_EDGE) {
                     s->irr &= ~mask;
+                    s->ioredtbl[i] &= ~IOAPIC_LVT_REMOTE_IRR;
                 } else {
                     coalesce = s->ioredtbl[i] & IOAPIC_LVT_REMOTE_IRR;
                     s->ioredtbl[i] |= IOAPIC_LVT_REMOTE_IRR;