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

[Paolo Bonzini]

Il 17/02/2014 19:01, Gabriel L. Somlo ha scritto:
> On Mon, Feb 17, 2014 at 12:57:00PM -0500, Gabriel L. Somlo wrote:
> > On Sun, Feb 16, 2014 at 06:23:11PM +0200, Michael S. Tsirkin wrote:
> > > Well there is a bigger issue: any interrupt with
> > > multiple sources is broken.
> > >
> > > __kvm_irq_line_state does a logical OR of all sources,
> > > before XOR with polarity.
> > >
> > > This makes no sense if polarity is active low.
> >
> > So, do you think something like this would make sense, to address
> > active-low polarity in __kvm_irq_line_state ?
> > (this would be independent of the subsequent xor in
> > kvm_ioapic_set_irq()):
>
> Make that rather:
>
> -static inline int __kvm_irq_line_state(unsigned long *irq_state,
> +static inline int __kvm_irq_line_state(unsigned long *irq_state, int polarity,
>                                         int irq_source_id, int level)
> {
> -        /* Logical OR for level trig interrupt */
>         if (level)
>                 __set_bit(irq_source_id, irq_state);
>         else
>                 __clear_bit(irq_source_id, irq_state);
>
> -       return !!(*irq_state);
> +       if (polarity) {
> +               /* Logical AND for level trig interrupt, active-low */
> +               return !~(*irq_state);

This is ~*irq_state == 0, i.e. *irq_state == ~0.

What if high-order bits of *irq_state are never used?  That is, do you 
need to consider the maximum valid irq_source_id too?


> +       } else {
> +               /* Logical OR for level trig interrupt, active-high */
> +               return !!(*irq_state);

Better rewrite this as *irq_state != 0.

Paolo

> +       }
> }
>
> Thanks, and sorry for the noise :)
> --Gabriel