Re: [PATCH v2 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

On Tue, 10 Mar 2020, BALATON Zoltan wrote:
> On Tue, 10 Mar 2020, Mark Cave-Ayland wrote:
> > On 09/03/2020 20:17, BALATON Zoltan wrote:
> > > On Mon, 9 Mar 2020, Mark Cave-Ayland wrote:
> > > > On 09/03/2020 00:42, BALATON Zoltan wrote:
> > > > > Some machines operate in "non 100% native mode" where interrupts are
> > > > > fixed at legacy IDE interrupts and some guests expect this behaviour
> > > > > without checking based on knowledge about hardware. Even Linux has
> > > > > arch specific workarounds for this that are activated on such boards
> > > > > so this needs to be emulated as well.
> > > > >
> > > > > Signed-off-by: BALATON Zoltan <address@hidden>
> > > > > ---
> > > > > v2: Don't use PCI_INTERRUPT_LINE in via_ide_set_irq()
> > > > >
> > > > >  hw/ide/via.c            | 57 +++++++++++++++++++++++++++++++++++------
> > > > >  hw/mips/mips_fulong2e.c |  2 +-
> > > > >  include/hw/ide.h        |  3 ++-
> > > > >  3 files changed, 52 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/hw/ide/via.c b/hw/ide/via.c
> > > > > index 096de8dba0..44ecc2af29 100644
> > > > > --- a/hw/ide/via.c
> > > > > +++ b/hw/ide/via.c
> > > > > @@ -1,9 +1,10 @@
> > > > >  /*
> > > > > - * QEMU IDE Emulation: PCI VIA82C686B support.
> > > > > + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231)
> > > > >   *
> > > > >   * Copyright (c) 2003 Fabrice Bellard
> > > > >   * Copyright (c) 2006 Openedhand Ltd.
> > > > >   * Copyright (c) 2010 Huacai Chen <address@hidden>
> > > > > + * Copyright (c) 2019-2020 BALATON Zoltan
> > > > >   *
> > > > >   * Permission is hereby granted, free of charge, to any person 
> > > > > obtaining a copy
> > > > >   * of this software and associated documentation files (the 
> > > > > "Software"), to deal
> > > > > @@ -25,6 +26,8 @@
> > > > >   */
> > > > >
> > > > >  #include "qemu/osdep.h"
> > > > > +#include "qemu/range.h"
> > > > > +#include "hw/qdev-properties.h"
> > > > >  #include "hw/pci/pci.h"
> > > > >  #include "migration/vmstate.h"
> > > > >  #include "qemu/module.h"
> > > > > @@ -111,14 +114,40 @@ static void via_ide_set_irq(void *opaque, int n, 
> > > > > int level)
> > > > >      } else {
> > > > >          d->config[0x70 + n * 8] &= ~0x80;
> > > > >      }
> > > > > -
> > > > >      level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
> > > > > -    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
> > > > > -    if (n) {
> > > > > -        qemu_set_irq(isa_get_irq(NULL, n), level);
> > > > > +
> > > > > +    /*
> > > > > +     * Some machines operate in "non 100% native mode" where 
> > > > > PCI_INTERRUPT_LINE
> > > > > +     * is not used but IDE always uses ISA IRQ 14 and 15 even in native 
> > > > > mode.
> > > > > +     * Some guest drivers expect this, often without checking.
> > > > > +     */
> > > > > +    if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) ||
> > > > > +        PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) {
> > > > > +        qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level);
> > > > > +    } else {
> > > > > +        qemu_set_irq(isa_get_irq(NULL, 14), level);
> > > > >      }
> > > > >  }
> > > >
> > > > There's still the need to convert this to qdev gpio, but for now I'll 
> > > > review the
> > > > updated version of this patch (and provide an example once the rest of 
> > > > the patchset
> > > > is okay).
> > >
> > > There's no need to do that now. I think it only makes sense to do that 
> > > when the 686B
> > > and VT8231 models are also qdevified (which I'll plan to do when cleaning 
> > > up my
> > > pegasos2 patches later) since that may change how this should be 
> > > qdevified. But I
> > > don't have time to fully do it now so don't even ask. This will have to do 
> > > for now as
> > > it's not worse than it is already and does fix clients so I see no 
> > > immediate need to
> > > force more clean ups upon me.
> > >
> > > > This looks much closer to what I was expecting with the fixed IRQs, and 
> > > > in fact
> > > > doesn't it make the feature bit requirement obsolete? The PCI_CLASS_PROG 
> > > > bit is now
> > > > the single source of truth as to whether native or legacy IRQs are used, 
> > > > and the code
> > > > routes the IRQ accordingly.
> > >
> > > No, the feature bit is still needed to flag if this device should work 
> > > with 100%
> > > native mode as on fulong2e or with forced legacy IRQ non-100% native mode 
> > > as on
> > > pegasos2. In both cases PCI_CLASS_PROG is actually set to native mode 
> > > (most of the
> > > time, Linux fixes this up on pegasos2 for it's own convenience but other 
> > > OSes don't
> > > care about it but we still need to know to use legacy interrupts) so the 
> > > feature bit
> > > is needed to know when to use legacy and when native interrupts.
> > >
> > > The hardcoded IRQ14 in native mode is also wrong, If you check VT8231 
> > > datasheet it
> > > clearly says that the IRQ raised is selected by PCI_INTERRUPT_LINE so I 
> > > think my
> > > previous version was correct but this still works because we fixed 
> > > PCI_INTERRUPT_LINE
> > > to 14 so we know here it's 14 without looking at the config reg that you 
> > > forbid me to
> > > do. But due to coincidence it still works and matches your ideals about 
> > > PCI specs
> > > that I don't think apply for this device but I could not convince you 
> > > about that so
> > > if this is what it takes then so be it. As long as it works with clients I 
> > > don't
> > > care, we can always clean this up later.
> >
> > Actually I've just realised that the conversion to qdev solves all these issues,
> > because qdev has the concept of connecting outputs to inputs. The way to do 
> > this is
> > to define the VIA(PCI)IDEState with an array of two name legacy IRQs (as 
> > detailed in
> > my previous email), plus the PCI IRQ.
>
> This device (being part of an integrated southbridge chip) has no PCI IRQ at 
> all, stop talking about it, that only may apply to CMD646. Once you accept 
> this it may be easier to actually get to a solution. The via-ide either 
> drives the legacy interrupts in legacy mode and on pegasos2 always, 
> regardless of mode. Or in true native mode it drives one ISA interrupt for 
> both channels selected by PCI_INTERRUPT_LINE config reg. This is clear from 
> the VT8231 datasheet and my test with guests.
>
> > Then all via_ide_set_irq() has to do is set each qdev gpio level accordingly; 
> > if a
> > board isn't interested in an IRQ it just doesn't wire it up. The final piece of 
> > the
> > puzzle is what do we do in PCI native mode, but that is simple: always drive the
> > legacy IRQs if there is no PCI IRQ connected.

Also even though we don't model that but real hardware can switch between 
native and legacy mode so in reality all boards need all irqs as they 
could be used depending on device setting. I think this legacy-irq thing 
is something peculiar to pegasos2 so using a feature but only set by that 
board is appropriate here. Unless you can propose something simpler you 
could just accept it at least for now and maybe we can try to clean it up 
later when other parts of the southbridge are qdevified.

> How does the device code know if there's anything connected to the gpio it 
> defines? I don't see your proposal would be simpler or even easier to 
> understand (aka cleaner) than what I've implemented. You'll need to explain a 
> bit more but I think the real solution is to model VT82C686B and VT8231 and 
> not parts of it in qdev. We don't want to model the inside of these chips.
>
> Regards,
> BALATON Zoltan