[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 4/8] hw/isa/vt82c686: Implement PCI IRQ routing

From: Mark Cave-Ayland
Subject: Re: [PATCH v3 4/8] hw/isa/vt82c686: Implement PCI IRQ routing
Date: Wed, 1 Mar 2023 20:59:41 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0

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 
     >>>>>> 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, 
     >>>>>> 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 
     >>>>>> +{
     >>>>>> +    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 
     >>>>>> 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 
     >>>>> 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 
     >>>>> 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 
     >>>> be connected by boards so I think this belongs here otherwise this 
     >>>> even compile because the function you've added would be unused and 
     >>>> 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 
     >>>> 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 
     >>> 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 
     >> 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 
     > 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]