Re: [PATCH v5 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> Am 1. März 2023 11:15:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> > > Am 1. März 2023 00:17:09 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > > > 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. The real chip may allow routing IRQs from
> > > > internal functions independently of PCI interrupts but since guests
> > > > usually configute it to a single shared interrupt we don't model that
> > > > here for simplicity.
> > > >
> > > > Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
> > > >
> > > > Suggested-by: Bernhard Beschow <shentey@gmail.com>
> > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > ---
> > > > hw/isa/vt82c686.c | 38 +++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 37 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > > > index 01e0148967..018a119964 100644
> > > > --- a/hw/isa/vt82c686.c
> > > > +++ b/hw/isa/vt82c686.c
> > > > @@ -604,6 +604,42 @@ 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
> > > > +
> > > > +    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
> > >
> > > Where does the "pic_irq > 14" come from? It's not mentioned in the datasheet.
> >
> > Check at 0x3c register of USB and AC97 functions. For the others it may be 
> > valid but unlikely to be used hence we just disallow it. (In my version which 
> > also mapped IDE here I've checkrf for each source but there's no way to do that 
> > in this version.)
>
> I'm not sure what you mean. The 0x3c regs aren't related to the PCI IRQ routing 
> regs.
>
> Moreover, as I wrote in my other mail, I wonder what the datasheet tries to 
> tell us here at all. The information there partly contradicts itself.
>
> Can you please clarify?

Here is the entire register desription that you've partly quoted before:

Offset 3C - Interrupt Line (00h)...................................... RW
7-4 Reserved ........................................always reads 0
3-0 USB Interrupt Routing ........................ default = 16h
0000 Disabled................................................. default
0001 IRQ1
0010 Reserved
0011 IRQ3
0100 IRQ4
0101 IRQ5
0110 IRQ6
0111 IRQ7
1000 IRQ8
1001 IRQ9
1010 IRQ10
1011 IRQ11
1100 IRQ12
1101 IRQ13
1110 IRQ14
1111 Disabled

Apart from the obvious typo stating default 16h the list below clearly 
says that the default is really 0 and 0 and 15 means Disabled (so if this 
is a copy paste error and the default should be 15 that would still mean 
it's disabled by default) and could be routed to any other ISA IRQ but 
you really should not route it to 2 as that would mess up the cascade IRQ. 
That's how I read that.

And yes I was trying to tell you rhat this is not related to the PCI IRQ 
routing regs which only set the IRQ for the PIRQ pins ahd this one sets 
the IRQ for the function it belongs to (USB, AC97, etc.) independently of 
that. Your patch which is now in the series does not implement this but 
uses pci interrupts instead and still works because guests don't seem to 
actually route IRQs to different interrupts just put everything on IRQ9 so 
your patch still works. As this makes QEMU model simpler we can do that 
and later if we ever need to model this for a guest that actually wants to 
use this feature of the chip you'll have my v1 series in the list archives 
where I've tried to implement the above. For me we can end it here.

Regards,
BALATON Zoltan