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

[Paolo Bonzini]

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" <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> >
> > ---
> >
> > 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.  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