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

[BALATON Zoltan]

On Sat, 22 Feb 2020, address@hidden wrote:
> 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.

I've also found this thread:

https://lucky.openbsd.misc.narkive.com/hA6XG7Fu/bus-master-dma-error-missing-interrupt

which seems to talk about missing IRQ in UDMA mode similar to our problem 
and it suggests OpenBSD detects this and downgrades to PIO mode so it 
would still work. Did you check if this is why it works with OpenBSD or it 
really uses UDMA mode?

> 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 is possible. The hang I get during boot with PPC OSes I've tried is 
also becuase not being able to read CD drive after switching to UDMA mode. 
This would suggest bug may be in either common ide code or ide-cd 
emulation but could as well be in irq routing (in which case it's separate 
but similar bug in different machine emulations). Is there a way to 
disable UDMA mode in Solaris to check if it would work better when only 
using PIO? That might help locate the bug further.

In my case I've tested with both on board IDE and adding an sii3112 PCI 
card emulation, these both use common bmdma code but route IRQs 
differently. I see some irqs arriving up to the interrupt controller but 
CPU irq is not raised for some reason so I'm not sure it's a bug in common 
code or somewhere else.

> > 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

OK, I haven't checked the test just noticed the failure.

> 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.

I'd suggest to wait a few days to give people a chance to review the patch 
then submit a v2 with all the requested changes if any. You can submit v2 
right away but then if someone finds something you'll need more versions 
which is OK as well, your decision how many versions you want to submit. 
Since this patch is only 1 line there's not much people could ask to 
change about it and v2 could allow CI to run and maybe reveal problems so 
maybe in this case a v2 with also fixing the test might help to get it 
reviewed faster. I assume you're aware of the page about patch submission:

https://wiki.qemu.org/Contribute/SubmitAPatch

Regards,
BALATON Zoltan