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
>

Reply via email to