Re: [PATCH] hw/ide: Remove status register read side effect

[<jasper.lowell@bt.com>]

> The chip docs don't mention any side effect, they only say that the
> DMA 
> IRQ and Error bits in the bus master IDE status reg (bits 2 and 1)
> are 
> write 1 to clear

I haven't found any documentation that mention that side effect either.
As you say, it only mentions that you should set the bit to clear.
While the side effect of clearing interrupts by reading the status
register doesn't appear to be in documentation anywhere (to my
knowledge), I do see this side effect referenced in the source code of
drivers occasionally.

In /drivers/ide/ide-io.c of the Linux kernel:
/*
 * Whack the status register, just in case
 * we have a leftover pending IRQ.
 */
(void)hwif->tp_ops->read_status(hwif);

Along with:
 *	There's nothing really useful we can do with an unexpected
interrupt,
 *	other than reading the status register (to clear it), and
logging it.

The CMD64x specific code in the Linux kernel does explicitly set the
IRQ bits in the bus master IDE status register to clear it. I'm not
sure why, so maybe someone else can chime in explaining why Linux
sometimes clears interrupts by reading the status register and other
times follows the documentation and sets the required bits. The OpenBSD
driver also appears to set the bits explicitly.

I think the reason why the Solaris 10 driver crashes fatally whereas
Linux and OpenBSD ignore the side effect is because when clearing
interrupts, Solaris 10 expects the interrupt bit to be set and checks
this. Linux and OpenBSD appear to clear it regardless of its state.

With the patch, I didn't notice any regression for OpenBSD under Sun4u
and there were improvements to the Solaris 10 boot under Sun4u.

> Unfortunately 
> this does not change my problems with other BMDMA devices on PPC.

This patch doesn't solve all the problems for Solaris 10. It gets
further in the boot process but it is still unable to mount the file
system. I suspect that there are more bugs in the IDE/CMD646 emulation.
I'm going to continue looking into it. It's still possible we are being
affected by the same bugs. Right now, I'm considering that the
unresponsive serial console under Sun4u on Solaris 10 is due to not
being able to mount the file system and pull the required assets for
the installation menu.

> this change seems to break 
> something else according to a CI test report from patchew.

The test appears to break here in /tests/qtest/ide-test.c for obvious
reasons:
/* Reading the status register clears the IRQ */
g_assert(!qtest_get_irq(qts, IDE_PRIMARY_IRQ));

Should the patch I've suggested be correct, this test would need to be
updated. This is my first patch attempt for QEMU so I'm not sure what
the process is. Should I be submitting a new V2 patch with these
changes? I won't have the opportunity to update the patch for a few
days but will continue watching the thread for reviews.

Thanks,
Jasper Lowell.


On Fri, 2020-02-21 at 16:43 +0100, BALATON Zoltan wrote:
> On Fri, 21 Feb 2020, jasper.lowell@bt.com wrote:
> > The Linux libATA API documentation mentions that on some hardware,
> > reading the status register has the side effect of clearing the
> > interrupt condition. When emulating the generic Sun4u machine
> > running
> > Solaris 10, the Solaris 10 CMD646 driver exits fatally because of
> > this
> > emulated side effect. This side effect is likely to not exist on
> > real
> > CMD646 hardware.
> 
> The chip docs don't mention any side effect, they only say that the
> DMA 
> IRQ and Error bits in the bus master IDE status reg (bits 2 and 1)
> are 
> write 1 to clear so this might be OK but this change seems to break 
> something else according to a CI test report from patchew.
> Unfortunately 
> this does not change my problems with other BMDMA devices on PPC.
> 
> Regards,
> BALATON Zoltan
> 
> > Signed-off-by: Jasper Lowell <jasper.lowell@bt.com>
> > ---
> > hw/ide/core.c | 1 -
> > 1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 80000eb766..82fd0632ac 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -2210,7 +2210,6 @@ uint32_t ide_ioport_read(void *opaque,
> > uint32_t addr)
> >         } else {
> >             ret = s->status;
> >         }
> > -        qemu_irq_lower(bus->irq);
> >         break;
> >     }
> > 
> >