Re: [Qemu-devel] [PATCH RFC] kvm: ignore apic polarity

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 27, 2014 at 11:30:55PM +0100, Paolo Bonzini wrote:
> Il 27/02/2014 22:41, Gabriel L. Somlo ha scritto:
> >On Thu, Feb 27, 2014 at 07:05:49PM +0200, Michael S. Tsirkin wrote:
> >>apic polarity in KVM does not work: too many things assume active high.
> >>Let's not pretend it works, let's just ignore polarity flag.  If we ever
> >>want to emulate it exactly, this will need a feature flag anyway.
> >>
> >>Also report this to userspace: this makes it
> >>possible to report the interrupt active-low
> >>in ACPI, this way we are closer to real hardware.
> >>
> >>This patch fixes OSX running on KVM.
> >>
> >>Reported-by: "Gabriel L. Somlo" <gsomlo@gmail.com>
> >>Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>---
> >>
> >>Gabriel, could you confirm this fixes OSX for you?
> >>If you can play with linux tweaking interrupt
> >>to active low, that would be very nice too:
> >>it's weekend here.
> >
> >With Fedora 20 Live x86_64:
> >
> >If all I do is 's/ActiveHigh/ActiveLow/' in hw/i386/[q36-]acpi-dsdt.dsl,
> >but otherwise don't try to change how QEMU deals with "logical" vs.
> >"physical" ioapic polarity, things work great. Printk's show polarity
> >set to 1, but with the ignore-polarity patch things work fine.
> >
> >With normal (ActiveHigh) ACPI, printk reports polarity set to 0, and
> >things *still* work exactly the same.
> >
> >
> >So, the way I understand this (and I'm writing this mainly for myself,
> >to make sure I understand correctly, so please kick me if I got it
> >wrong), ACPI tells the guest OS how to configure "physical" ioapic polarity.
> >
> >With ActiveHigh, "physical" == "logical", i.e. "high" == "asserted"
> >and "low" == "deasserted".
> >
> >With ActiveLow, "physical" == !"logical", so the other way around.
> >
> >QEMU being hard-coded to ActiveHigh is the moral equivalent of always
> >sending the kernel (KVM) "logical" line states, rather than "physical"
> >ones.
> >
> >Assuming KVM's userland clients are all coded for ActiveHigh, we can
> >(should, for sanity's sake) just assume line states from userland are
> >logical, and stop paying attention the polarity bits. That way,
> >misbehaving guests [*] can configure their ioapics as they please, and
> >things will just work OK regardless.
> >
> >As you pointed out earlier, even KVM itself already kind-of assumes
> >ActiveHigh (e.g. in __kvm_irq_line_state(), which should be coded
> >differently if ActiveLow were a serious possibility, and, BTW,
> >irq_states[irq] would probably have to be initialized to all-1's if
> >ActiveLow wre used, etc, etc).
> 
> This is a much better description.  Can you turn it into a patch to
> Documentation/virtual/kvm/api.txt and a more complete commit
> message?
> 
> Also, there is a problem in this: we definitely do not want to have
> different ACPI tables for TCG vs. KVM.

Why?

We already have different ACPI tables for CG vs KVM.
Specifically apic interrupt override flag in MADT is set
for KVM but not TCG.

As KVM hardware differs from TCG, I expect them to move
further apart with time.

> Have you guys tested what
> happens with Linux guests + TCG if interrupts are declared
> active-low?
> 
> QEMU likely has many other places that hard-code active-high.  One
> approach could be to add a QOM property to the ioapic that is a
> bitmask of which GSIs are active-low.  The ioapic can consult it
> like this:
> 
>      if (vector >= 0 && vector < IOAPIC_NUM_PINS) {
>          uint32_t mask = 1 << vector;
>          uint64_t entry = s->ioredtbl[vector];
> 
>          if (entry & (1 << IOAPIC_LVT_POLARITY_SHIFT)) {
>              level = !level;
>          }
> +        if (s->active_low_mask & (1 << vector)) {
> +            level = !level;
> +        }
>          if (((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1) ==
>              IOAPIC_TRIGGER_LEVEL) {
>              /* level triggered */
> 
> etc. so that the two NOTs undo each other, making the input to
> QEMU's ioapic also "logical" rather than "physical".
> 
> Paolo

Or we can do a patch like we did for kvm and ignore polarity,
treating levels as logical rather than physical.

-- 
MST