| Message ID | 0fd9eac9174a840054c511fbc015048929c7bc40.1677445307.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Pegasos2 fixes and audio output support | expand |
On 25/2/23 19:11, BALATON Zoltan wrote: > From: Bernhard Beschow <shentey@gmail.com> > > 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> > [balaton: declare gpio inputs instead of changing pci bus irqs so it can > be connected in board code; remove some empty lines] > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > Tested-by: Rene Engel <ReneEngel80@emailn.de> > --- > hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > +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) { the ISA IRQ is stored in 4-bit so will always be in range. > + 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); > + } > +}
On Sun, 26 Feb 2023, Philippe Mathieu-Daudé wrote: > On 25/2/23 19:11, BALATON Zoltan wrote: >> From: Bernhard Beschow <shentey@gmail.com> >> >> 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> >> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >> be connected in board code; remove some empty lines] >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> Tested-by: Rene Engel <ReneEngel80@emailn.de> >> --- >> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) > >> +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) { > > the ISA IRQ is stored in 4-bit so will always be in range. Complain at hw/isa/piix4 I guess or clean this up together later :-) Or maybe Bernhard has an idea or patch for this coming up that's why he pushed this in here. In any case, since this is now the same as piix4 either both or nothing for this and both would be out of scope for this series. Regards, BALATON Zoltan >> + 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); >> + } >> +} > > > >
Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >From: Bernhard Beschow <shentey@gmail.com> > >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> >[balaton: declare gpio inputs instead of changing pci bus irqs so it can > be connected in board code; remove some empty lines] >Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >Tested-by: Rene Engel <ReneEngel80@emailn.de> >--- > hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > >diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >index 3f9bd0c04d..4025f9bcdc 100644 >--- a/hw/isa/vt82c686.c >+++ b/hw/isa/vt82c686.c >@@ -604,6 +604,44 @@ 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); >@@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) > int i; > > qdev_init_gpio_out(dev, &s->cpu_intr, 1); >+ qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS); This line is a Pegasos2 specific addition for fixing its IRQ handling. Since this code must also work with the Fuloong2e board we should aim for a minimal changeset here which renders this line out of scope. Let's keep the two series separate since now I need to watch two series for comments. Please use Based-on: tag next time instead. Thanks, Bernhard > isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); > isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), > errp);
Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 25/2/23 19:11, BALATON Zoltan wrote: >> From: Bernhard Beschow <shentey@gmail.com> >> >> 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> >> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >> be connected in board code; remove some empty lines] >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> Tested-by: Rene Engel <ReneEngel80@emailn.de> >> --- >> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) > >> +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) { > >the ISA IRQ is stored in 4-bit so will always be in range. Indeed. I'd turn this into an assert to keep this assum visible. I'll do another iteration of the PCI IRQ router series. Best regards, Bernhard > >> + 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); >> + } >> +} > >
On Sun, 26 Feb 2023, Bernhard Beschow wrote: > Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> From: Bernhard Beschow <shentey@gmail.com> >> >> 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> >> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >> be connected in board code; remove some empty lines] >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> Tested-by: Rene Engel <ReneEngel80@emailn.de> >> --- >> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index 3f9bd0c04d..4025f9bcdc 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -604,6 +604,44 @@ 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); >> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >> int i; >> >> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS); > > This line is a Pegasos2 specific addition for fixing its IRQ handling. Since this code must also work with the Fuloong2e board we should aim for a minimal changeset here which renders this line out of scope. > > Let's keep the two series separate since now I need to watch two series for comments. Please use Based-on: tag next time instead. Well, it's not. It's part of the QDev model for VT8231 that allows it to be connected by boards so I think this belongs here otherwise this won't even compile because the function you've added would be unused and bail on -Werror. Let's not make this more difficult than it is. I'm OK with reasonable changes but what's your goal now? You can't get rid of this line as it's how QDev can model it. Either I have to call into this model or have to export these pins as gpios. Regards, BALATON Zoltan > Thanks, > Bernhard > >> isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >> isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), >> errp); > >
On Sun, 26 Feb 2023, Bernhard Beschow wrote: > Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >> On 25/2/23 19:11, BALATON Zoltan wrote: >>> From: Bernhard Beschow <shentey@gmail.com> >>> >>> 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> >>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>> be connected in board code; remove some empty lines] >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>> --- >>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >> >>> +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) { >> >> the ISA IRQ is stored in 4-bit so will always be in range. > > Indeed. I'd turn this into an assert to keep this assum visible. I'll do another iteration of the PCI IRQ router series. If you do that please don't break it and make me redo this series again. Sumbit a patch as a reply to this that can be substituted without changing other patches and I can include in later respins. We don't have time to make extensive changes now, the freeze is too close. Regards, BALATON Zoltan > Best regards, > Bernhard >> >>> + 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); >>> + } >>> +} >> >> > >
On Sun, 26 Feb 2023, Bernhard Beschow wrote: > Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >> On 25/2/23 19:11, BALATON Zoltan wrote: >>> From: Bernhard Beschow <shentey@gmail.com> >>> >>> 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> >>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>> be connected in board code; remove some empty lines] >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>> --- >>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >> >>> +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) { >> >> the ISA IRQ is stored in 4-bit so will always be in range. > > Indeed. I'd turn this into an assert to keep this assum visible. I'll do another iteration of the PCI IRQ router series. I don't like a useless assert in an irq handler that's potentially called a lot. If you want to keep that add a comment instead. Also I can't use Based-on because having all patches in a single series helps maintainers to follow what belongs to here so this should be one series. You don't have to follow your one any more as it was fully incorporated here so this is the only version you'd have to watch. Regards, BALATON Zoltan > Best regards, > Bernhard >> >>> + 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); >>> + } >>> +} >> >> > >
Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sun, 26 Feb 2023, Bernhard Beschow wrote: >> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >>> From: Bernhard Beschow <shentey@gmail.com> >>> >>> 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> >>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>> be connected in board code; remove some empty lines] >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>> --- >>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>> index 3f9bd0c04d..4025f9bcdc 100644 >>> --- a/hw/isa/vt82c686.c >>> +++ b/hw/isa/vt82c686.c >>> @@ -604,6 +604,44 @@ 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); >>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >>> int i; >>> >>> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS); >> >> This line is a Pegasos2 specific addition for fixing its IRQ handling. Since this code must also work with the Fuloong2e board we should aim for a minimal changeset here which renders this line out of scope. >> >> Let's keep the two series separate since now I need to watch two series for comments. Please use Based-on: tag next time instead. > >Well, it's not. It's part of the QDev model for VT8231 that allows it to be connected by boards so I think this belongs here otherwise this won't even compile because the function you've added would be unused and bail on -Werror. Let's not make this more difficult than it is. I'm OK with reasonable changes but what's your goal now? You can't get rid of this line as it's how QDev can model it. Either I have to call into this model or have to export these pins as gpios. Exporting the pins is a separate aspect on top of implementing PCI IRQ routing. To make this clear and obvious this should be a dedicated patch. In case either patch has an issue this would also ease and therefore acellerate discussions. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> Thanks, >> Bernhard >> >>> isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); >>> isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), >>> errp); >> >>
Am 26. Februar 2023 23:37:24 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sun, 26 Feb 2023, Bernhard Beschow wrote: >> Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>> On 25/2/23 19:11, BALATON Zoltan wrote: >>>> From: Bernhard Beschow <shentey@gmail.com> >>>> >>>> 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> >>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>>> be connected in board code; remove some empty lines] >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>>> --- >>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>> >>>> +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) { >>> >>> the ISA IRQ is stored in 4-bit so will always be in range. >> >> Indeed. I'd turn this into an assert to keep this assum visible. I'll do another iteration of the PCI IRQ router series. > >If you do that please don't break it and make me redo this series again. Sumbit a patch as a reply to this that can be substituted without changing other patches and I can include in later respins. I will only do the mist minimal changes satisfying the review comments. This should minimize the risk of breakage. Also, you can minimize the chance of breakage on your side by not introducing more changes than needed, e.g. by not doing any formatting changes. >We don't have time to make extensive changes now, the freeze is too close. I don't intend to make extensive changes unless review comments requre it. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> Best regards, >> Bernhard >>> >>>> + 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); >>>> + } >>>> +} >>> >>> >> >>
Am 26. Februar 2023 23:58:32 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sun, 26 Feb 2023, Bernhard Beschow wrote: >> Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>> On 25/2/23 19:11, BALATON Zoltan wrote: >>>> From: Bernhard Beschow <shentey@gmail.com> >>>> >>>> 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> >>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>>> be connected in board code; remove some empty lines] >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>>> --- >>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>> >>>> +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) { >>> >>> the ISA IRQ is stored in 4-bit so will always be in range. >> >> Indeed. I'd turn this into an assert to keep this assum visible. I'll do another iteration of the PCI IRQ router series. > >I don't like a useless assert in an irq handler that's potentially called a lot. If you want to keep that add a comment instead. It won't because it will only affect debug builds. If it fails it's immediately clear in which direction to look for the culprit. > >Also I can't use Based-on because having all patches in a single series helps maintainers to follow what belongs to here so this should be one series. You don't have to follow your one any more as it was fully incorporated here so this is the only version you'd have to watch. > >Regards, >BALATON Zoltan > >> Best regards, >> Bernhard >>> >>>> + 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); >>>> + } >>>> +} >>> >>> >> >>
On Mon, 27 Feb 2023, Bernhard Beschow wrote: > Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sun, 26 Feb 2023, Bernhard Beschow wrote: >>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >>>> From: Bernhard Beschow <shentey@gmail.com> >>>> >>>> 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> >>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>>> be connected in board code; remove some empty lines] >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>>> --- >>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>> index 3f9bd0c04d..4025f9bcdc 100644 >>>> --- a/hw/isa/vt82c686.c >>>> +++ b/hw/isa/vt82c686.c >>>> @@ -604,6 +604,44 @@ 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); >>>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) >>>> int i; >>>> >>>> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>>> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS); >>> >>> This line is a Pegasos2 specific addition for fixing its IRQ handling. Since this code must also work with the Fuloong2e board we should aim for a minimal changeset here which renders this line out of scope. >>> >>> Let's keep the two series separate since now I need to watch two series for comments. Please use Based-on: tag next time instead. >> >> Well, it's not. It's part of the QDev model for VT8231 that allows it to be connected by boards so I think this belongs here otherwise this won't even compile because the function you've added would be unused and bail on -Werror. Let's not make this more difficult than it is. I'm OK with reasonable changes but what's your goal now? You can't get rid of this line as it's how QDev can model it. Either I have to call into this model or have to export these pins as gpios. > > Exporting the pins is a separate aspect on top of implementing PCI IRQ > routing. To make this clear and obvious this should be a dedicated > patch. In case either patch has an issue this would also ease and > therefore acellerate discussions. How would you do that? Introduce via_isa_set_pci_irq() as unused in this patch then have a one line patch to add connecting it to gpio pins? That just makes no sense. This is not modeling the chip with QDev and then the boatd can connect it as appropriate modeling the board that the chip is in. So if fuloon2e needs to do that then it should. I'll check that as I was focusing on pegasos2 for now. You can't use pci_bur_irqs here because that breaks pegasos2 so no point in doing that way. Regards, BALATON Zoltan
On Mon, 27 Feb 2023, Bernhard Beschow wrote: > Am 26. Februar 2023 23:37:24 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sun, 26 Feb 2023, Bernhard Beschow wrote: >>> Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>> On 25/2/23 19:11, BALATON Zoltan wrote: >>>>> From: Bernhard Beschow <shentey@gmail.com> >>>>> >>>>> 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> >>>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>>>> be connected in board code; remove some empty lines] >>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>>>> --- >>>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>> >>>>> +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) { >>>> >>>> the ISA IRQ is stored in 4-bit so will always be in range. >>> >>> Indeed. I'd turn this into an assert to keep this assum visible. I'll do another iteration of the PCI IRQ router series. >> >> If you do that please don't break it and make me redo this series again. Sumbit a patch as a reply to this that can be substituted without changing other patches and I can include in later respins. > > I will only do the mist minimal changes satisfying the review comments. This should minimize the risk of breakage. Hopefully this gets reviewed soon, we only have this week to finalise it so I hope we get comments early and not at the weekend or next week. I can make changes but I need to know in time for that. > Also, you can minimize the chance of breakage on your side by not > introducing more changes than needed, e.g. by not doing any formatting > changes. Deleting empty lines is hardly a breaking change. Also your formatting looked like it's missing break statements and did not conveniently fit in my screen so I took the opportunity to make it clearer at least for me. Personal preferences may differ but this should not be a big deal. Regards, BALATON Zoltan >> We don't have time to make extensive changes now, the freeze is too close. > > I don't intend to make extensive changes unless review comments requre it. > > Best regards, > Bernhard > >> >> Regards, >> BALATON Zoltan >> >>> Best regards, >>> Bernhard >>>> >>>>> + 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); >>>>> + } >>>>> +} >>>> >>>> >>> >>> > >
On Mon, 27 Feb 2023, Bernhard Beschow wrote: > Am 26. Februar 2023 23:58:32 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sun, 26 Feb 2023, Bernhard Beschow wrote: >>> Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>> On 25/2/23 19:11, BALATON Zoltan wrote: >>>>> From: Bernhard Beschow <shentey@gmail.com> >>>>> >>>>> 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> >>>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>>>> be connected in board code; remove some empty lines] >>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>>>> --- >>>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>> >>>>> +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) { >>>> >>>> the ISA IRQ is stored in 4-bit so will always be in range. >>> >>> Indeed. I'd turn this into an assert to keep this assum visible. I'll do another iteration of the PCI IRQ router series. >> >> I don't like a useless assert in an irq handler that's potentially called a lot. If you want to keep that add a comment instead. > > It won't because it will only affect debug builds. If it fails it's > immediately clear in which direction to look for the culprit. You may think so but it's not. QEMU cannot be built with NDEBUG (there was even some discussion about this recently on the list) so all the asserts are there in release builds. Let's at least keep them out from frequently called parts especially if they can never fire. Regards, BALATON Zoltan
On Mon, 27 Feb 2023, BALATON Zoltan wrote: > On Mon, 27 Feb 2023, Bernhard Beschow wrote: >> Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan >> <balaton@eik.bme.hu>: >>> On Sun, 26 Feb 2023, Bernhard Beschow wrote: >>>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan >>>> <balaton@eik.bme.hu>: >>>>> From: Bernhard Beschow <shentey@gmail.com> >>>>> >>>>> 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> >>>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it can >>>>> be connected in board code; remove some empty lines] >>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>>>> --- >>>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>>> >>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>>> index 3f9bd0c04d..4025f9bcdc 100644 >>>>> --- a/hw/isa/vt82c686.c >>>>> +++ b/hw/isa/vt82c686.c >>>>> @@ -604,6 +604,44 @@ 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); >>>>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error >>>>> **errp) >>>>> int i; >>>>> >>>>> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>>>> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", >>>>> PCI_NUM_PINS); >>>> >>>> This line is a Pegasos2 specific addition for fixing its IRQ handling. >>>> Since this code must also work with the Fuloong2e board we should aim for >>>> a minimal changeset here which renders this line out of scope. >>>> >>>> Let's keep the two series separate since now I need to watch two series >>>> for comments. Please use Based-on: tag next time instead. >>> >>> Well, it's not. It's part of the QDev model for VT8231 that allows it to >>> be connected by boards so I think this belongs here otherwise this won't >>> even compile because the function you've added would be unused and bail on >>> -Werror. Let's not make this more difficult than it is. I'm OK with >>> reasonable changes but what's your goal now? You can't get rid of this >>> line as it's how QDev can model it. Either I have to call into this model >>> or have to export these pins as gpios. >> >> Exporting the pins is a separate aspect on top of implementing PCI IRQ >> routing. To make this clear and obvious this should be a dedicated patch. >> In case either patch has an issue this would also ease and therefore >> acellerate discussions. > > How would you do that? Introduce via_isa_set_pci_irq() as unused in this > patch then have a one line patch to add connecting it to gpio pins? That just > makes no sense. This is not modeling the chip with QDev and then the boatd This is now modeling... > can connect it as appropriate modeling the board that the chip is in. So if > fuloon2e needs to do that then it should. I'll check that as I was focusing fuloong2e > on pegasos2 for now. You can't use pci_bur_irqs here because that breaks > pegasos2 so no point in doing that way. > > Regards, > BALATON Zoltan > >
On Mon, 27 Feb 2023, BALATON Zoltan wrote: > On Mon, 27 Feb 2023, BALATON Zoltan wrote: >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >>> Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan >>> <balaton@eik.bme.hu>: >>>> On Sun, 26 Feb 2023, Bernhard Beschow wrote: >>>>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan >>>>> <balaton@eik.bme.hu>: >>>>>> From: Bernhard Beschow <shentey@gmail.com> >>>>>> >>>>>> 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> >>>>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it >>>>>> can >>>>>> be connected in board code; remove some empty lines] >>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> >>>>>> --- >>>>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 39 insertions(+) >>>>>> >>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >>>>>> index 3f9bd0c04d..4025f9bcdc 100644 >>>>>> --- a/hw/isa/vt82c686.c >>>>>> +++ b/hw/isa/vt82c686.c >>>>>> @@ -604,6 +604,44 @@ 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); >>>>>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error >>>>>> **errp) >>>>>> int i; >>>>>> >>>>>> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >>>>>> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", >>>>>> PCI_NUM_PINS); >>>>> >>>>> This line is a Pegasos2 specific addition for fixing its IRQ handling. >>>>> Since this code must also work with the Fuloong2e board we should aim >>>>> for a minimal changeset here which renders this line out of scope. >>>>> >>>>> Let's keep the two series separate since now I need to watch two series >>>>> for comments. Please use Based-on: tag next time instead. >>>> >>>> Well, it's not. It's part of the QDev model for VT8231 that allows it to >>>> be connected by boards so I think this belongs here otherwise this won't >>>> even compile because the function you've added would be unused and bail >>>> on -Werror. Let's not make this more difficult than it is. I'm OK with >>>> reasonable changes but what's your goal now? You can't get rid of this >>>> line as it's how QDev can model it. Either I have to call into this model >>>> or have to export these pins as gpios. >>> >>> Exporting the pins is a separate aspect on top of implementing PCI IRQ >>> routing. To make this clear and obvious this should be a dedicated patch. >>> In case either patch has an issue this would also ease and therefore >>> acellerate discussions. >> >> How would you do that? Introduce via_isa_set_pci_irq() as unused in this >> patch then have a one line patch to add connecting it to gpio pins? That >> just makes no sense. This is not modeling the chip with QDev and then the >> boatd > > This is now modeling... > >> can connect it as appropriate modeling the board that the chip is in. So if >> fuloon2e needs to do that then it should. I'll check that as I was focusing > > fuloong2e I've checked fuloong2e and it still works as before. PCI bus is handled by bonito on that board so your patch would actually break it. The VIA chip is a PCIDevice. You're not supposed to replace the interrupts of the bus it's connected to from this model as that should be done by the pci-host or the board. Therefore modeling the chip's PIRQ/PINT pins as gpios which is the QDev concept for that is right and your usage of pci_set_irq here is wrong. Please stop breaking my series. I had a working and tested series (still have that on my pegasos2 branch in case we end up chosing that) which was changed but you to something that is now conceptually wrong but still works because of guests don't change firmware defaults to share PCI interrupts with internal functions just to fulfill your assumption that internal functions are PCI devices (which now I have proof that they don't conform to that PCI standard doc, look at the comment in the last patch about PCI Command register in via-ac97 and compare that with the chip datasheet) but OK if this allows simlpler code in QEMU and still works I can accept that but don't push ideas that are wrong. Regards, BALATON Zoltan
On Mon, Feb 27, 2023 at 1:57 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Mon, 27 Feb 2023, BALATON Zoltan wrote: > > On Mon, 27 Feb 2023, BALATON Zoltan wrote: > >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: > >>> Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan > >>> <balaton@eik.bme.hu>: > >>>> On Sun, 26 Feb 2023, Bernhard Beschow wrote: > >>>>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan > >>>>> <balaton@eik.bme.hu>: > >>>>>> From: Bernhard Beschow <shentey@gmail.com> > >>>>>> > >>>>>> 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> > >>>>>> [balaton: declare gpio inputs instead of changing pci bus irqs so > it > >>>>>> can > >>>>>> be connected in board code; remove some empty lines] > >>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > >>>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de> > >>>>>> --- > >>>>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ > >>>>>> 1 file changed, 39 insertions(+) > >>>>>> > >>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > >>>>>> index 3f9bd0c04d..4025f9bcdc 100644 > >>>>>> --- a/hw/isa/vt82c686.c > >>>>>> +++ b/hw/isa/vt82c686.c > >>>>>> @@ -604,6 +604,44 @@ 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); > >>>>>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error > >>>>>> **errp) > >>>>>> int i; > >>>>>> > >>>>>> qdev_init_gpio_out(dev, &s->cpu_intr, 1); > >>>>>> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", > >>>>>> PCI_NUM_PINS); > >>>>> > >>>>> This line is a Pegasos2 specific addition for fixing its IRQ > handling. > >>>>> Since this code must also work with the Fuloong2e board we should > aim > >>>>> for a minimal changeset here which renders this line out of scope. > >>>>> > >>>>> Let's keep the two series separate since now I need to watch two > series > >>>>> for comments. Please use Based-on: tag next time instead. > >>>> > >>>> Well, it's not. It's part of the QDev model for VT8231 that allows it > to > >>>> be connected by boards so I think this belongs here otherwise this > won't > >>>> even compile because the function you've added would be unused and > bail > >>>> on -Werror. Let's not make this more difficult than it is. I'm OK > with > >>>> reasonable changes but what's your goal now? You can't get rid of > this > >>>> line as it's how QDev can model it. Either I have to call into this > model > >>>> or have to export these pins as gpios. > >>> > >>> Exporting the pins is a separate aspect on top of implementing PCI IRQ > >>> routing. To make this clear and obvious this should be a dedicated > patch. > >>> In case either patch has an issue this would also ease and therefore > >>> acellerate discussions. > >> > >> How would you do that? Introduce via_isa_set_pci_irq() as unused in > this > >> patch then have a one line patch to add connecting it to gpio pins? > That > >> just makes no sense. This is not modeling the chip with QDev and then > the > >> boatd > > > > This is now modeling... > > > >> can connect it as appropriate modeling the board that the chip is in. > So if > >> fuloon2e needs to do that then it should. I'll check that as I was > focusing > > > > fuloong2e > > I've checked fuloong2e and it still works as before. PCI bus is handled by > bonito on that board so your patch would actually break it. The VIA chip > is a PCIDevice. You're not supposed to replace the interrupts of the bus > it's connected to from this model as that should be done by the pci-host > or the board. Therefore modeling the chip's PIRQ/PINT pins as gpios which > is the QDev concept for that is right and your usage of pci_set_irq here > is wrong. > Works for me: (08/84) tests/avocado/boot_linux_console.py:BootLinuxConsole.test_mips64el_fuloong2e: PASS (2.77 s) > > Please stop breaking my series. I had a working and tested series (still > have that on my pegasos2 branch in case we end up chosing that) which was > changed but you to something that is now conceptually wrong but still > works because of guests don't change firmware defaults to share PCI > interrupts with internal functions just to fulfill your assumption that > internal functions are PCI devices (which now I have proof that they don't > conform to that PCI standard doc, look at the comment in the last patch > about PCI Command register in via-ac97 and compare that with the chip > datasheet) but OK if this allows simlpler code in QEMU and still works I > can accept that but don't push ideas that are wrong. > > Regards, > BALATON Zoltan >
On 27/02/2023 16:52, Bernhard Beschow wrote: > On Mon, Feb 27, 2023 at 1:57 PM BALATON Zoltan <balaton@eik.bme.hu > <mailto:balaton@eik.bme.hu>> wrote: > > On Mon, 27 Feb 2023, BALATON Zoltan wrote: > > On Mon, 27 Feb 2023, BALATON Zoltan wrote: > >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: > >>> Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan > >>> <balaton@eik.bme.hu <mailto:balaton@eik.bme.hu>>: > >>>> On Sun, 26 Feb 2023, Bernhard Beschow wrote: > >>>>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan > >>>>> <balaton@eik.bme.hu <mailto:balaton@eik.bme.hu>>: > >>>>>> From: Bernhard Beschow <shentey@gmail.com <mailto:shentey@gmail.com>> > >>>>>> > >>>>>> 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 > <mailto:shentey@gmail.com>> > >>>>>> [balaton: declare gpio inputs instead of changing pci bus irqs so it > >>>>>> can > >>>>>> be connected in board code; remove some empty lines] > >>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu > <mailto:balaton@eik.bme.hu>> > >>>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de <mailto:ReneEngel80@emailn.de>> > >>>>>> --- > >>>>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ > >>>>>> 1 file changed, 39 insertions(+) > >>>>>> > >>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > >>>>>> index 3f9bd0c04d..4025f9bcdc 100644 > >>>>>> --- a/hw/isa/vt82c686.c > >>>>>> +++ b/hw/isa/vt82c686.c > >>>>>> @@ -604,6 +604,44 @@ 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); > >>>>>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error > >>>>>> **errp) > >>>>>> int i; > >>>>>> > >>>>>> qdev_init_gpio_out(dev, &s->cpu_intr, 1); > >>>>>> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", > >>>>>> PCI_NUM_PINS); > >>>>> > >>>>> This line is a Pegasos2 specific addition for fixing its IRQ handling. > >>>>> Since this code must also work with the Fuloong2e board we should aim > >>>>> for a minimal changeset here which renders this line out of scope. > >>>>> > >>>>> Let's keep the two series separate since now I need to watch two series > >>>>> for comments. Please use Based-on: tag next time instead. > >>>> > >>>> Well, it's not. It's part of the QDev model for VT8231 that allows it to > >>>> be connected by boards so I think this belongs here otherwise this won't > >>>> even compile because the function you've added would be unused and bail > >>>> on -Werror. Let's not make this more difficult than it is. I'm OK with > >>>> reasonable changes but what's your goal now? You can't get rid of this > >>>> line as it's how QDev can model it. Either I have to call into this model > >>>> or have to export these pins as gpios. > >>> > >>> Exporting the pins is a separate aspect on top of implementing PCI IRQ > >>> routing. To make this clear and obvious this should be a dedicated patch. > >>> In case either patch has an issue this would also ease and therefore > >>> acellerate discussions. > >> > >> How would you do that? Introduce via_isa_set_pci_irq() as unused in this > >> patch then have a one line patch to add connecting it to gpio pins? That > >> just makes no sense. This is not modeling the chip with QDev and then the > >> boatd > > > > This is now modeling... > > > >> can connect it as appropriate modeling the board that the chip is in. So if > >> fuloon2e needs to do that then it should. I'll check that as I was focusing > > > > fuloong2e > > I've checked fuloong2e and it still works as before. PCI bus is handled by > bonito on that board so your patch would actually break it. The VIA chip > is a PCIDevice. You're not supposed to replace the interrupts of the bus > it's connected to from this model as that should be done by the pci-host > or the board. Therefore modeling the chip's PIRQ/PINT pins as gpios which > is the QDev concept for that is right and your usage of pci_set_irq here > is wrong. > > > Works for me: > (08/84) tests/avocado/boot_linux_console.py:BootLinuxConsole.test_mips64el_fuloong2e: > PASS(2.77 s) The bonito code is interesting in that the IRQ is swizzled in pci_bonito_map_irq() to the internal IRQ, and then pci_bonito_set_irq() sets the output (CPU?) IRQ accordingly. This means that the routing is currently fixed based upon the slot number, rather than using the VIA PCI IRQ routing. This bit will need some thought as to how this interacts with pci_bus_irqs() in your proposed patch, feel free to suggest a suitable approach. > Please stop breaking my series. I had a working and tested series (still > have that on my pegasos2 branch in case we end up chosing that) which was > changed but you to something that is now conceptually wrong but still > works because of guests don't change firmware defaults to share PCI > interrupts with internal functions just to fulfill your assumption that > internal functions are PCI devices (which now I have proof that they don't > conform to that PCI standard doc, look at the comment in the last patch > about PCI Command register in via-ac97 and compare that with the chip > datasheet) but OK if this allows simlpler code in QEMU and still works I > can accept that but don't push ideas that are wrong. I don't think this is fair on Bernhard since his patches are coming from the place of improving the VIA ISA bridge implementation so that it can be used as an alternative to PIIX. From my perspective the work on PIIX and VIA has been well thought out, with a good understanding of the relevant specifications: the intention has always been to improve the existing code based upon a working implementation, and not to deliberately cause more work. ATB, Mark.
On Wed, 1 Mar 2023, Mark Cave-Ayland wrote: > On 27/02/2023 16:52, Bernhard Beschow wrote: >> On Mon, Feb 27, 2023 at 1:57 PM BALATON Zoltan <balaton@eik.bme.hu >> <mailto:balaton@eik.bme.hu>> wrote: >> >> On Mon, 27 Feb 2023, BALATON Zoltan wrote: >> > On Mon, 27 Feb 2023, BALATON Zoltan wrote: >> >> On Mon, 27 Feb 2023, Bernhard Beschow wrote: >> >>> Am 26. Februar 2023 23:33:20 UTC schrieb BALATON Zoltan >> >>> <balaton@eik.bme.hu <mailto:balaton@eik.bme.hu>>: >> >>>> On Sun, 26 Feb 2023, Bernhard Beschow wrote: >> >>>>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan >> >>>>> <balaton@eik.bme.hu <mailto:balaton@eik.bme.hu>>: >> >>>>>> From: Bernhard Beschow <shentey@gmail.com >> <mailto:shentey@gmail.com>> >> >>>>>> >> >>>>>> 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 >> <mailto:shentey@gmail.com>> >> >>>>>> [balaton: declare gpio inputs instead of changing pci bus irqs >> so it >> >>>>>> can >> >>>>>> be connected in board code; remove some empty lines] >> >>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu >> <mailto:balaton@eik.bme.hu>> >> >>>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de >> <mailto:ReneEngel80@emailn.de>> >> >>>>>> --- >> >>>>>> hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++ >> >>>>>> 1 file changed, 39 insertions(+) >> >>>>>> >> >>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> >>>>>> index 3f9bd0c04d..4025f9bcdc 100644 >> >>>>>> --- a/hw/isa/vt82c686.c >> >>>>>> +++ b/hw/isa/vt82c686.c >> >>>>>> @@ -604,6 +604,44 @@ 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); >> >>>>>> @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, >> Error >> >>>>>> **errp) >> >>>>>> int i; >> >>>>>> >> >>>>>> qdev_init_gpio_out(dev, &s->cpu_intr, 1); >> >>>>>> + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", >> >>>>>> PCI_NUM_PINS); >> >>>>> >> >>>>> This line is a Pegasos2 specific addition for fixing its IRQ >> handling. >> >>>>> Since this code must also work with the Fuloong2e board we >> should aim >> >>>>> for a minimal changeset here which renders this line out of >> scope. >> >>>>> >> >>>>> Let's keep the two series separate since now I need to watch two >> series >> >>>>> for comments. Please use Based-on: tag next time instead. >> >>>> >> >>>> Well, it's not. It's part of the QDev model for VT8231 that >> allows it to >> >>>> be connected by boards so I think this belongs here otherwise >> this won't >> >>>> even compile because the function you've added would be unused >> and bail >> >>>> on -Werror. Let's not make this more difficult than it is. I'm OK >> with >> >>>> reasonable changes but what's your goal now? You can't get rid of >> this >> >>>> line as it's how QDev can model it. Either I have to call into >> this model >> >>>> or have to export these pins as gpios. >> >>> >> >>> Exporting the pins is a separate aspect on top of implementing PCI >> IRQ >> >>> routing. To make this clear and obvious this should be a dedicated >> patch. >> >>> In case either patch has an issue this would also ease and >> therefore >> >>> acellerate discussions. >> >> >> >> How would you do that? Introduce via_isa_set_pci_irq() as unused in >> this >> >> patch then have a one line patch to add connecting it to gpio pins? >> That >> >> just makes no sense. This is not modeling the chip with QDev and >> then the >> >> boatd >> > >> > This is now modeling... >> > >> >> can connect it as appropriate modeling the board that the chip is >> in. So if >> >> fuloon2e needs to do that then it should. I'll check that as I was >> focusing >> > >> > fuloong2e >> >> I've checked fuloong2e and it still works as before. PCI bus is handled >> by >> bonito on that board so your patch would actually break it. The VIA >> chip >> is a PCIDevice. You're not supposed to replace the interrupts of the >> bus >> it's connected to from this model as that should be done by the >> pci-host >> or the board. Therefore modeling the chip's PIRQ/PINT pins as gpios >> which >> is the QDev concept for that is right and your usage of pci_set_irq >> here >> is wrong. >> >> >> Works for me: >> (08/84) >> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_mips64el_fuloong2e: >> PASS(2.77 s) > > The bonito code is interesting in that the IRQ is swizzled in > pci_bonito_map_irq() to the internal IRQ, and then pci_bonito_set_irq() sets > the output (CPU?) IRQ accordingly. This means that the routing is currently > fixed based upon the slot number, rather than using the VIA PCI IRQ routing. > This bit will need some thought as to how this interacts with pci_bus_irqs() > in your proposed patch, feel free to suggest a suitable approach. I don't know if you've checked what that test does but it seems it tries to boot an old debian kernel from linux-image-3.16.0-6-loongson-2e_3.16.56-1+deb8u1_mipsel.deb which even on master does not boot, it ends in: [ 0.256000] PCI: Enabling device 0000:00:06.0 (0000 -> 0003) [ 0.256000] Data bus error, epc == ffffffff8045aed8, ra == ffffffff8045aeb8 [ 0.256000] Oops[#1]: so this test likely won't notice if you break PCI interrupts replacing the bonito irq handler from VT82C686B and either this board does not seem to be working anyway or that kernel may not be the right one to boot (the board firmware does produxe picture and a boot console but maybe that's not using interrupts). I did not reply to this before because it's quite irrelevant to the series we were discussing so it would just be a distraction. >> Please stop breaking my series. I had a working and tested series >> (still >> have that on my pegasos2 branch in case we end up chosing that) which >> was >> changed but you to something that is now conceptually wrong but still >> works because of guests don't change firmware defaults to share PCI >> interrupts with internal functions just to fulfill your assumption that >> internal functions are PCI devices (which now I have proof that they >> don't >> conform to that PCI standard doc, look at the comment in the last patch >> about PCI Command register in via-ac97 and compare that with the chip >> datasheet) but OK if this allows simlpler code in QEMU and still works >> I >> can accept that but don't push ideas that are wrong. > > I don't think this is fair on Bernhard since his patches are coming from the > place of improving the VIA ISA bridge implementation so that it can be used > as an alternative to PIIX. From my perspective the work on PIIX and VIA has > been well thought out, with a good understanding of the relevant > specifications: the intention has always been to improve the existing code > based upon a working implementation, and not to deliberately cause more work. I fully support that and tried to help with review or info that could help but this particular change and the way it was forced on me did cause me to do more work. Anyway I hope we could come to a conclusion now so don't start again. Regards, BALATON Zoltan
On Wed, 1 Mar 2023, Mark Cave-Ayland wrote: > On 27/02/2023 16:52, Bernhard Beschow wrote: >> On Mon, Feb 27, 2023 at 1:57 PM BALATON Zoltan <balaton@eik.bme.hu >> <mailto:balaton@eik.bme.hu>> wrote: >> in. So if >> >> fuloon2e needs to do that then it should. I'll check that as I was >> focusing >> > >> > fuloong2e >> >> I've checked fuloong2e and it still works as before. PCI bus is handled by >> bonito on that board so your patch would actually break it. The VIA chip >> is a PCIDevice. You're not supposed to replace the interrupts of the bus >> it's connected to from this model as that should be done by the pci-host >> or the board. Therefore modeling the chip's PIRQ/PINT pins as gpios which >> is the QDev concept for that is right and your usage of pci_set_irq here >> is wrong. >> >> >> Works for me: >> (08/84) >> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_mips64el_fuloong2e: >> PASS(2.77 s) > > The bonito code is interesting in that the IRQ is swizzled in > pci_bonito_map_irq() to the internal IRQ, and then pci_bonito_set_irq() sets > the output (CPU?) IRQ accordingly. This means that the routing is currently > fixed based upon the slot number, rather than using the VIA PCI IRQ routing. > This bit will need some thought as to how this interacts with pci_bus_irqs() > in your proposed patch, feel free to suggest a suitable approach. I believe the fuloong2e may be similarly connected as the pegasos2. The Marvell Discovery II mv64361 was based on a MIPS counterpart so the concepts may be similar in these just the CPU arch is different. This doc https://wiki.qemu.org/images/0/09/Bonito64-spec.pdf says the bonito north bridge has some GPin and GPIO pins which are connected to the interrupt controller (see section 5.15). Probably you can infer which pins PCI IRQs should come in from the map_irq function in the bonito model. I'd expect GPIO0-3 based on description in the table in section 6.1 On the other hand the board's firmware suggests PCI interrupt lines are also connected to the PIRQ pins of th 686B: https://github.com/loongson-community/pmon/blob/master/sys/dev/pci/vt82c686_devbd2e.c (if this is the right file to look at as there are different versions but dev board 2e said to inlude fuloong2e in the main README). Then in 686B PCI interrupts are mapped to 9.10.11.13 with the PnP IRQ routing registers in 686B. This could then be modeled similarly to how I did it in this series for pegasos2: One could add gpio inputs in bonito to model the pins where the PCI interrupt lines are connected then connect these together in the board code just like they are wired on the real board. Although this board does not have any PCI slots so these are only for the on board PCI devices: https://www.linux-mips.org/wiki/Fuloong_2E but a similar dev board may have 4 PCI slots. Regards, BALATON Zoltan
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index 3f9bd0c04d..4025f9bcdc 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -604,6 +604,44 @@ 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); @@ -614,6 +652,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp) int i; qdev_init_gpio_out(dev, &s->cpu_intr, 1); + qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS); isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d), errp);