| Message ID | 20161120231605.D90DD745789@zero.eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
On 20.11.2016 15:12, BALATON Zoltan wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > It's currently broken as it uses an incorrect shift, it tries > to use the slot number but uses the top bits of the bus number > instead. > > Note: Neither implementation matches what OpenBIOS ends up putting > in the device-tree either, which will have to be fixed separately. > > This is not quite correct for modelling a real Mac since Apple > tend to tie all 4 interrupt lines of a slot together and have > separate interrupts for every slot and every motherboard devices > going straight to the PIC but we'll sort that out later. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > hw/pci-host/uninorth.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > This needs a corresponding fix in OpenBIOS but this has to be > committed first for that. As this is already broken making this change > should not make things worse as they are now. Could we get this in now > as a bugfix commit? > > diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c > index 7aac4d6..df342ac 100644 > --- a/hw/pci-host/uninorth.c > +++ b/hw/pci-host/uninorth.c > @@ -62,9 +62,7 @@ typedef struct UNINState { > > static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num) > { > - int devfn = pci_dev->devfn & 0x00FFFFFF; > - > - return (((devfn >> 11) & 0x1F) + irq_num) & 3; > + return (irq_num + (pci_dev->devfn >> 3)) & 3; > } > > static void pci_unin_set_irq(void *opaque, int irq_num, int level) > Looks reasonable. Reviewed-by: Thomas Huth <thuth@redhat.com>
On Mon, Nov 21, 2016 at 12:12:31AM +1000, BALATON Zoltan wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > It's currently broken as it uses an incorrect shift, it tries > to use the slot number but uses the top bits of the bus number > instead. > > Note: Neither implementation matches what OpenBIOS ends up putting > in the device-tree either, which will have to be fixed separately. > > This is not quite correct for modelling a real Mac since Apple > tend to tie all 4 interrupt lines of a slot together and have > separate interrupts for every slot and every motherboard devices > going straight to the PIC but we'll sort that out later. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > hw/pci-host/uninorth.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > This needs a corresponding fix in OpenBIOS but this has to be > committed first for that. As this is already broken making this change > should not make things worse as they are now. Could we get this in now > as a bugfix commit? Applied to ppc-for-2.8. > > diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c > index 7aac4d6..df342ac 100644 > --- a/hw/pci-host/uninorth.c > +++ b/hw/pci-host/uninorth.c > @@ -62,9 +62,7 @@ typedef struct UNINState { > > static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num) > { > - int devfn = pci_dev->devfn & 0x00FFFFFF; > - > - return (((devfn >> 11) & 0x1F) + irq_num) & 3; > + return (irq_num + (pci_dev->devfn >> 3)) & 3; > } > > static void pci_unin_set_irq(void *opaque, int irq_num, int level)
On 21/11/16 23:05, David Gibson wrote: > On Mon, Nov 21, 2016 at 12:12:31AM +1000, BALATON Zoltan wrote: >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> >> It's currently broken as it uses an incorrect shift, it tries >> to use the slot number but uses the top bits of the bus number >> instead. >> >> Note: Neither implementation matches what OpenBIOS ends up putting >> in the device-tree either, which will have to be fixed separately. >> >> This is not quite correct for modelling a real Mac since Apple >> tend to tie all 4 interrupt lines of a slot together and have >> separate interrupts for every slot and every motherboard devices >> going straight to the PIC but we'll sort that out later. >> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> --- >> hw/pci-host/uninorth.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> This needs a corresponding fix in OpenBIOS but this has to be >> committed first for that. As this is already broken making this change >> should not make things worse as they are now. Could we get this in now >> as a bugfix commit? > > Applied to ppc-for-2.8. Is this going in for 2.8? If so, I'll need to apply the corresponding patch to OpenBIOS to match and also do a PPC testing cycle to make sure that there are no regressions on other OSs. Plus it would be useful to get both pull requests in close proximity as this will break bisection. ATB, Mark.
On Tue, 22 Nov 2016, Mark Cave-Ayland wrote: > On 21/11/16 23:05, David Gibson wrote: >> On Mon, Nov 21, 2016 at 12:12:31AM +1000, BALATON Zoltan wrote: >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>> >>> It's currently broken as it uses an incorrect shift, it tries >>> to use the slot number but uses the top bits of the bus number >>> instead. >>> >>> Note: Neither implementation matches what OpenBIOS ends up putting >>> in the device-tree either, which will have to be fixed separately. >>> >>> This is not quite correct for modelling a real Mac since Apple >>> tend to tie all 4 interrupt lines of a slot together and have >>> separate interrupts for every slot and every motherboard devices >>> going straight to the PIC but we'll sort that out later. >>> >>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>> --- >>> hw/pci-host/uninorth.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> This needs a corresponding fix in OpenBIOS but this has to be >>> committed first for that. As this is already broken making this change >>> should not make things worse as they are now. Could we get this in now >>> as a bugfix commit? >> >> Applied to ppc-for-2.8. > > Is this going in for 2.8? If so, I'll need to apply the corresponding > patch to OpenBIOS to match and also do a PPC testing cycle to make sure > that there are no regressions on other OSs. Plus it would be useful to > get both pull requests in close proximity as this will break bisection. (Adding BenH as these are his patches I just apparently forgot the cc.) I'd appreciate if this could get in 2.8. As this is a bugfix this should be OK during the freeze. Likely there will be more rc cycles where it could be reverted if found to cause problems. But since it's already broken, applying this patch to QEMU should not break it more and adding the fix on the OpenBIOS side (patch 4/4) would complete the fix and may actually make it work. This should only affect mac99 and this one patch is independent from the other three I've sent to OpenBIOS so this could be applied independently to get at least the interrupts fixed if you can't test and apply the other three until the next pull. Regards, BALATON Zoltan
On Tue, 2016-11-22 at 02:34 +0100, BALATON Zoltan wrote: > > Is this going in for 2.8? If so, I'll need to apply the corresponding > > patch to OpenBIOS to match and also do a PPC testing cycle to make sure > > that there are no regressions on other OSs. Plus it would be useful to > > get both pull requests in close proximity as this will break bisection. > > (Adding BenH as these are his patches I just apparently forgot the cc.) > > I'd appreciate if this could get in 2.8. As this is a bugfix this should > be OK during the freeze. Likely there will be more rc cycles where it > could be reverted if found to cause problems. But since it's already > broken, applying this patch to QEMU should not break it more and adding > the fix on the OpenBIOS side (patch 4/4) would complete the fix and may > actually make it work. This should only affect mac99 and this one patch is > independent from the other three I've sent to OpenBIOS so this could be > applied independently to get at least the interrupts fixed if you can't > test and apply the other three until the next pull. Right, PCI IRQs are terminally broken already. This patch without the OpenBIOS change won't hurt. With the OpenBIOS change, things start working better :-) I appologize for being basically MIA these days, way too busy with other things. Feel free to pick up ownership of my patches if you want to get them into upstreamable state, such as the SunGEM one. I don't know if I'll have any bandwidth to work on that stuff again until later next year. Cheers, Ben.
diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c index 7aac4d6..df342ac 100644 --- a/hw/pci-host/uninorth.c +++ b/hw/pci-host/uninorth.c @@ -62,9 +62,7 @@ typedef struct UNINState { static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num) { - int devfn = pci_dev->devfn & 0x00FFFFFF; - - return (((devfn >> 11) & 0x1F) + irq_num) & 3; + return (irq_num + (pci_dev->devfn >> 3)) & 3; } static void pci_unin_set_irq(void *opaque, int irq_num, int level)