| Message ID | 20140214220600.GI29329@ERROL.INI.CMU.EDU (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14.02.2014, at 23:06, Gabriel L. Somlo <gsomlo@gmail.com> wrote: > On Fri, Feb 14, 2014 at 10:21:09PM +0100, Alexander Graf wrote: >> >> Can't you just turn the polarity around in the pci host adapter? > > I tried this: > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 1221f32..0e86d21 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -118,13 +118,13 @@ static int pci_bar(PCIDevice *d, int reg) > > static inline int pci_irq_state(PCIDevice *d, int irq_num) > { > - return (d->irq_state >> irq_num) & 0x1; > + return !(d->irq_state >> irq_num) & 0x1; > } > > static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) > { > d->irq_state &= ~(0x1 << irq_num); > - d->irq_state |= level << irq_num; > + d->irq_state &= ~(level << irq_num); > } > > static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) > @@ -229,7 +229,7 @@ static void pcibus_reset(BusState *qbus) > } > > for (i = 0; i < bus->nirq; i++) { > - assert(bus->irq_count[i] == 0); > + assert(bus->irq_count[i] != 0); > } > } > > --- > > but now OS X freezes during boot right after > > [ PCI configuration begin ] > [ PCI configuration end, bridges 1, devices 10 ] > RTC: Only single RAM bank (128 bytes) > > which all looks normal, except the process is supposed to continue on > from there and doesn't :) > > On Linux, I get Fedora 20 live all the way up with no obvious/loud > complaints, but mouse and keyboard don't work at all... > > I have to admit I'm a bit out of my depth here, though :) Yeah, another thing we have to take into account is vhost-net which generates IRQs directly through irqfd. I guess for those we'll have to configure the polarity in the irq routing table? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 14, 2014 at 11:13:04PM +0100, Alexander Graf wrote: > > On 14.02.2014, at 23:06, Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > > On Fri, Feb 14, 2014 at 10:21:09PM +0100, Alexander Graf wrote: > >> > >> Can't you just turn the polarity around in the pci host adapter? > > > > I tried this: > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 1221f32..0e86d21 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -118,13 +118,13 @@ static int pci_bar(PCIDevice *d, int reg) > > > > static inline int pci_irq_state(PCIDevice *d, int irq_num) > > { > > - return (d->irq_state >> irq_num) & 0x1; > > + return !(d->irq_state >> irq_num) & 0x1; > > } > > > > static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) > > { > > d->irq_state &= ~(0x1 << irq_num); > > - d->irq_state |= level << irq_num; > > + d->irq_state &= ~(level << irq_num); > > } > > > > static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) > > @@ -229,7 +229,7 @@ static void pcibus_reset(BusState *qbus) > > } > > > > for (i = 0; i < bus->nirq; i++) { > > - assert(bus->irq_count[i] == 0); > > + assert(bus->irq_count[i] != 0); > > } > > } > > > > --- > > > > but now OS X freezes during boot right after > > > > [ PCI configuration begin ] > > [ PCI configuration end, bridges 1, devices 10 ] > > RTC: Only single RAM bank (128 bytes) > > > > which all looks normal, except the process is supposed to continue on > > from there and doesn't :) > > > > On Linux, I get Fedora 20 live all the way up with no obvious/loud > > complaints, but mouse and keyboard don't work at all... > > > > I have to admit I'm a bit out of my depth here, though :) > > Yeah, another thing we have to take into account is vhost-net which generates IRQs directly through irqfd. I guess for those we'll have to configure the polarity in the irq routing table? > > > Alex This is using MSI-X interrupts which are edge though, not going through IOAPIC at all. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 14, 2014 at 11:13:04PM +0100, Alexander Graf wrote: > > On 14.02.2014, at 23:06, Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > > On Fri, Feb 14, 2014 at 10:21:09PM +0100, Alexander Graf wrote: > >> > >> Can't you just turn the polarity around in the pci host adapter? > > > > I tried this: > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 1221f32..0e86d21 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -118,13 +118,13 @@ static int pci_bar(PCIDevice *d, int reg) > > > > static inline int pci_irq_state(PCIDevice *d, int irq_num) > > { > > - return (d->irq_state >> irq_num) & 0x1; > > + return !(d->irq_state >> irq_num) & 0x1; > > } > > > > static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) > > { > > d->irq_state &= ~(0x1 << irq_num); > > - d->irq_state |= level << irq_num; > > + d->irq_state &= ~(level << irq_num); > > } > > > > static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) > > @@ -229,7 +229,7 @@ static void pcibus_reset(BusState *qbus) > > } > > > > for (i = 0; i < bus->nirq; i++) { > > - assert(bus->irq_count[i] == 0); > > + assert(bus->irq_count[i] != 0); > > } > > } > > > > --- > > > > but now OS X freezes during boot right after > > > > [ PCI configuration begin ] > > [ PCI configuration end, bridges 1, devices 10 ] > > RTC: Only single RAM bank (128 bytes) > > > > which all looks normal, except the process is supposed to continue on > > from there and doesn't :) > > > > On Linux, I get Fedora 20 live all the way up with no obvious/loud > > complaints, but mouse and keyboard don't work at all... > > > > I have to admit I'm a bit out of my depth here, though :) > > Yeah, another thing we have to take into account is vhost-net which generates IRQs directly through irqfd. I guess for those we'll have to configure the polarity in the irq routing table? > > > Alex What will be affected is VFIO which uses IRQFD for level interrupts with KVM_IRQFD_FLAG_RESAMPLE. I suspect this will need a kernel change, maybe a new flag for IRQFD: KVM_IRQFD_FLAG_ACTIVE_LOW, since at the moment that does: static void irqfd_inject(struct work_struct *work) { struct _irqfd *irqfd = container_of(work, struct _irqfd, inject); struct kvm *kvm = irqfd->kvm; if (!irqfd->resampler) { kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1, false); kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0, false); } else kvm_set_irq(kvm, KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID, irqfd->gsi, 1, false); }
On Sun, 2014-02-16 at 13:41 +0200, Michael S. Tsirkin wrote: > On Fri, Feb 14, 2014 at 11:13:04PM +0100, Alexander Graf wrote: > > > > On 14.02.2014, at 23:06, Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > > > > On Fri, Feb 14, 2014 at 10:21:09PM +0100, Alexander Graf wrote: > > >> > > >> Can't you just turn the polarity around in the pci host adapter? > > > > > > I tried this: > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > index 1221f32..0e86d21 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -118,13 +118,13 @@ static int pci_bar(PCIDevice *d, int reg) > > > > > > static inline int pci_irq_state(PCIDevice *d, int irq_num) > > > { > > > - return (d->irq_state >> irq_num) & 0x1; > > > + return !(d->irq_state >> irq_num) & 0x1; > > > } > > > > > > static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) > > > { > > > d->irq_state &= ~(0x1 << irq_num); > > > - d->irq_state |= level << irq_num; > > > + d->irq_state &= ~(level << irq_num); > > > } > > > > > > static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) > > > @@ -229,7 +229,7 @@ static void pcibus_reset(BusState *qbus) > > > } > > > > > > for (i = 0; i < bus->nirq; i++) { > > > - assert(bus->irq_count[i] == 0); > > > + assert(bus->irq_count[i] != 0); > > > } > > > } > > > > > > --- > > > > > > but now OS X freezes during boot right after > > > > > > [ PCI configuration begin ] > > > [ PCI configuration end, bridges 1, devices 10 ] > > > RTC: Only single RAM bank (128 bytes) > > > > > > which all looks normal, except the process is supposed to continue on > > > from there and doesn't :) > > > > > > On Linux, I get Fedora 20 live all the way up with no obvious/loud > > > complaints, but mouse and keyboard don't work at all... > > > > > > I have to admit I'm a bit out of my depth here, though :) > > > > Yeah, another thing we have to take into account is vhost-net which generates IRQs directly through irqfd. I guess for those we'll have to configure the polarity in the irq routing table? > > > > > > Alex > > What will be affected is VFIO which uses IRQFD > for level interrupts with KVM_IRQFD_FLAG_RESAMPLE. > I suspect this will need a kernel change, maybe > a new flag for IRQFD: KVM_IRQFD_FLAG_ACTIVE_LOW, > since at the moment that does: > > static void > irqfd_inject(struct work_struct *work) > { > struct _irqfd *irqfd = container_of(work, struct _irqfd, inject); > struct kvm *kvm = irqfd->kvm; > > if (!irqfd->resampler) { > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1, > false); > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0, > false); > } else > kvm_set_irq(kvm, KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID, > irqfd->gsi, 1, false); > } As you said in a previous message, devices just want assert & de-assert, 1 & 0, which is what we have here. I would think that what asserted means only needs to be interpreted at the IOAPIC, so I'd hope we could get it right w/o an API change. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 16, 2014 at 07:47:00AM -0700, Alex Williamson wrote: > On Sun, 2014-02-16 at 13:41 +0200, Michael S. Tsirkin wrote: > > On Fri, Feb 14, 2014 at 11:13:04PM +0100, Alexander Graf wrote: > > > > > > On 14.02.2014, at 23:06, Gabriel L. Somlo <gsomlo@gmail.com> wrote: > > > > > > > On Fri, Feb 14, 2014 at 10:21:09PM +0100, Alexander Graf wrote: > > > >> > > > >> Can't you just turn the polarity around in the pci host adapter? > > > > > > > > I tried this: > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > > index 1221f32..0e86d21 100644 > > > > --- a/hw/pci/pci.c > > > > +++ b/hw/pci/pci.c > > > > @@ -118,13 +118,13 @@ static int pci_bar(PCIDevice *d, int reg) > > > > > > > > static inline int pci_irq_state(PCIDevice *d, int irq_num) > > > > { > > > > - return (d->irq_state >> irq_num) & 0x1; > > > > + return !(d->irq_state >> irq_num) & 0x1; > > > > } > > > > > > > > static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) > > > > { > > > > d->irq_state &= ~(0x1 << irq_num); > > > > - d->irq_state |= level << irq_num; > > > > + d->irq_state &= ~(level << irq_num); > > > > } > > > > > > > > static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) > > > > @@ -229,7 +229,7 @@ static void pcibus_reset(BusState *qbus) > > > > } > > > > > > > > for (i = 0; i < bus->nirq; i++) { > > > > - assert(bus->irq_count[i] == 0); > > > > + assert(bus->irq_count[i] != 0); > > > > } > > > > } > > > > > > > > --- > > > > > > > > but now OS X freezes during boot right after > > > > > > > > [ PCI configuration begin ] > > > > [ PCI configuration end, bridges 1, devices 10 ] > > > > RTC: Only single RAM bank (128 bytes) > > > > > > > > which all looks normal, except the process is supposed to continue on > > > > from there and doesn't :) > > > > > > > > On Linux, I get Fedora 20 live all the way up with no obvious/loud > > > > complaints, but mouse and keyboard don't work at all... > > > > > > > > I have to admit I'm a bit out of my depth here, though :) > > > > > > Yeah, another thing we have to take into account is vhost-net which generates IRQs directly through irqfd. I guess for those we'll have to configure the polarity in the irq routing table? > > > > > > > > > Alex > > > > What will be affected is VFIO which uses IRQFD > > for level interrupts with KVM_IRQFD_FLAG_RESAMPLE. > > I suspect this will need a kernel change, maybe > > a new flag for IRQFD: KVM_IRQFD_FLAG_ACTIVE_LOW, > > since at the moment that does: > > > > static void > > irqfd_inject(struct work_struct *work) > > { > > struct _irqfd *irqfd = container_of(work, struct _irqfd, inject); > > struct kvm *kvm = irqfd->kvm; > > > > if (!irqfd->resampler) { > > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1, > > false); > > kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0, > > false); > > } else > > kvm_set_irq(kvm, KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID, > > irqfd->gsi, 1, false); > > } > > > > As you said in a previous message, devices just want assert & de-assert, > 1 & 0, which is what we have here. I would think that what asserted > means only needs to be interpreted at the IOAPIC, so I'd hope we could > get it right w/o an API change. 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. One is beginning to think the simplest fix would be Gabriel's patch after all: - irq_level ^= entry.fields.polarity; although it's ugly in that it perpetuates the bug in more places instead of fixing it. > Thanks, > > Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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()): -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 OR for level trig interrupt, active-high */ + return !!(*irq_state); + } else { // active-low + /* Logical AND for level trig interrupt, active-low */ + return !~(*irq_state); + } } Thanks, --Gabriel -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + } else { + /* Logical OR for level trig interrupt, active-high */ + return !!(*irq_state); + } } Thanks, and sorry for the noise :) --Gabriel -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 17, 2014 at 07:06:11PM +0100, Paolo Bonzini wrote: > 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()): > > > >- 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? Oh, I think I'm starting to comprehend the problem here. The bits of "*irq_state" are indexed by "irq_source_id", which is dynamically assigned by kvm_request_irq_source_id(). So, doing the OR thing when assuming always-active-high makes sense. Doing AND based on an active-low assumption doesn't make sense, because there could ALWAYS be 0 bits that just weren't allocated (yet), and I'm having trouble imagining how I'd keep track of where the current allocation boundary is in a sane way :) Which I *think* was Michael's original point... --Gabriel -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 17, 2014 at 02:38:09PM -0500, Gabriel L. Somlo wrote: > Oh, I think I'm starting to comprehend the problem here. The bits of > "*irq_state" are indexed by "irq_source_id", which is dynamically > assigned by kvm_request_irq_source_id(). > > So, doing the OR thing when assuming always-active-high makes > sense. Doing AND based on an active-low assumption doesn't make > sense, because there could ALWAYS be 0 bits that just weren't > allocated (yet), and I'm having trouble imagining how I'd keep > track of where the current allocation boundary is in a sane way :) Hmm, I thought maybe I could use kvm->arch.irq_sources_bitmap, but that's global across the whole VM, whereas irq_state belongs to one given GSI. So, the per-GSI bitmap is sparse, so it's at least as bad as I thought earlier, if not worse :) Am I missing anything that would put this in a better light ? Thanks, --Gabriel -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 1221f32..0e86d21 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -118,13 +118,13 @@ static int pci_bar(PCIDevice *d, int reg) static inline int pci_irq_state(PCIDevice *d, int irq_num) { - return (d->irq_state >> irq_num) & 0x1; + return !(d->irq_state >> irq_num) & 0x1; } static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) { d->irq_state &= ~(0x1 << irq_num); - d->irq_state |= level << irq_num; + d->irq_state &= ~(level << irq_num); } static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) @@ -229,7 +229,7 @@ static void pcibus_reset(BusState *qbus) } for (i = 0; i < bus->nirq; i++) { - assert(bus->irq_count[i] == 0); + assert(bus->irq_count[i] != 0); } }