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);
>  }
>  

If we really wanted to change it, we could
change pci_set_irq to reverse the polarity.
But I think doing this in PIC is cleaner.


> @@ -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;
> ---
> 
> 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