| Message ID | 20230223202053.117050-3-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VT82xx PCI fixes and audio output support | expand |
On Thu, 23 Feb 2023, Bernhard Beschow wrote: > The real VIA south bridges implement a PCI IRQ router which is configured > by the BIOS or the OS. In order to respect these configurations, QEMU > needs to implement it as well. > > Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 3f9bd0c04d..f24e387d63 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level) > qemu_set_irq(s->cpu_intr, level); > } > > +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) > +{ > + switch (irq_num) { > + case 0: > + return s->dev.config[0x55] >> 4; > + > + case 1: > + return s->dev.config[0x56] & 0xf; > + > + case 2: > + return s->dev.config[0x56] >> 4; > + > + case 3: > + return s->dev.config[0x57] >> 4; > + } > + > + return 0; > +} > + > +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) > +{ > + ViaISAState *s = opaque; > + PCIBus *bus = pci_get_bus(&s->dev); > + int pic_irq; > + > + /* now we change the pic irq level according to the via irq mappings */ > + /* XXX: optimize */ > + pic_irq = via_isa_get_pci_irq(s, irq_num); > + if (pic_irq < ISA_NUM_IRQS) { > + int i, pic_level; > + > + /* The pic level is the logical OR of all the PCI irqs mapped to it. */ > + pic_level = 0; > + for (i = 0; i < PCI_NUM_PINS; i++) { > + if (pic_irq == via_isa_get_pci_irq(s, i)) { > + pic_level |= pci_bus_get_irq_level(bus, i); > + } > + } > + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); > + } > +} > + > static void via_isa_realize(PCIDevice *d, Error **errp) > { > ViaISAState *s = VIA_ISA(d); > @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) > if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { > return; > } > + > + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); Please no oversimplification. This replaces the connections to mv64361 gpp pins made in mv64361_realize() and breaks the interrupts that can be enabled in mv64361. I've implemented that for something but can't remember now which guest exactly, but this would be needed so please restore my pegasos2 patch and move this there connecting both mv64361 and via-isa to PCI interrupts as shown in the schematics. That means you also need the PIRQ pins here. Can you do a new version with that? I'll try this one in the meantime but I'm quite sure this is wrong as it is. You can drop the via-ac97 patches from this series, I can submit them separately rebased on the final IRQ series we come up with. Regards, BALATON Zoltan > } > > /* TYPE_VT82C686B_ISA */ >
Am 23. Februar 2023 21:11:23 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Thu, 23 Feb 2023, Bernhard Beschow wrote: >> The real VIA south bridges implement a PCI IRQ router which is configured >> by the BIOS or the OS. In order to respect these configurations, QEMU >> needs to implement it as well. >> >> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 3f9bd0c04d..f24e387d63 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >> qemu_set_irq(s->cpu_intr, level); >> } >> >> +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) >> +{ >> + switch (irq_num) { >> + case 0: >> + return s->dev.config[0x55] >> 4; >> + >> + case 1: >> + return s->dev.config[0x56] & 0xf; >> + >> + case 2: >> + return s->dev.config[0x56] >> 4; >> + >> + case 3: >> + return s->dev.config[0x57] >> 4; >> + } >> + >> + return 0; >> +} >> + >> +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) >> +{ >> + ViaISAState *s = opaque; >> + PCIBus *bus = pci_get_bus(&s->dev); >> + int pic_irq; >> + >> + /* now we change the pic irq level according to the via irq mappings */ >> + /* XXX: optimize */ >> + pic_irq = via_isa_get_pci_irq(s, irq_num); >> + if (pic_irq < ISA_NUM_IRQS) { >> + int i, pic_level; >> + >> + /* The pic level is the logical OR of all the PCI irqs mapped to it. */ >> + pic_level = 0; >> + for (i = 0; i < PCI_NUM_PINS; i++) { >> + if (pic_irq == via_isa_get_pci_irq(s, i)) { >> + pic_level |= pci_bus_get_irq_level(bus, i); >> + } >> + } >> + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); >> + } >> +} >> + >> static void via_isa_realize(PCIDevice *d, Error **errp) >> { >> ViaISAState *s = VIA_ISA(d); >> @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { >> return; >> } >> + >> + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); > >Please no oversimplification. This replaces the connections to mv64361 gpp pins made in mv64361_realize() and breaks the interrupts that can be enabled in mv64361. Let's split our work as follows: You'll do the audio and pegasos2 changes including exporting the pirq properties, I'll do the first three routing patches of this series as the base. > I've implemented that for something but can't remember now which guest exactly, Could you please put this information into the commit message or into the code? That would ease maintainance a lot. > but this would be needed so please restore my pegasos2 patch and move this there connecting both mv64361 and via-isa to PCI interrupts as shown in the schematics. That means you also need the PIRQ pins here. Can you do a new version with that? As proposed above I'd fold the first three patches into a separate series which you can build upon. I have no way to test the pegasos2 IRQ changes since the test cases I'm aware of either work or we agreed that they can be fixed later (-> USB). > I'll try this one in the meantime Sounds good to me -- that's what I wanted to achieve ;) Thanks! Best regards, Bernhard > but I'm quite sure this is wrong as it is. You can drop the via-ac97 patches from this series, I can submit them separately rebased on the final IRQ series we come up with. > >Regards, >BALATON Zoltan > >> } >> >> /* TYPE_VT82C686B_ISA */ >>
On Thu, 23 Feb 2023, BALATON Zoltan wrote: > On Thu, 23 Feb 2023, Bernhard Beschow wrote: >> The real VIA south bridges implement a PCI IRQ router which is configured >> by the BIOS or the OS. In order to respect these configurations, QEMU >> needs to implement it as well. >> >> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 3f9bd0c04d..f24e387d63 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, >> int irq, int level) >> qemu_set_irq(s->cpu_intr, level); >> } >> >> +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) >> +{ >> + switch (irq_num) { >> + case 0: >> + return s->dev.config[0x55] >> 4; >> + >> + case 1: >> + return s->dev.config[0x56] & 0xf; >> + >> + case 2: >> + return s->dev.config[0x56] >> 4; >> + >> + case 3: >> + return s->dev.config[0x57] >> 4; >> + } >> + >> + return 0; >> +} >> + >> +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) >> +{ >> + ViaISAState *s = opaque; >> + PCIBus *bus = pci_get_bus(&s->dev); >> + int pic_irq; >> + >> + /* now we change the pic irq level according to the via irq mappings >> */ >> + /* XXX: optimize */ >> + pic_irq = via_isa_get_pci_irq(s, irq_num); >> + if (pic_irq < ISA_NUM_IRQS) { >> + int i, pic_level; >> + >> + /* The pic level is the logical OR of all the PCI irqs mapped to >> it. */ >> + pic_level = 0; >> + for (i = 0; i < PCI_NUM_PINS; i++) { >> + if (pic_irq == via_isa_get_pci_irq(s, i)) { >> + pic_level |= pci_bus_get_irq_level(bus, i); >> + } >> + } >> + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); >> + } >> +} >> + >> static void via_isa_realize(PCIDevice *d, Error **errp) >> { >> ViaISAState *s = VIA_ISA(d); >> @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { >> return; >> } >> + >> + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); > > Please no oversimplification. This replaces the connections to mv64361 gpp > pins made in mv64361_realize() and breaks the interrupts that can be enabled > in mv64361. I've implemented that for something but can't remember now which > guest exactly, but this would be needed so please restore my pegasos2 patch > and move this there connecting both mv64361 and via-isa to PCI interrupts as > shown in the schematics. That means you also need the PIRQ pins here. Can you > do a new version with that? I'll try this one in the meantime but I'm quite > sure this is wrong as it is. You can drop the via-ac97 patches from this > series, I can submit them separately rebased on the final IRQ series we come > up with. We'd also need the workaround I've posted yesterday for missing level sensitive interrupts in the PIC model here somewhere which in my series fixed the hang with sound and USB but it's still a problem with this series. After brief testing my guests still boot with this, I could not find the guest that needed the MV64361 gpp interrupts for PCI but let's not break that if it's already implemented. Can you do another version with these changes (restore PIRQ and pegasos2 patches and add workaround for level sensitive mode) so I can ask others to do some more tests with this over the weekend? Regards, BALATON Zoltan
On Thu, 23 Feb 2023, Bernhard Beschow wrote: > Am 23. Februar 2023 21:11:23 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Thu, 23 Feb 2023, Bernhard Beschow wrote: >>> The real VIA south bridges implement a PCI IRQ router which is configured >>> by the BIOS or the OS. In order to respect these configurations, QEMU >>> needs to implement it as well. >>> >>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>> index 3f9bd0c04d..f24e387d63 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >>> qemu_set_irq(s->cpu_intr, level); >>> } >>> >>> +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) >>> +{ >>> + switch (irq_num) { >>> + case 0: >>> + return s->dev.config[0x55] >> 4; >>> + >>> + case 1: >>> + return s->dev.config[0x56] & 0xf; >>> + >>> + case 2: >>> + return s->dev.config[0x56] >> 4; >>> + >>> + case 3: >>> + return s->dev.config[0x57] >> 4; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) >>> +{ >>> + ViaISAState *s = opaque; >>> + PCIBus *bus = pci_get_bus(&s->dev); >>> + int pic_irq; >>> + >>> + /* now we change the pic irq level according to the via irq mappings */ >>> + /* XXX: optimize */ >>> + pic_irq = via_isa_get_pci_irq(s, irq_num); >>> + if (pic_irq < ISA_NUM_IRQS) { >>> + int i, pic_level; >>> + >>> + /* The pic level is the logical OR of all the PCI irqs mapped to it. */ >>> + pic_level = 0; >>> + for (i = 0; i < PCI_NUM_PINS; i++) { >>> + if (pic_irq == via_isa_get_pci_irq(s, i)) { >>> + pic_level |= pci_bus_get_irq_level(bus, i); >>> + } >>> + } >>> + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); >>> + } >>> +} >>> + >>> static void via_isa_realize(PCIDevice *d, Error **errp) >>> { >>> ViaISAState *s = VIA_ISA(d); >>> @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >>> if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { >>> return; >>> } >>> + >>> + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); >> >> Please no oversimplification. This replaces the connections to mv64361 gpp pins made in mv64361_realize() and breaks the interrupts that can be enabled in mv64361. > > Let's split our work as follows: You'll do the audio and pegasos2 > changes including exporting the pirq properties, I'll do the first three > routing patches of this series as the base. I'm OK with doing audio as said and already did the PIRQ and pegaosos2 changes (patch 2 and 3 in my series), just take those without deleting half of them. So drop the last two via-ac97 patches and do the IRQ fixes in your way but keep working what now works. >> I've implemented that for something but can't remember now which guest exactly, > > Could you please put this information into the commit message or into > the code? That would ease maintainance a lot. I did, see patch 3 in my series. >> but this would be needed so please restore my pegasos2 patch and move >> this there connecting both mv64361 and via-isa to PCI interrupts as >> shown in the schematics. That means you also need the PIRQ pins here. >> Can you do a new version with that? > > As proposed above I'd fold the first three patches into a separate > series which you can build upon. I have no way to test the pegasos2 IRQ > changes since the test cases I'm aware of either work or we agreed that > they can be fixed later (-> USB). I did fix the USB just haven't sent a v2 yet due to this thread but it's just the change I've sent yesterday, just add this before qemu_set_irq at the end of via_isa_set_irq() in my series. This is what I have now: + uint16_t old_state; + if (old_state && s->isa_irq_state[isa_irq]) { + /* FIXME: i8259 model does not support level sensitive mode */ + qemu_set_irq(s->isa_irqs[isa_irq], 0); + } How to do that with your version I have no idea but this fixed the problem with my series. I can send a v2 of this patch with this change if it's not clear from this and the other message what I did. >> I'll try this one in the meantime > > Sounds good to me -- that's what I wanted to achieve ;) Thanks! I've answered about that in the other message, I've tried with AmigaOS, Debian Linux 8.11.0 netboot iso and MorphOS and they still boot but couldn't test them much yet. MorphOS works on my series with sound and USB and does not hang with the above workaround but found now it still hangs if I send something to it over serial (e.g. press space or enter where you've typed boot cd boot.img after it starts playing sound). This happens on both of our series but only with the via-ac97 patch so probably related to that. This could easily be a guest bug too so I don't care that much, the pegasos2 changes are more interesting to get AmigaOS run well so that's my main focus now, MorphOS already runs on other QEMU machines well. I'll still try to find this out but AmigaOS can use other sound card so as long as the IRQ problems are fixed it would work but we need more than one PCI cards working as we'd need sound card and network card for it to be usable. This was tested to work with my series, if you give alternative series I can ask to have those tested too. Regards, BALATON Zoltan
On Fri, 24 Feb 2023, BALATON Zoltan wrote: > On Thu, 23 Feb 2023, Bernhard Beschow wrote: >> Am 23. Februar 2023 21:11:23 UTC schrieb BALATON Zoltan >> <balaton@eik.bme.hu>: >>> On Thu, 23 Feb 2023, Bernhard Beschow wrote: >>>> The real VIA south bridges implement a PCI IRQ router which is configured >>>> by the BIOS or the OS. In order to respect these configurations, QEMU >>>> needs to implement it as well. >>>> >>>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 44 insertions(+) >>>> >>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>> index 3f9bd0c04d..f24e387d63 100644 >>>> --- a/hw/isa/vt82c686.c >>>> +++ b/hw/isa/vt82c686.c >>>> @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, >>>> int irq, int level) >>>> qemu_set_irq(s->cpu_intr, level); >>>> } >>>> >>>> +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) >>>> +{ >>>> + switch (irq_num) { >>>> + case 0: >>>> + return s->dev.config[0x55] >> 4; >>>> + >>>> + case 1: >>>> + return s->dev.config[0x56] & 0xf; >>>> + >>>> + case 2: >>>> + return s->dev.config[0x56] >> 4; >>>> + >>>> + case 3: >>>> + return s->dev.config[0x57] >> 4; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) >>>> +{ >>>> + ViaISAState *s = opaque; >>>> + PCIBus *bus = pci_get_bus(&s->dev); >>>> + int pic_irq; >>>> + >>>> + /* now we change the pic irq level according to the via irq mappings >>>> */ >>>> + /* XXX: optimize */ >>>> + pic_irq = via_isa_get_pci_irq(s, irq_num); >>>> + if (pic_irq < ISA_NUM_IRQS) { >>>> + int i, pic_level; >>>> + >>>> + /* The pic level is the logical OR of all the PCI irqs mapped to >>>> it. */ >>>> + pic_level = 0; >>>> + for (i = 0; i < PCI_NUM_PINS; i++) { >>>> + if (pic_irq == via_isa_get_pci_irq(s, i)) { >>>> + pic_level |= pci_bus_get_irq_level(bus, i); >>>> + } >>>> + } >>>> + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); >>>> + } >>>> +} >>>> + >>>> static void via_isa_realize(PCIDevice *d, Error **errp) >>>> { >>>> ViaISAState *s = VIA_ISA(d); >>>> @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error >>>> **errp) >>>> if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { >>>> return; >>>> } >>>> + >>>> + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); >>> >>> Please no oversimplification. This replaces the connections to mv64361 gpp >>> pins made in mv64361_realize() and breaks the interrupts that can be >>> enabled in mv64361. >> >> Let's split our work as follows: You'll do the audio and pegasos2 changes >> including exporting the pirq properties, I'll do the first three routing >> patches of this series as the base. > > I'm OK with doing audio as said and already did the PIRQ and pegaosos2 > changes (patch 2 and 3 in my series), just take those without deleting half > of them. So drop the last two via-ac97 patches and do the IRQ fixes in your > way but keep working what now works. > >>> I've implemented that for something but can't remember now which guest >>> exactly, >> >> Could you please put this information into the commit message or into the >> code? That would ease maintainance a lot. > > I did, see patch 3 in my series. > >>> but this would be needed so please restore my pegasos2 patch and move this >>> there connecting both mv64361 and via-isa to PCI interrupts as shown in >>> the schematics. That means you also need the PIRQ pins here. Can you do a >>> new version with that? >> >> As proposed above I'd fold the first three patches into a separate series >> which you can build upon. I have no way to test the pegasos2 IRQ changes >> since the test cases I'm aware of either work or we agreed that they can be >> fixed later (-> USB). > > I did fix the USB just haven't sent a v2 yet due to this thread but it's just > the change I've sent yesterday, just add this before qemu_set_irq at the end > of via_isa_set_irq() in my series. This is what I have now: > > + uint16_t old_state; > + if (old_state && s->isa_irq_state[isa_irq]) { > + /* FIXME: i8259 model does not support level sensitive mode */ > + qemu_set_irq(s->isa_irqs[isa_irq], 0); > + } Actually that should be: + old_state = s->isa_irq_state[isa_irq]; + if (level) { + s->isa_irq_state[isa_irq] |= BIT(n); + } else { + s->isa_irq_state[isa_irq] &= ~BIT(n); + } + if (old_state && s->isa_irq_state[isa_irq]) { + /* FIXME: i8259 model does not support level sensitive mode */ + qemu_set_irq(s->isa_irqs[isa_irq], 0); + } + qemu_set_irq(s->isa_irqs[isa_irq], !!s->isa_irq_state[isa_irq]); with the old_state variable definition at the top of the func of course. > How to do that with your version I have no idea but this fixed the problem > with my series. I can send a v2 of this patch with this change if it's not > clear from this and the other message what I did. > >>> I'll try this one in the meantime >> >> Sounds good to me -- that's what I wanted to achieve ;) Thanks! > > I've answered about that in the other message, I've tried with AmigaOS, > Debian Linux 8.11.0 netboot iso and MorphOS and they still boot but couldn't > test them much yet. MorphOS works on my series with sound and USB and does > not hang with the above workaround but found now it still hangs if I send > something to it over serial (e.g. press space or enter where you've typed > boot cd boot.img after it starts playing sound). This happens on both of our > series but only with the via-ac97 patch so probably related to that. This > could easily be a guest bug too so I don't care that much, the pegasos2 > changes are more interesting to get AmigaOS run well so that's my main focus > now, MorphOS already runs on other QEMU machines well. I'll still try to find > this out but AmigaOS can use other sound card so as long as the IRQ problems > are fixed it would work but we need more than one PCI cards working as we'd > need sound card and network card for it to be usable. This was tested to work > with my series, if you give alternative series I can ask to have those tested > too. > > Regards, > BALATON Zoltan > >
Am 23. Februar 2023 23:47:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Thu, 23 Feb 2023, Bernhard Beschow wrote: >> Am 23. Februar 2023 21:11:23 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >>> On Thu, 23 Feb 2023, Bernhard Beschow wrote: >>>> The real VIA south bridges implement a PCI IRQ router which is configured >>>> by the BIOS or the OS. In order to respect these configurations, QEMU >>>> needs to implement it as well. >>>> >>>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 44 insertions(+) >>>> >>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>> index 3f9bd0c04d..f24e387d63 100644 >>>> --- a/hw/isa/vt82c686.c >>>> +++ b/hw/isa/vt82c686.c >>>> @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >>>> qemu_set_irq(s->cpu_intr, level); >>>> } >>>> >>>> +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) >>>> +{ >>>> + switch (irq_num) { >>>> + case 0: >>>> + return s->dev.config[0x55] >> 4; >>>> + >>>> + case 1: >>>> + return s->dev.config[0x56] & 0xf; >>>> + >>>> + case 2: >>>> + return s->dev.config[0x56] >> 4; >>>> + >>>> + case 3: >>>> + return s->dev.config[0x57] >> 4; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) >>>> +{ >>>> + ViaISAState *s = opaque; >>>> + PCIBus *bus = pci_get_bus(&s->dev); >>>> + int pic_irq; >>>> + >>>> + /* now we change the pic irq level according to the via irq mappings */ >>>> + /* XXX: optimize */ >>>> + pic_irq = via_isa_get_pci_irq(s, irq_num); >>>> + if (pic_irq < ISA_NUM_IRQS) { >>>> + int i, pic_level; >>>> + >>>> + /* The pic level is the logical OR of all the PCI irqs mapped to it. */ >>>> + pic_level = 0; >>>> + for (i = 0; i < PCI_NUM_PINS; i++) { >>>> + if (pic_irq == via_isa_get_pci_irq(s, i)) { >>>> + pic_level |= pci_bus_get_irq_level(bus, i); >>>> + } >>>> + } >>>> + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); >>>> + } >>>> +} >>>> + >>>> static void via_isa_realize(PCIDevice *d, Error **errp) >>>> { >>>> ViaISAState *s = VIA_ISA(d); >>>> @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >>>> if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { >>>> return; >>>> } >>>> + >>>> + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); >>> >>> Please no oversimplification. This replaces the connections to mv64361 gpp pins made in mv64361_realize() and breaks the interrupts that can be enabled in mv64361. >> >> Let's split our work as follows: You'll do the audio and pegasos2 changes including exporting the pirq properties, I'll do the first three routing patches of this series as the base. > >I'm OK with doing audio as said and already did the PIRQ and pegaosos2 changes (patch 2 and 3 in my series), just take those without deleting half of them. I can only take the three VT82xx patches as I proposed since I don't know the Pegasos2 board as well as you do and I don't want to iterate on any review comments for the other patches. I'll send my series soonish. Best regards, Bernhard >So drop the last two via-ac97 patches and do the IRQ fixes in your way but keep working what now works. > >>> I've implemented that for something but can't remember now which guest exactly, >> >> Could you please put this information into the commit message or into the code? That would ease maintainance a lot. > >I did, see patch 3 in my series. > >>> but this would be needed so please restore my pegasos2 patch and move this there connecting both mv64361 and via-isa to PCI interrupts as shown in the schematics. That means you also need the PIRQ pins here. Can you do a new version with that? >> >> As proposed above I'd fold the first three patches into a separate series which you can build upon. I have no way to test the pegasos2 IRQ changes since the test cases I'm aware of either work or we agreed that they can be fixed later (-> USB). > >I did fix the USB just haven't sent a v2 yet due to this thread but it's just the change I've sent yesterday, just add this before qemu_set_irq at the end of via_isa_set_irq() in my series. This is what I have now: > >+ uint16_t old_state; >+ if (old_state && s->isa_irq_state[isa_irq]) { >+ /* FIXME: i8259 model does not support level sensitive mode */ >+ qemu_set_irq(s->isa_irqs[isa_irq], 0); >+ } > >How to do that with your version I have no idea but this fixed the problem with my series. I can send a v2 of this patch with this change if it's not clear from this and the other message what I did. > >>> I'll try this one in the meantime >> >> Sounds good to me -- that's what I wanted to achieve ;) Thanks! > >I've answered about that in the other message, I've tried with AmigaOS, Debian Linux 8.11.0 netboot iso and MorphOS and they still boot but couldn't test them much yet. MorphOS works on my series with sound and USB and does not hang with the above workaround but found now it still hangs if I send something to it over serial (e.g. press space or enter where you've typed boot cd boot.img after it starts playing sound). This happens on both of our series but only with the via-ac97 patch so probably related to that. This could easily be a guest bug too so I don't care that much, the pegasos2 changes are more interesting to get AmigaOS run well so that's my main focus now, MorphOS already runs on other QEMU machines well. I'll still try to find this out but AmigaOS can use other sound card so as long as the IRQ problems are fixed it would work but we need more than one PCI cards working as we'd need sound card and network card for it to be usable. This was tested to work with my series, if you give alternative series I can ask to have those tested too. > >Regards, >BALATON Zoltan
On Fri, 24 Feb 2023, Bernhard Beschow wrote: > I can only take the three VT82xx patches as I proposed since I don't > know the Pegasos2 board as well as you do and I don't want to iterate on > any review comments for the other patches. I'll send my series soonish. Does soonish means still today? Sorry for being impatient but I'd like to finalise this by Monday so to be able to rebase my changes and make it avaialbe for testing as soon as possible, still in the weekend as people won't have time on weekdays, then we should need the final version of your alternative patches about now. Regards, BALATON Zoltan
Am 25. Februar 2023 13:12:05 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Fri, 24 Feb 2023, Bernhard Beschow wrote: >> I can only take the three VT82xx patches as I proposed since I don't know the Pegasos2 board as well as you do and I don't want to iterate on any review comments for the other patches. I'll send my series soonish. > >Does soonish means still today? Sorry for being impatient but I'd like to finalise this by Monday so to be able to rebase my changes and make it avaialbe for testing as soon as possible, still in the weekend as people won't have time on weekdays, then we should need the final version of your alternative patches about now. Sure, respin is out. It's so fresh I can't even give you a link yet ;) Best regards, Bernhard > >Regards, >BALATON Zoltan
On 23/02/2023 20:20, Bernhard Beschow wrote: > The real VIA south bridges implement a PCI IRQ router which is configured > by the BIOS or the OS. In order to respect these configurations, QEMU > needs to implement it as well. > > Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index 3f9bd0c04d..f24e387d63 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level) > qemu_set_irq(s->cpu_intr, level); > } > > +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) > +{ > + switch (irq_num) { > + case 0: > + return s->dev.config[0x55] >> 4; > + > + case 1: > + return s->dev.config[0x56] & 0xf; > + > + case 2: > + return s->dev.config[0x56] >> 4; > + > + case 3: > + return s->dev.config[0x57] >> 4; > + } > + > + return 0; > +} > + > +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) > +{ > + ViaISAState *s = opaque; > + PCIBus *bus = pci_get_bus(&s->dev); > + int pic_irq; > + > + /* now we change the pic irq level according to the via irq mappings */ > + /* XXX: optimize */ > + pic_irq = via_isa_get_pci_irq(s, irq_num); > + if (pic_irq < ISA_NUM_IRQS) { > + int i, pic_level; > + > + /* The pic level is the logical OR of all the PCI irqs mapped to it. */ > + pic_level = 0; > + for (i = 0; i < PCI_NUM_PINS; i++) { > + if (pic_irq == via_isa_get_pci_irq(s, i)) { > + pic_level |= pci_bus_get_irq_level(bus, i); > + } > + } > + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); > + } > +} > + > static void via_isa_realize(PCIDevice *d, Error **errp) > { > ViaISAState *s = VIA_ISA(d); > @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) > if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { > return; > } > + > + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); > } > > /* TYPE_VT82C686B_ISA */ This looks right, however generally a PCI device shouldn't really be setting PCI bus IRQs: this is normally done by the PCI host bridge. Is it just the case that the x86 world is different here for legacy reasons? ATB, Mark.
Am 1. März 2023 14:20:54 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >On 23/02/2023 20:20, Bernhard Beschow wrote: > >> The real VIA south bridges implement a PCI IRQ router which is configured >> by the BIOS or the OS. In order to respect these configurations, QEMU >> needs to implement it as well. >> >> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 3f9bd0c04d..f24e387d63 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level) >> qemu_set_irq(s->cpu_intr, level); >> } >> +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) >> +{ >> + switch (irq_num) { >> + case 0: >> + return s->dev.config[0x55] >> 4; >> + >> + case 1: >> + return s->dev.config[0x56] & 0xf; >> + >> + case 2: >> + return s->dev.config[0x56] >> 4; >> + >> + case 3: >> + return s->dev.config[0x57] >> 4; >> + } >> + >> + return 0; >> +} >> + >> +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) >> +{ >> + ViaISAState *s = opaque; >> + PCIBus *bus = pci_get_bus(&s->dev); >> + int pic_irq; >> + >> + /* now we change the pic irq level according to the via irq mappings */ >> + /* XXX: optimize */ >> + pic_irq = via_isa_get_pci_irq(s, irq_num); >> + if (pic_irq < ISA_NUM_IRQS) { >> + int i, pic_level; >> + >> + /* The pic level is the logical OR of all the PCI irqs mapped to it. */ >> + pic_level = 0; >> + for (i = 0; i < PCI_NUM_PINS; i++) { >> + if (pic_irq == via_isa_get_pci_irq(s, i)) { >> + pic_level |= pci_bus_get_irq_level(bus, i); >> + } >> + } >> + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); >> + } >> +} >> + >> static void via_isa_realize(PCIDevice *d, Error **errp) >> { >> ViaISAState *s = VIA_ISA(d); >> @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { >> return; >> } >> + >> + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); >> } >> /* TYPE_VT82C686B_ISA */ > >This looks right, however generally a PCI device shouldn't really be setting PCI bus IRQs: this is normally done by the PCI host bridge. Is it just the case that the x86 world is different here for legacy reasons? Well, looking at the pegasos2 schematics it seems to me that at least the intA + intB lines are connected to both chips (and that they are even shared between the AGP slot and the PCI bus). On the Marvell north bridge they seem to be connected to GPIO pins and on the VIA chip to the PCI IRQ router. Note that GPIO pins can usually be deactivated (tristated) and that the four VIA PCI IRQs can be deactivated by assigning "interrupt 0". Such a design would allow the lines to be hardwired to both "interrupt controllers", allowing system software to use either controller, or possibly even mixed (e.g. intA to be treated by the north bridge and intB by VIA). Such designs are currently not easily implementable in QEMU since only one IRQ handler can be assigned to a PCI bus. As a workaround, one could assign a custom IRQ handler which implements special handling. Getting back to your question, I think you are right that assigning the IRQ handler in the VIA model may break e.g. the Fuloong2e machine where the IRQ handler is set in the north bridge. Since the VIA chip is instantiated later it now effectively replaces the handler. It would be really neat if QEMU allowed for assigning two or more IRQ handlers to a PCI bus... Do you think that two interrupt controllers connected to IRQ lines like that sounds reasonable? Best regards, Bernhard > > >ATB, > >Mark.
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 3f9bd0c04d..f24e387d63 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -604,6 +604,48 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level) qemu_set_irq(s->cpu_intr, level); } +static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num) +{ + switch (irq_num) { + case 0: + return s->dev.config[0x55] >> 4; + + case 1: + return s->dev.config[0x56] & 0xf; + + case 2: + return s->dev.config[0x56] >> 4; + + case 3: + return s->dev.config[0x57] >> 4; + } + + return 0; +} + +static void via_isa_set_pci_irq(void *opaque, int irq_num, int level) +{ + ViaISAState *s = opaque; + PCIBus *bus = pci_get_bus(&s->dev); + int pic_irq; + + /* now we change the pic irq level according to the via irq mappings */ + /* XXX: optimize */ + pic_irq = via_isa_get_pci_irq(s, irq_num); + if (pic_irq < ISA_NUM_IRQS) { + int i, pic_level; + + /* The pic level is the logical OR of all the PCI irqs mapped to it. */ + pic_level = 0; + for (i = 0; i < PCI_NUM_PINS; i++) { + if (pic_irq == via_isa_get_pci_irq(s, i)) { + pic_level |= pci_bus_get_irq_level(bus, i); + } + } + qemu_set_irq(s->isa_irqs[pic_irq], pic_level); + } +} + static void via_isa_realize(PCIDevice *d, Error **errp) { ViaISAState *s = VIA_ISA(d); @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp) if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) { return; } + + pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS); } /* TYPE_VT82C686B_ISA */
The real VIA south bridges implement a PCI IRQ router which is configured by the BIOS or the OS. In order to respect these configurations, QEMU needs to implement it as well. Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)