Re: [Qemu-devel] RFC: ioapic polarity vs. qemu os-x guest

[Michael S. Tsirkin]

On Fri, Feb 14, 2014 at 04:13:11PM -0500, Gabriel L. Somlo wrote:
> On Tue, Feb 11, 2014 at 09:54:44PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Feb 11, 2014 at 01:23:31PM -0500, Gabriel L. Somlo wrote:
> > > 1. Regarding KVM and the polarity xor line in the patch above: Does
> > > anyone have experience with any *other* guests which insist on setting
> > > level-triggered interrupt polarity to 1/active-low ? Is that xor line
> > > actually doing anything useful in practice, for any other guest, on
> > > either QEMU or any other platform ?
> > > 
> > > 
> > > 2. Is there anything in QEMU (besides the ACPI DSDT .dsl files) which
> > > has a hardcoded assumption re. "polarity == 0", or active-high, for
> > > level-triggered interrupts? I tried to dig through hw/i386/kvm/ioapic.c
> > > and a bunch of other files, but couldn't isolate anything that I could
> > > "flip" to fix things in userspace.
> > > 
> > > 
> > > Any ideas or suggestions about the appropriate way to move forward would
> > > be much appreciated !!!
> > > 
> > > 
> > > Thanks much,
> > > --Gabriel
> > 
> > I think changing ACPI is the right thing to
> > do really. But we'll need to fix some things
> > first of course.
> 
> So I followed your advice, and was able to boot OS X just fine (but
> booting Linux after this patch still resulted in multiple "no one
> cared" complaints on IRQs 17, 18, 19, etc.:
> 
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index d618e9e..9c52f64 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -353,7 +353,7 @@ DefinitionBlock (
>          Method(IQCR, 1, Serialized) {
>              // _CRS method - get current settings
>              Name(PRR0, ResourceTemplate() {
> -                Interrupt(, Level, ActiveHigh, Shared) { 0 }
> +                Interrupt(, Level, ActiveLow, Shared) { 0 }
>              })
>              CreateDWordField(PRR0, 0x05, PRRI)
>              Store(And(Arg0, 0x0F), PRRI)
> @@ -365,7 +365,7 @@ DefinitionBlock (
>              Name(_HID, EISAID("PNP0C0F"))                       \
>              Name(_UID, uid)                                     \
>              Name(_PRS, ResourceTemplate() {                     \
> -                Interrupt(, Level, ActiveHigh, Shared) {        \
> +                Interrupt(, Level, ActiveLow, Shared) {        \
>                      5, 10, 11                                   \
>                  }                                               \
>              })                                                  \
> @@ -398,12 +398,12 @@ DefinitionBlock (
>              Name(_HID, EISAID("PNP0C0F"))                       \
>              Name(_UID, uid)                                     \
>              Name(_PRS, ResourceTemplate() {                     \
> -                Interrupt(, Level, ActiveHigh, Shared) {        \
> +                Interrupt(, Level, ActiveLow, Shared) {        \
>                      gsi                                         \
>                  }                                               \
>              })                                                  \
>              Name(_CRS, ResourceTemplate() {                     \
> -                Interrupt(, Level, ActiveHigh, Shared) {        \
> +                Interrupt(, Level, ActiveLow, Shared) {        \
>                      gsi                                         \
>                  }                                               \
>              })                                                  \
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 51ce12d..fe1527a 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -206,17 +206,17 @@ static void ich9_lpc_update_pic(ICH9LPCState *lpc, int 
> pic_irq)
>      int i, pic_level;
>  
>      /* The pic level is the logical OR of all the PCI irqs mapped to it */
> -    pic_level = 0;
> +    pic_level = 1;
>      for (i = 0; i < ICH9_LPC_NB_PIRQS; i++) {
>          int tmp_irq;
>          int tmp_dis;
>          ich9_lpc_pic_irq(lpc, i, &tmp_irq, &tmp_dis);
>          if (!tmp_dis && pic_irq == tmp_irq) {
> -            pic_level |= pci_bus_get_irq_level(lpc->d.bus, i);
> +            pic_level &= !pci_bus_get_irq_level(lpc->d.bus, i);
>          }
>      }
>      if (pic_irq == ich9_lpc_sci_irq(lpc)) {
> -        pic_level |= lpc->sci_level;
> +        pic_level &= !lpc->sci_level;
>      }
>  
>      qemu_set_irq(lpc->pic[pic_irq], pic_level);
> --
> 
> However, even on OS X, the Ethernet (e1000) card won't link up at all.
> Fixing that requires another patch:
> 
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 58ba93b..c7a2c07 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -301,7 +301,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t 
> val)
>      s->mac_reg[ICS] = val;
>  
>      pending_ints = (s->mac_reg[IMS] & s->mac_reg[ICR]);
> -    if (!s->mit_irq_level && pending_ints) {
> +    if (s->mit_irq_level && pending_ints) {
>          /*
>           * Here we detect a potential raising edge. We postpone raising the
>           * interrupt line if we are inside the mitigation delay window
> @@ -339,7 +339,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t 
> val)
>          }
>      }
>  
> -    s->mit_irq_level = (pending_ints != 0);
> +    s->mit_irq_level = (pending_ints == 0);
>      pci_set_irq(d, s->mit_irq_level);
>  }
>  
> @@ -393,7 +393,7 @@ static void e1000_reset(void *opaque)
>      timer_del(d->autoneg_timer);
>      timer_del(d->mit_timer);
>      d->mit_timer_on = 0;
> -    d->mit_irq_level = 0;
> +    d->mit_irq_level = 1;
>      d->mit_ide = 0;
>      memset(d->phy_reg, 0, sizeof d->phy_reg);
>      memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init);
> @@ -1324,7 +1324,7 @@ static int e1000_post_load(void *opaque, int version_id)
>      if (!(s->compat_flags & E1000_FLAG_MIT)) {
>          s->mac_reg[ITR] = s->mac_reg[RDTR] = s->mac_reg[RADV] =
>              s->mac_reg[TADV] = 0;
> -        s->mit_irq_level = false;
> +        s->mit_irq_level = true;
>      }
>      s->mit_ide = 0;
>      s->mit_timer_on = false;

Hmm no this is all wrong, from API point of view,
devices shoud not care about value of interrupt.
They just assert/deassert interrupts.
It so happens that 1 means assert 0 means deassert.
It's the PCI host or the PIC that needs to flip it.
It can't be host ATM because PIC does the logical OR
so it must be encoded as 1 == assert.

So how about simply
-      qemu_set_irq(lpc->pic[pic_irq], pic_level);
+      qemu_set_irq(lpc->pic[pic_irq], !pic_level);
plus the ACPI tweaks ... ?

> ---
> 
> At this point, I'm beginning to realize that the "ActiveHigh"
> assumption is rather pervasively baked in throughout the QEMU
> source code...
> 
> And I'm wondering whether a ton of changes to make it either
> programatically configurable or change the hard-codded assumption
> to "ActiveLow" would be feasible, welcome, etc... I personally
> would prefer configurable
> (e.g. "-machine foo,kernel_irqchip=on,polarity=high", or some such).
> 
> Thanks much for any ideas,
> --Gabriel

I don't think we need to make this configurable unless
there's real hardware that makes PCI active low.
Don't give up yet :)

-- 
MST