On Mon, Feb 27, 2023 at 1:57 PM BALATON Zoltan <bala...@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 > >>> <bala...@eik.bme.hu>: > >>>> On Sun, 26 Feb 2023, Bernhard Beschow wrote: > >>>>> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan > >>>>> <bala...@eik.bme.hu>: > >>>>>> From: Bernhard Beschow <shen...@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 <shen...@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 <bala...@eik.bme.hu> > >>>>>> Tested-by: Rene Engel <reneenge...@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 >