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

[BALATON Zoltan]

On Tue, 25 Feb 2020, address@hidden wrote:
> I don't believe the quick interrupt here is the problem. Solaris 10
> will spin for a short time while waiting for the interrupt bit to be
> set before continuing with its routine. If it doesn't see the interrupt
> bit is set before some timeout, it will print an error about the
> missing interrupt and give up loading the driver.

I don't think missing delay should cause any problem either just pointed 
this out as a difference from real controller which may have an effect but 
I agree this is probably not a problem.

> > > pci_cfg_read 53.231 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
> > > offs=0x50 val=0x4
> > > ide_ioport_read 35.577 pid=147030 addr=0x7 reg=b'Status' val=0x50
> > > bus=0x55b77f922d10 s=0x55b77f922d98
> > > ide_ioport_read 29.095 pid=147030 addr=0x7 reg=b'Status' val=0x50
> > > bus=0x55b77f922d10 s=0x55b77f922d98
> >
> > So these ide_ioport_read calls clear the irq bit...
>
> That's right. The line that I proposed removing in the patch clears
> CFG_INTR_CH0 on ide_ioport_read.

Problem with that patch is that it removes this clearing from the func 
that's also used to emulate ISA IDE ioports which according to their spec 
should clear irq on read so that function should be OK but maybe should 
not be called by PCI IDE code?

> > > ide_ioport_write 19.146 pid=147030 addr=0x6 reg=b'Device/Head'
> > > val=0xe0 bus=0x55b77f922d10 s=0x55b77f922d98
> > > pci_cfg_read 9.468 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
> > > offs=0x50 val=0x0
> > > pci_cfg_read 127.712 pid=147030 dev=b'cmd646-ide' devid=0x3
> > > fnid=0x0 offs=0x50 val=0x0
> > > pci_cfg_read 101.942 pid=147030 dev=b'cmd646-ide' devid=0x3
> > > fnid=0x0 offs=0x50 val=0x0
> >
> > ...that would be checked here?
>
> That's right.
>
> Solaris is performing pci_cfg_read on offs=0x50 until it either sees
> the interrupt bit set or times out. If it times out, you get a fatal
> error for the driver. The behaviour is not expected and aggressively
> checked against by the Solaris kernel. From what I can tell, Linux and
> OpenBSD don't check if the bit is set before clearing it.
>
> > What I don't get is why ide_ioport_read is called at all and from where 
> > if that's meant to emulate legacy ide ISA ioport reads and we have a 
> > PCI device accessed via PCI regs?

What I meant was where is ide_ioport_read() is called in this case when we 
have a PCI IDE controller? Searching for it I think it may come from 
pci_ide_data_read() in hw/ide/pci.c. This document:

http://www.bswd.com/pciide.pdf

has some info on this and there are mentions of status using Alternate 
Status (which does not clear interrupt bit) but I think that corresponds 
to pci_ide_cmd_read() which already uses ide_status_read() so that seems 
correct. I did not find anything about contents of the Command Block in 
this doc which the function with ide_ioport_read call implements so not 
sure if that's expected to clear interrupt in PCI native mode or should we 
change pci_ide_data_read() to avoid that.

> > mention irq in a lot of regs (all say write to clear) but I don't
> > understand their relation to each other and irq raised by the drive.
>
> I agree and I think that's part of the problem. The documentation does
> not explicitly mention their relation to each other. I can't see
> anything that suggests that reading the status register on the drive
> will unset bits in the pci configuration space of the controller. They
> are seperate devices.

Except the legacy IDE spec that does say reading status is clearing IRQ 
but not sure PCI native mode should do the same but it seems to use the 
same function in QEMU so it will clear IRQ as in legacy IDE mode. But this 
Linux driver says IRQ is cleared on read for PCI as well:

https://github.com/torvalds/linux/blob/master/drivers/ata/libata-sff.c

as does the CMD646 driver:

https://github.com/torvalds/linux/blob/master/drivers/ata/pata_cmd64x.c

in cmd64x_sff_irq_check() although for different chip revisions it uses 
cmd648_sff_irq_* functions which does this differently and avoids reading 
status reg and clears irq explicitely. It also has a warning at the 
beginning that UDMA mode is broken on most of these chips so it won't try 
to use it on anything below CMD646U2 so this suggests maybe there's a 
problem with clearing IRQs on at least some CMD646 chip revisions. I think 
the Sun Ultra 10 used CMD646U but not sure what the Solaris driver expects 
and if it can work with later chip revisions. Maybe we should either 
emulate the chip bugs or change something to identify as CMD646U2 which 
should behave more like stadard PCI IDE controllers? Although if I got 
that correctly Linux thinks revisions over 5 are OK and QEMU has 7.

I think we need advice from someone more knowledgeable about real hardware 
on this.

Regards,
BALATON Zoltan