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

[BALATON Zoltan]

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)) ||
>
> One other thing whilst I'm here: the above line is quite cryptic to read unless
> you're very familiar with the PCI IDE specifications. How about something like 
> this
> towards the top of the function:
>
> int native = n ? pci_get_byte(d->config + PCI_CLASS_PROG) & 4 :
>                 pci_get_byte(d->config + PCI_CLASS_PROG) & 1;
>
> and change the if() accordingly:
>
> if (native) {
>   ...
> } else {
>   ...
> }
>
> With that you can could probably drop the comment since it's really obvious 
> what the
> code is doing when reading it against the datasheet.

Too late, already sent v3, may consider later but the comment is actually 
for the line after the || not the one checking CLASS_PROG.

Regards,
BALATON Zoltan