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

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

[jasper.lowell]

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.

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;
     }
 
-- 
2.24.1

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200221065015.337915-1-jasper.lowell@bt.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-qtest-x86_64: tests/qtest/ide-test
  TEST    check-unit: tests/test-iov
**
ERROR:/tmp/qemu-test/src/tests/qtest/ide-test.c:294:send_dma_request: assertion failed: (!qtest_get_irq(qts, IDE_PRIMARY_IRQ))
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/ide-test.c:294:send_dma_request: assertion failed: (!qtest_get_irq(qts, IDE_PRIMARY_IRQ))
make: *** [check-qtest-x86_64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-unit: tests/test-bitmap
  TEST    check-unit: tests/test-aio
---
  TEST    iotest-qcow2: 283
Failures: 161
Failed 1 of 116 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3349986ab83849d5b475dd101bed4f05', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-gw5__giy/src/docker-src.2020-02-21-02.54.48.27828:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=3349986ab83849d5b475dd101bed4f05
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-gw5__giy/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    13m27.186s
user    0m9.054s


The full log is available at
http://patchew.org/logs/20200221065015.337915-1-jasper.lowell@bt.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[BALATON Zoltan]

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

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

[jasper.lowell]

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

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

[BALATON Zoltan]

On Sat, 22 Feb 2020, jasper.lowell@bt.com 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

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

[BALATON Zoltan]

On Sat, 22 Feb 2020, jasper.lowell@bt.com wrote:
> 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

The PCI bus master IDE spec also only says the status register is read / 
write clear but does not mention clearing bits on read. I've also traced 
back the origin of this code in QEMU and looks like it was there forever 
since commit fc01f7e7f90 when IDE emulation was added. Interesting that 
only Solaris driver broke because of this so maybe there are hardware 
implementations which do clear this bit so drivers are prepared to handle 
that. I think based on at least two docs it would be correct to remove 
this but wonder what other drivers would this change break. Anyway:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> 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

It seems likely that CMD646 does not clear irq on read but some other 
controllers probably do.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

On Sat, 22 Feb 2020, jasper.lowell@bt.com wrote:
> 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

One more idea to check is if the irq of the IDE device is mapped at the 
same IRQ line and described correctly in the open firmware device tree. 
You may need info from a real machine for that, I've tried to search for 
it but could not find any device tree dumps or info on how irqs are mapped 
in the real hardware. Since Linux can find it it's probably OK but maybe 
Solaris expects the irq to be mapped somewhere else or gets the info on 
this from some other place than Linux and that could have a bug? Not sure 
if this helps but I have no better idea.

Regards,
BALATON Zoltan

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

[Mark Cave-Ayland]

On 21/02/2020 06:50, 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.
> 
> 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;
>      }

I don't think that this is correct: from memory when I last looked at this, there
were 2 IDE status registers: the one from the original specification which clears the
IRQ upon read, and another one in subsequent revisions which allows you to read the
value without clearing any pending IRQ. My guess would be that changing this would
not only cause QEMU to deviate from the specification, but causes problems in other OSs.


ATB,

Mark.

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

[BALATON Zoltan]

On Sat, 22 Feb 2020, Mark Cave-Ayland wrote:
> On 21/02/2020 06:50, 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.
>>
>> 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;
>>      }
>
> I don't think that this is correct: from memory when I last looked at this, there
> were 2 IDE status registers: the one from the original specification which clears the
> IRQ upon read, and another one in subsequent revisions which allows you to read the
> value without clearing any pending IRQ. My guess would be that changing this would
> not only cause QEMU to deviate from the specification, but causes problems in other OSs.

You're right, legacy ide has two status registers as described here:

ftp://ftp.seagate.com/pub/acrobat/reference/111-1c.pdf

Now question is which of these the above is emulating? Looks like CMD646 
should not clear interrupt when reading status reg so maybe instead of 
removing this from here another change is needed to CMD646 specific read 
func to read alternate status instead of status reg?

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

On Sat, 22 Feb 2020, BALATON Zoltan wrote:
> On Sat, 22 Feb 2020, Mark Cave-Ayland wrote:
>> On 21/02/2020 06:50, 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.
>>> 
>>> 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;
>>>      }
>> 
>> I don't think that this is correct: from memory when I last looked at this, 
>> there
>> were 2 IDE status registers: the one from the original specification which 
>> clears the
>> IRQ upon read, and another one in subsequent revisions which allows you to 
>> read the
>> value without clearing any pending IRQ. My guess would be that changing 
>> this would
>> not only cause QEMU to deviate from the specification, but causes problems 
>> in other OSs.
>
> You're right, legacy ide has two status registers as described here:
>
> ftp://ftp.seagate.com/pub/acrobat/reference/111-1c.pdf
>
> Now question is which of these the above is emulating? Looks like CMD646

We have both ide_status_read() which does not clear irq and 
ide_ioport_read() which does. pci_ide_cmd_read() which PCI ide should use 
calls ide_status_read() so I wonder why did reading status cleared irq on 
CMD646? So maybe it's cleared from somewhere else and above change should 
not be needed.

Regards,
BALATON Zoltan

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

[jasper.lowell]

I'm having another look at the SET_FEATURE Solaris 10 error. I've
enabled tracing and I see the following. The pci_cfg_read that shows up
at the end continues a few thousand times(?) but I've omitted it. This
appears to time out or something and then Solaris gives up on the
device.

ide_ioport_read 41.468 pid=147030 addr=0x7 reg=b'Status' val=0x0
bus=0x55b77f922d10 s=0x55b77f
923168
ide_ioport_write 19.677 pid=147030 addr=0x6 reg=b'Device/Head' val=0xe0
bus=0x55b77f922d10 s=0
x55b77f923168
ide_ioport_write 19.417 pid=147030 addr=0x1 reg=b'Features' val=0x3
bus=0x55b77f922d10 s=0x55b
77f922d98
ide_ioport_write 9.027 pid=147030 addr=0x2 reg=b'Sector Count' val=0x42
bus=0x55b77f922d10 s=0
x55b77f922d98
ide_ioport_write 8.025 pid=147030 addr=0x7 reg=b'Command' val=0xef
bus=0x55b77f922d10 s=0x55b7
7f922d98
ide_exec_cmd 0.461 pid=147030 bus=0x55b77f922d10 state=0x55b77f922d98
cmd=0xef
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=0x55b77
f922d98
ide_ioport_read 29.095 pid=147030 addr=0x7 reg=b'Status' val=0x50
bus=0x55b77f922d10 s=0x55b77
f922d98
ide_ioport_write 19.146 pid=147030 addr=0x6 reg=b'Device/Head' val=0xe0
bus=0x55b77f922d10 s=0
x55b77f922d98
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
pci_cfg_read 101.793 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 101.852 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 101.803 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 101.762 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 105.219 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 102.634 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 101.943 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 101.883 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0
pci_cfg_read 101.792 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x0

It looks like I've made mistakes in previous comments about the error
and what the problem might be. Excuse my inexperience. Rather than
spinning on ARTTIM23_INTR_CH1 it might be the case that Solaris 10 is
spinning on CFR_INTR_CH0. I think this because of the following trace:

pci_cfg_read 53.231 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x4

The two reads on the io status register show that DRDY (drive ready
indicator bit) and DSC (drive seek complete bit). This doesn't look
unusual to me. The error bit is also not set which is reassuring.

I read through some of 
ftp://ftp.seagate.com/pub/acrobat/reference/111-1c.pdf and I'm confused
by the discussion regarding interrupts and the status register.

> INTRQ is cleared when the host reads the status register.

My understand is that INTRQ is the signal from pin 31 on the drive and
that the status register is on the drive. I understand the quoted
statement as when the host (CMD646) reads the status register of the
drive, the drive will lower the interrupt on this pin.

The CMD646 has CFR_INTR_CH0 and ARTTIM23_INTR_CH1 in it's PCI
configuration space. This is necessary to determine the source of an
interrupt when the CMD646 ports are in PCI IDE Native Mode. Are we
saying that when the drive lowers the interrupt, the CMD646 sees this
and then clears CFR_INTR_CH0 and ARTTIM23_INTR_CH1 automatically? If
this were the case then I don't know why there is an interface to clear
them by writing to them.

Also, if reading the ioport status register was enough to clear
CFR_INTR_CH0 and ARTTIM23_INTR_CH1 (specific to CMD646) I can't reason
why Linux, Solaris, and OpenBSD would have specific routines to clear
them (following the CMD646 documentation) rather than just reading the
ioport status register.

With the patch, the tracing output changes to this:
ide_ioport_read 128.512 pid=162907 addr=0x7 reg=b'Status' val=0x0
bus=0x55909512bd10 s=0x55909512c168
ide_ioport_write 22.622 pid=162907 addr=0x6 reg=b'Device/Head' val=0xe0
bus=0x55909512bd10 s=0x55909512c168
ide_ioport_write 21.330 pid=162907 addr=0x1 reg=b'Features' val=0x3
bus=0x55909512bd10 s=0x55909512bd98
ide_ioport_write 13.926 pid=162907 addr=0x2 reg=b'Sector Count'
val=0x42 bus=0x55909512bd10 s=0x55909512bd98
ide_ioport_write 9.278 pid=162907 addr=0x7 reg=b'Command' val=0xef
bus=0x55909512bd10 s=0x55909512bd98
ide_exec_cmd 0.921 pid=162907 bus=0x55909512bd10 state=0x55909512bd98
cmd=0xef
pci_cfg_read 40.647 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x4
ide_ioport_read 40.445 pid=162907 addr=0x7 reg=b'Status' val=0x50
bus=0x55909512bd10 s=0x55909512bd98
ide_ioport_read 31.580 pid=162907 addr=0x7 reg=b'Status' val=0x50
bus=0x55909512bd10 s=0x55909512bd98
ide_ioport_write 17.923 pid=162907 addr=0x6 reg=b'Device/Head' val=0xe0
bus=0x55909512bd10 s=0x55909512bd98
pci_cfg_read 10.931 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x4
pci_cfg_read 19.136 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x4
pci_cfg_write 26.650 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0
offs=0x50 val=0x4

Now there is no fatal error and Solaris does the expected by setting
CFR_INTR_CH0 to clear it. QEMU then updates the interrupt status in
cmd646_pci_config_write.

I'm still unconvinced that we should be lowering interrupts when the
ide ioport status register is read. I might be misunderstanding the
documentation though (definitely possible). If I am misunderstanding
the documentation, could you or Mark clarify what I'm getting wrong
here.

Thanks,
Jasper Lowell.


On Sat, 2020-02-22 at 21:05 +0100, BALATON Zoltan wrote:
> On Sat, 22 Feb 2020, BALATON Zoltan wrote:
> > On Sat, 22 Feb 2020, Mark Cave-Ayland wrote:
> > > On 21/02/2020 06:50, 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.
> > > > 
> > > > 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;
> > > >      }
> > > 
> > > I don't think that this is correct: from memory when I last
> > > looked at this, 
> > > there
> > > were 2 IDE status registers: the one from the original
> > > specification which 
> > > clears the
> > > IRQ upon read, and another one in subsequent revisions which
> > > allows you to 
> > > read the
> > > value without clearing any pending IRQ. My guess would be that
> > > changing 
> > > this would
> > > not only cause QEMU to deviate from the specification, but causes
> > > problems 
> > > in other OSs.
> > 
> > You're right, legacy ide has two status registers as described
> > here:
> > 
> > ftp://ftp.seagate.com/pub/acrobat/reference/111-1c.pdf
> > 
> > Now question is which of these the above is emulating? Looks like
> > CMD646
> 
> We have both ide_status_read() which does not clear irq and 
> ide_ioport_read() which does. pci_ide_cmd_read() which PCI ide should
> use 
> calls ide_status_read() so I wonder why did reading status cleared
> irq on 
> CMD646? So maybe it's cleared from somewhere else and above change
> should 
> not be needed.
> 
> Regards,
> BALATON Zoltan

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

[BALATON Zoltan]

On Sun, 23 Feb 2020, jasper.lowell@bt.com wrote:
> ide_exec_cmd 0.461 pid=147030 bus=0x55b77f922d10 state=0x55b77f922d98 cmd=0xef

The command is run here if I'm not mistaken Does this set the irq right 
away on QEMU where on real hadware this may take some time? Not sure if 
that's a problem but trying to understand what's happening.

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

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

> It looks like I've made mistakes in previous comments about the error
> and what the problem might be. Excuse my inexperience. Rather than
> spinning on ARTTIM23_INTR_CH1 it might be the case that Solaris 10 is
> spinning on CFR_INTR_CH0. I think this because of the following trace:
>
> pci_cfg_read 53.231 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0 offs=0x50 val=0x4
>
> The two reads on the io status register show that DRDY (drive ready
> indicator bit) and DSC (drive seek complete bit). This doesn't look
> unusual to me. The error bit is also not set which is reassuring.

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? Should the device behave differently in 
legacy and native mode with respect of clearing irq bit on register reads?

> I read through some of
> ftp://ftp.seagate.com/pub/acrobat/reference/111-1c.pdf and I'm confused
> by the discussion regarding interrupts and the status register.
>
>> INTRQ is cleared when the host reads the status register.
>
> My understand is that INTRQ is the signal from pin 31 on the drive and
> that the status register is on the drive. I understand the quoted
> statement as when the host (CMD646) reads the status register of the
> drive, the drive will lower the interrupt on this pin.
>
> The CMD646 has CFR_INTR_CH0 and ARTTIM23_INTR_CH1 in it's PCI
> configuration space. This is necessary to determine the source of an
> interrupt when the CMD646 ports are in PCI IDE Native Mode. Are we
> saying that when the drive lowers the interrupt, the CMD646 sees this
> and then clears CFR_INTR_CH0 and ARTTIM23_INTR_CH1 automatically? If
> this were the case then I don't know why there is an interface to clear
> them by writing to them.

There's a possibility that software may want to clear bits without reading 
the current value so having a way to do that can be explained.

> Also, if reading the ioport status register was enough to clear
> CFR_INTR_CH0 and ARTTIM23_INTR_CH1 (specific to CMD646) I can't reason
> why Linux, Solaris, and OpenBSD would have specific routines to clear
> them (following the CMD646 documentation) rather than just reading the
> ioport status register.

But the doc not mentioning irq bits should be cleared on read and drivers 
do it by writing after reading is sufficient evidence that CMD646 likely 
does not clear bits on read.

> With the patch, the tracing output changes to this:
> ide_ioport_read 128.512 pid=162907 addr=0x7 reg=b'Status' val=0x0 bus=0x55909512bd10 s=0x55909512c168
> ide_ioport_write 22.622 pid=162907 addr=0x6 reg=b'Device/Head' val=0xe0 bus=0x55909512bd10 s=0x55909512c168
> ide_ioport_write 21.330 pid=162907 addr=0x1 reg=b'Features' val=0x3 bus=0x55909512bd10 s=0x55909512bd98
> ide_ioport_write 13.926 pid=162907 addr=0x2 reg=b'Sector Count' val=0x42 bus=0x55909512bd10 s=0x55909512bd98
> ide_ioport_write 9.278 pid=162907 addr=0x7 reg=b'Command' val=0xef bus=0x55909512bd10 s=0x55909512bd98
> ide_exec_cmd 0.921 pid=162907 bus=0x55909512bd10 state=0x55909512bd98 cmd=0xef
> pci_cfg_read 40.647 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0 offs=0x50 val=0x4
> ide_ioport_read 40.445 pid=162907 addr=0x7 reg=b'Status' val=0x50 bus=0x55909512bd10 s=0x55909512bd98
> ide_ioport_read 31.580 pid=162907 addr=0x7 reg=b'Status' val=0x50 bus=0x55909512bd10 s=0x55909512bd98
> ide_ioport_write 17.923 pid=162907 addr=0x6 reg=b'Device/Head' val=0xe0 bus=0x55909512bd10 s=0x55909512bd98
> pci_cfg_read 10.931 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0 offs=0x50 val=0x4
> pci_cfg_read 19.136 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0 offs=0x50 val=0x4
> pci_cfg_write 26.650 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0 offs=0x50 val=0x4

The difference here is that status bits still there after ide_ioport_read 
when it gets it via pci_cfg_read than writes to that reg to clear it.

> Now there is no fatal error and Solaris does the expected by setting
> CFR_INTR_CH0 to clear it. QEMU then updates the interrupt status in
> cmd646_pci_config_write.
>
> I'm still unconvinced that we should be lowering interrupts when the
> ide ioport status register is read. I might be misunderstanding the
> documentation though (definitely possible). If I am misunderstanding
> the documentation, could you or Mark clarify what I'm getting wrong
> here.

I'm afraid I don't understand the problem enough either to be able to 
help. Maybe you could try to find out where is ide_ioport_read called in 
the above and if that's correct to call it there. Also the CMD646U docs 
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've 
found these:

0x3c INTLINE R/W Interrupt line default=14

0x50 CFR R Configuration register
      bit2 1=interrupt pending, write 1 will clear this bit (in read only reg?)

0x57 ARTTIM23 R/W Drive2/3 Control/Status Register
      bit4 IDE drive 2/3 interrupt status (write 1 to clear)

0x71 or BAR4+1 MRDMODE DMA Master Read Mode Select
      bit2 Primary channel IDE interrupt (write 1 to clear)
      bit3 Secondary channel IDE interrupt (write 1 to clear)
      bit4 Primary channel interrupt enable
           0 = propagate primary channel IDE interrupts to IRQ14 or INTA# pin (default)
           1 = block primary channel IDE interrupts (NOTE: this does not affect the function of bit 2)
      bit5 same as bit4 for but for Secondary channel and IRQ15

0x72 or BAR4+2 R/W BMIDESR0 Bus Master IDE Status Reg 0 for primary channel
      bit2 interrupt generated on IDE bus for DMA transfer (write 1 to clear)

0x7a or BAR4+0xa R/W BMIDESR1 Bus Master IDE Status Reg 0 for secondary channel
      bit2 like BMIDESR0 but for channel 1 I think

So maybe in DMA mode the BM* regs should be used and in legacy mode these 
interrupts would go to ISA IRQ14 and 15 and cleared on read as per the IDE 
spec while in native mode PCI INTA is raised and not cleared but the chip 
docs don't say anything about this so it's only guessing. Does anyone have 
more info on this?

Regards,
BALATON Zoltan

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

[jasper.lowell]

> > ide_exec_cmd 0.461 pid=147030 bus=0x55b77f922d10
> > state=0x55b77f922d98 cmd=0xef

> The command is run here if I'm not mistaken Does this set the irq
> right 
> away on QEMU where on real hadware this may take some time? Not sure
> if 
> that's a problem but trying to understand what's happening.

Yes. QEMU raises an IRQ on the completion of the command.

complete = ide_cmd_table[val].handler(s, val);
if (complete) {
	s->status &= ~BUSY_STAT;
	assert(!!s->error == !!(s->status & ERR_STAT));

	if ((ide_cmd_table[val].flags & SET_DSC) && !s->error) {
		s->status |= SEEK_STAT;
	}

	ide_cmd_done(s);
	ide_set_irq(s->bus);
}

This code from /qemu/hw/ide/core.c is executed when the SET_FEATURE
command request is made. I have tested that if this interrupt is not
made, Solaris 10 will complain accordingly with a unique error message.

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.

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

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

Taken from the ATA specification:
All commands that do not include read- or write-data transfers generate
a single interrupt when the command completes. Resets do not generate
an interrupt.

There will be an interrupt whether the command is successful or not. If
the host wants to know if an error occured it needs to inspect the
status register. Solaris might be doing this. As the trace shows, there
is no error and nothing is out of the ordinary.

There are two devices. The PCI/IDE controller (CMD646) and the ATA
compliant drive. The command, feature, and status registers belong to
the drive. If you want to configure the drive in some way or interact
with it you will use the ioport_read/write interface. CFR_INTR_CH0 and
ARTTIM23_INTR_CH1 are PCI registers in the PCI configuration space that
belongs to the PCI/IDE controller (CMD646). It makes sense to me that
both are used.

> There's a possibility that software may want to clear bits without
> reading 
> the current value so having a way to do that can be explained.

I agree that this might be a possibility. I also think its very normal
for kernel drivers to drop the return value from operations when they
are only interested in the side-effect.

> I'm afraid I don't understand the problem enough either to be able
> to 
> help. Maybe you could try to find out where is ide_ioport_read called
> in 
> the above and if that's correct to call it there. Also the CMD646U
> docs 
> 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.

> So maybe in DMA mode the BM* regs should be used and in legacy mode
> these 
> interrupts would go to ISA IRQ14 and 15 and cleared on read as per
> the IDE 
> spec while in native mode PCI INTA is raised and not cleared but the
> chip 
> docs don't say anything about this so it's only guessing.

This might be true but I'm suspicious. In native mode the host should
be checking the PCI registers to identify what device was responsible
for the interrupt because it's multiplexed. This should be done before
inspecting the status register of the device that caused the interrupt.

> Does anyone
> have 
> more info on this?

Thanks,
Jasper Lowell.

On Sun, 2020-02-23 at 16:16 +0100, BALATON Zoltan wrote:
> On Sun, 23 Feb 2020, jasper.lowell@bt.com wrote:
> > ide_exec_cmd 0.461 pid=147030 bus=0x55b77f922d10
> > state=0x55b77f922d98 cmd=0xef
> 
> The command is run here if I'm not mistaken Does this set the irq
> right 
> away on QEMU where on real hadware this may take some time? Not sure
> if 
> that's a problem but trying to understand what's happening.
> 
> > 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...
> 
> > 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?
> 
> > It looks like I've made mistakes in previous comments about the
> > error
> > and what the problem might be. Excuse my inexperience. Rather than
> > spinning on ARTTIM23_INTR_CH1 it might be the case that Solaris 10
> > is
> > spinning on CFR_INTR_CH0. I think this because of the following
> > trace:
> > 
> > pci_cfg_read 53.231 pid=147030 dev=b'cmd646-ide' devid=0x3 fnid=0x0
> > offs=0x50 val=0x4
> > 
> > The two reads on the io status register show that DRDY (drive ready
> > indicator bit) and DSC (drive seek complete bit). This doesn't look
> > unusual to me. The error bit is also not set which is reassuring.
> 
> 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? Should the device behave differently
> in 
> legacy and native mode with respect of clearing irq bit on register
> reads?
> 
> > I read through some of
> > ftp://ftp.seagate.com/pub/acrobat/reference/111-1c.pdf and I'm
> > confused
> > by the discussion regarding interrupts and the status register.
> > 
> > > INTRQ is cleared when the host reads the status register.
> > 
> > My understand is that INTRQ is the signal from pin 31 on the drive
> > and
> > that the status register is on the drive. I understand the quoted
> > statement as when the host (CMD646) reads the status register of
> > the
> > drive, the drive will lower the interrupt on this pin.
> > 
> > The CMD646 has CFR_INTR_CH0 and ARTTIM23_INTR_CH1 in it's PCI
> > configuration space. This is necessary to determine the source of
> > an
> > interrupt when the CMD646 ports are in PCI IDE Native Mode. Are we
> > saying that when the drive lowers the interrupt, the CMD646 sees
> > this
> > and then clears CFR_INTR_CH0 and ARTTIM23_INTR_CH1 automatically?
> > If
> > this were the case then I don't know why there is an interface to
> > clear
> > them by writing to them.
> 
> There's a possibility that software may want to clear bits without
> reading 
> the current value so having a way to do that can be explained.
> 
> > Also, if reading the ioport status register was enough to clear
> > CFR_INTR_CH0 and ARTTIM23_INTR_CH1 (specific to CMD646) I can't
> > reason
> > why Linux, Solaris, and OpenBSD would have specific routines to
> > clear
> > them (following the CMD646 documentation) rather than just reading
> > the
> > ioport status register.
> 
> But the doc not mentioning irq bits should be cleared on read and
> drivers 
> do it by writing after reading is sufficient evidence that CMD646
> likely 
> does not clear bits on read.
> 
> > With the patch, the tracing output changes to this:
> > ide_ioport_read 128.512 pid=162907 addr=0x7 reg=b'Status' val=0x0
> > bus=0x55909512bd10 s=0x55909512c168
> > ide_ioport_write 22.622 pid=162907 addr=0x6 reg=b'Device/Head'
> > val=0xe0 bus=0x55909512bd10 s=0x55909512c168
> > ide_ioport_write 21.330 pid=162907 addr=0x1 reg=b'Features' val=0x3
> > bus=0x55909512bd10 s=0x55909512bd98
> > ide_ioport_write 13.926 pid=162907 addr=0x2 reg=b'Sector Count'
> > val=0x42 bus=0x55909512bd10 s=0x55909512bd98
> > ide_ioport_write 9.278 pid=162907 addr=0x7 reg=b'Command' val=0xef
> > bus=0x55909512bd10 s=0x55909512bd98
> > ide_exec_cmd 0.921 pid=162907 bus=0x55909512bd10
> > state=0x55909512bd98 cmd=0xef
> > pci_cfg_read 40.647 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0
> > offs=0x50 val=0x4
> > ide_ioport_read 40.445 pid=162907 addr=0x7 reg=b'Status' val=0x50
> > bus=0x55909512bd10 s=0x55909512bd98
> > ide_ioport_read 31.580 pid=162907 addr=0x7 reg=b'Status' val=0x50
> > bus=0x55909512bd10 s=0x55909512bd98
> > ide_ioport_write 17.923 pid=162907 addr=0x6 reg=b'Device/Head'
> > val=0xe0 bus=0x55909512bd10 s=0x55909512bd98
> > pci_cfg_read 10.931 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0
> > offs=0x50 val=0x4
> > pci_cfg_read 19.136 pid=162907 dev=b'cmd646-ide' devid=0x3 fnid=0x0
> > offs=0x50 val=0x4
> > pci_cfg_write 26.650 pid=162907 dev=b'cmd646-ide' devid=0x3
> > fnid=0x0 offs=0x50 val=0x4
> 
> The difference here is that status bits still there after
> ide_ioport_read 
> when it gets it via pci_cfg_read than writes to that reg to clear it.
> 
> > Now there is no fatal error and Solaris does the expected by
> > setting
> > CFR_INTR_CH0 to clear it. QEMU then updates the interrupt status in
> > cmd646_pci_config_write.
> > 
> > I'm still unconvinced that we should be lowering interrupts when
> > the
> > ide ioport status register is read. I might be misunderstanding the
> > documentation though (definitely possible). If I am
> > misunderstanding
> > the documentation, could you or Mark clarify what I'm getting wrong
> > here.
> 
> I'm afraid I don't understand the problem enough either to be able
> to 
> help. Maybe you could try to find out where is ide_ioport_read called
> in 
> the above and if that's correct to call it there. Also the CMD646U
> docs 
> 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've 
> found these:
> 
> 0x3c INTLINE R/W Interrupt line default=14
> 
> 0x50 CFR R Configuration register
>       bit2 1=interrupt pending, write 1 will clear this bit (in read
> only reg?)
> 
> 0x57 ARTTIM23 R/W Drive2/3 Control/Status Register
>       bit4 IDE drive 2/3 interrupt status (write 1 to clear)
> 
> 0x71 or BAR4+1 MRDMODE DMA Master Read Mode Select
>       bit2 Primary channel IDE interrupt (write 1 to clear)
>       bit3 Secondary channel IDE interrupt (write 1 to clear)
>       bit4 Primary channel interrupt enable
>            0 = propagate primary channel IDE interrupts to IRQ14 or
> INTA# pin (default)
>            1 = block primary channel IDE interrupts (NOTE: this does
> not affect the function of bit 2)
>       bit5 same as bit4 for but for Secondary channel and IRQ15
> 
> 0x72 or BAR4+2 R/W BMIDESR0 Bus Master IDE Status Reg 0 for primary
> channel
>       bit2 interrupt generated on IDE bus for DMA transfer (write 1
> to clear)
> 
> 0x7a or BAR4+0xa R/W BMIDESR1 Bus Master IDE Status Reg 0 for
> secondary channel
>       bit2 like BMIDESR0 but for channel 1 I think
> 
> So maybe in DMA mode the BM* regs should be used and in legacy mode
> these 
> interrupts would go to ISA IRQ14 and 15 and cleared on read as per
> the IDE 
> spec while in native mode PCI INTA is raised and not cleared but the
> chip 
> docs don't say anything about this so it's only guessing. Does anyone
> have 
> more info on this?
> 
> Regards,
> BALATON Zoltan

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

[BALATON Zoltan]

On Tue, 25 Feb 2020, jasper.lowell@bt.com 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

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

[jasper.lowell]

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

This might be it.

The patch I provided is definitely incorrect and deviates from the
specification as Mark mentioned earlier. I misunderstood what
ide_ioport_read/write were for and haven't been thinking about legacy
mode. 

The bug that I believe exists is present when the CMD646 is operating
in PCI native mode. Yeah, I think a possible solution might be to avoid
using the ioport_read/write functions from the PCI code if they have
side effects that assume the device is in legacy mode. I'll have to
spend more time reading through the code and documentation.

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

According to the CMD646U2 specification:
"When an IDE port is in PCI IDE Legacy Mode, the PCI646U2 is compatible
with standard ISA IDE. The IDE task file registers are mapped to the
standard ISA port addresses, and IDE drive interrupts occur at IRQ14
(primary) or IRQ15 (secondary)."

In legacy mode, IRQ14 and IRQ15 mirror the state of INTRQ on each of
the selected IDE devices. QEMU appears to emulate this correctly.

In PCI native mode, INTRQ is not mirrored or given a single IRQ.
Interrupts are provided by the PCI IDE controller depending on the
controller's logic. For instance, an IDE device can raise an interrupt
but the CMD646 may not propagate that interrupt if MRDMODE has certain
bits set. I'm thinking that maybe the controller does not have logic to
unset the interrupt bits in CFR and ARTTIM23 when the IDE device lowers
INTRQ. This might mean that the controller will continue to assert an
interrupt while bits in CFR and ARTTIM23 remain set, even if the IDE
device lowers INTRQ. This would explain why the CMD646 documentation
instructs developers to lower them explicitly.

> 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'm not sure what it expects. If the Sun Ultra 10 shipped with the
CMD646U, I reason that Solaris 10 either expects it or has support for
it.

The Linux driver code appears to be consistent with the behaviour that
I'm seeing from Solaris 10.

The following appears to be used to initialise the CMD646U.

{	/* CMD 646U with broken UDMA */
	.flags = ATA_FLAG_SLAVE_POSS,
	.pio_mask = ATA_PIO4,
	.mwdma_mask = ATA_MWDMA2,
	.port_ops = &cmd646r3_port_ops
},

The port operations it uses are defined as so:

static struct ata_port_operations cmd646r3_port_ops = {
	.inherits	= &cmd64x_base_ops,
	.sff_irq_check	= cmd648_sff_irq_check,
	.sff_irq_clear	= cmd648_sff_irq_clear,
	.cable_detect	= ata_cable_40wire,
}

As you mention, cmd648_sff_irq_clear clears interrupts explicitly by
setting bits in MRDMODE - consistent with the CMD646U2 documentation.
This behaviour is very similar to Solaris 10.

> Although if I got 
> that correctly Linux thinks revisions over 5 are OK and QEMU has 7.

I'm not sure how revision numbers work with these chips. Do CMD646 and
CMD646U2 refer to different revisions of the CMD646 chip?

Thanks,
Jasper Lowell.


On Tue, 2020-02-25 at 16:08 +0100, BALATON Zoltan wrote:
> On Tue, 25 Feb 2020, jasper.lowell@bt.com 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

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

[BALATON Zoltan]

On Wed, 26 Feb 2020, jasper.lowell@bt.com wrote:
> > 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?
> 
> This might be it.
> 
> The patch I provided is definitely incorrect and deviates from the
> specification as Mark mentioned earlier. I misunderstood what
> ide_ioport_read/write were for and haven't been thinking about legacy
> mode. 
> 
> The bug that I believe exists is present when the CMD646 is operating
> in PCI native mode. Yeah, I think a possible solution might be to avoid
> using the ioport_read/write functions from the PCI code if they have
> side effects that assume the device is in legacy mode. I'll have to
> spend more time reading through the code and documentation.

Since both the generic PCI IDE and CMD646 Linux drivers mention irq is 
cleared on reading status I think using ide_ioport_read in hw/ide/pci.c 
may be correct for the generic case. Not sure if the CMD646 has some 
pecularity but maybe the difference in drivers is to avoid bugs not 
because of CMD646 not clearing irq. The wikipedia page of CMD640:

https://en.wikipedia.org/wiki/CMD640

mentions some versions of it has a bug similar to RZ-1000 for which 
there's a doc referenced that says the problem is that it forgets last 
data word after raising (or clearing?) IRQ and a workaround is to avoid 
checking status until all data transferred. This may explain why Linux 
checks alt status and clears interrupt instead of reading status register.

> According to the CMD646U2 specification:
> "When an IDE port is in PCI IDE Legacy Mode, the PCI646U2 is compatible
> with standard ISA IDE. The IDE task file registers are mapped to the
> standard ISA port addresses, and IDE drive interrupts occur at IRQ14
> (primary) or IRQ15 (secondary)."
> 
> In legacy mode, IRQ14 and IRQ15 mirror the state of INTRQ on each of
> the selected IDE devices. QEMU appears to emulate this correctly.
> 
> In PCI native mode, INTRQ is not mirrored or given a single IRQ.
> Interrupts are provided by the PCI IDE controller depending on the
> controller's logic. For instance, an IDE device can raise an interrupt
> but the CMD646 may not propagate that interrupt if MRDMODE has certain
> bits set. I'm thinking that maybe the controller does not have logic to
> unset the interrupt bits in CFR and ARTTIM23 when the IDE device lowers
> INTRQ. This might mean that the controller will continue to assert an
> interrupt while bits in CFR and ARTTIM23 remain set, even if the IDE
> device lowers INTRQ. This would explain why the CMD646 documentation
> instructs developers to lower them explicitly.

I don't know but sounds plausible that reading the status reg clears irq 
but reading the pci config words that mirrors it won't clear it. But the 
traces you had show that ide_ioport_read was called so driver was likely 
reading status and not the config regs?

I've found some further logs:

https://forums.gentoo.org/viewtopic-t-270357.html
https://www.redhat.com/archives/axp-list/2000-October/msg00070.html
https://www.linuxtopia.org/online_books/linux_beginner_books/debian_linux_desktop_survival_guide/Docking_Station.shtml

These show Linux messages for early CMD646 revisions that had bugs but 
what I've noticed is that they say something about not 100% native mode 
which seems to be similar to what I had with via-ide when it uses IRQ14-15 
even in native mode. Could your problem be similar? Maybe you could search 
for more such logs for Linux booting on Sun Ultra machines and see what 
those say and check how it determines which IRQ number it should use. This 
may depend on some setting that's not emulated correctly.

> The Linux driver code appears to be consistent with the behaviour that
> I'm seeing from Solaris 10.
> 
> The following appears to be used to initialise the CMD646U.
> 
> {	/* CMD 646U with broken UDMA */
> 	.flags = ATA_FLAG_SLAVE_POSS,
> 	.pio_mask = ATA_PIO4,
> 	.mwdma_mask = ATA_MWDMA2,
> 	.port_ops = &cmd646r3_port_ops
> },
> 
> The port operations it uses are defined as so:
> 
> static struct ata_port_operations cmd646r3_port_ops = {
> 	.inherits	= &cmd64x_base_ops,
> 	.sff_irq_check	= cmd648_sff_irq_check,
> 	.sff_irq_clear	= cmd648_sff_irq_clear,
> 	.cable_detect	= ata_cable_40wire,
> }
> 
> As you mention, cmd648_sff_irq_clear clears interrupts explicitly by
> setting bits in MRDMODE - consistent with the CMD646U2 documentation.
> This behaviour is very similar to Solaris 10.

I think this may be to avoid bug with CMD646U.

> > Although if I got 
> > that correctly Linux thinks revisions over 5 are OK and QEMU has 7.
> 
> I'm not sure how revision numbers work with these chips. Do CMD646 and
> CMD646U2 refer to different revisions of the CMD646 chip?

I'm not sure either but from what I've seen so far I think CMD646 either 
refers to the whole family (i.e. all versions) or early versions depending 
on context while there are at least two more newer versions referred to as 
CMD646U and CMD646U2 but probably there are more revisions within these as 
U2 seems to be rev5. QEMU sets the revision field to 7 but I'm not sure 
that's the same Linux checks for. There's some more info on this here:

https://ata.wiki.kernel.org/index.php/Pata_cmd64x

Regards,
BALATON Zoltan

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

[jasper.lowell]

I've since looked at a Ultra 5 board and can confirm that it shipped
with a CMD646U chip like the Ultra 10.

> Since both the generic PCI IDE and CMD646 Linux drivers mention irq
> is 
> cleared on reading status I think using ide_ioport_read in
> hw/ide/pci.c 
> may be correct for the generic case.

For clarity, the Linux drivers mention that for some chips reading CFR
or ARTTIM23 will clear interrupts. Here in
/linux/drivers/ata/pata_cmd64x.c, for instance:

static bool cmd64x_sff_irq_check(struct ata_port *ap)
{
	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
	int irq_mask = ap->port_no ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
	int irq_reg  = ap->port_no ? ARTTIM23 : CFR;
	u8 irq_stat;

	/* NOTE: reading the register should clear the interrupt */
	pci_read_config_byte(pdev, irq_reg, &irq_stat);

	return irq_stat & irq_mask;
}

I might be misunderstanding but isn't this a different side effect than
clearing interrupts from the IDE device when the IDE device status
register is read? This is saying that reading ARTTIM23 or CFR will
clear INTA#.

This code is also only used for the CMD643, CMD646, and CMD646 rev 1 -
none of which I believe QEMU is attempting to emulate. This doesn't
look relevant to us.

> I'm not sure either but from what I've seen so far I think CMD646
> either 
> refers to the whole family (i.e. all versions) or early versions
> depending 
> on context while there are at least two more newer versions referred
> to as 
> CMD646U and CMD646U2 but probably there are more revisions within
> these as 
> U2 seems to be rev5. QEMU sets the revision field to 7 but I'm not
> sure 
> that's the same Linux checks for. There's some more info on this
> here:
>
> https://ata.wiki.kernel.org/index.php/Pata_cmd64x

I've seen CMD646 used to refer to the family as well.

> QEMU sets the revision field to 7

In /linux/drivers/ide/cmd64x.c I found the following comment:

* UltraDMA only supported on PCI646U and PCI646U2, which
* correspond to revisions 0x03, 0x05 and 0x07 respectively.

I guess the PCI646U2 is both revisions 0x05 and 0x07.

According to /linux/drivers/ata/pata_cmd64x.c, interrupt behaviour
doesn't change across the CMD646U, CMD646U2, CMD648, and the CMD649.
These chips set the interrupt bit explicitly and do not have the
comment you highlighted earlier about clearing interrupts when reading
ARTTIM23 or CFR.

> This may explain why
> Linux 
> checks alt status and clears interrupt instead of reading status
> register.

I think Linux does this when there is no PCI IDE controller. The code
in /linux/drivers/ide/ide-io.c acts directly on IDE devices.

> I don't know but sounds plausible that reading the status reg clears
> irq 
> but reading the pci config words that mirrors it won't clear it. But
> the 
> traces you had show that ide_ioport_read was called so driver was
> likely 
> reading status and not the config regs?

When in PCI native mode, ide_ioport_read is called because of the
following code in /qemu/hw/ide/pci.c.

static uint64_t pci_ide_data_read(void *opaque, hwaddr addr, unsigned
size)
{
    IDEBus *bus = opaque;

    if (size == 1) {
        return ide_ioport_read(bus, addr);
    } else if (addr == 0) {
        if (size == 2) {
            return ide_data_readw(bus, addr);
        } else {
            return ide_data_readl(bus, addr);
        }
    }
    return ((uint64_t)1 << (size * 8)) - 1;
}

ide_ioport_read calls qemu_irq_lower when the IDE device status
register is read. This will propagate all the way to the root bus. I
think that when there is a PCI IDE controller inbetween and in PCI
native mode, this is not always propagated past the PCI IDE controller.
In the case of the CMD646U2, I think the lowering of interrupts is only
propagated when the controller is in legacy/compatibility mode.

From what I can tell cmd646_set_irq is only ever called from IDE device
code, ie. ide_ioport_read. If the controller is not supposed to
propagate the lowering of interrupts, I think a possible fix would be
changing cmd646_set_irq to do nothing when the provided level is 0.
Interrupts can still be cleared by writing to CFR, ARTTIM23, and
MRDMODE which leverage cmd646_update_irq instead. This fix is not
suitable if the emulated CMD646 can be used in compatibility mode but I
don't think we have support for that anyways. 

I'll submit a RFC V2 patch with a proposed fix.

Thanks,
Jasper Lowell.

On Wed, 2020-02-26 at 12:07 +0100, BALATON Zoltan wrote:
> On Wed, 26 Feb 2020, jasper.lowell@bt.com wrote:
> > > 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?
> > 
> > This might be it.
> > 
> > The patch I provided is definitely incorrect and deviates from the
> > specification as Mark mentioned earlier. I misunderstood what
> > ide_ioport_read/write were for and haven't been thinking about
> > legacy
> > mode. 
> > 
> > The bug that I believe exists is present when the CMD646 is
> > operating
> > in PCI native mode. Yeah, I think a possible solution might be to
> > avoid
> > using the ioport_read/write functions from the PCI code if they
> > have
> > side effects that assume the device is in legacy mode. I'll have to
> > spend more time reading through the code and documentation.
> 
> Since both the generic PCI IDE and CMD646 Linux drivers mention irq
> is 
> cleared on reading status I think using ide_ioport_read in
> hw/ide/pci.c 
> may be correct for the generic case. Not sure if the CMD646 has some 
> pecularity but maybe the difference in drivers is to avoid bugs not 
> because of CMD646 not clearing irq. The wikipedia page of CMD640:
> 
> https://en.wikipedia.org/wiki/CMD640
> 
> mentions some versions of it has a bug similar to RZ-1000 for which 
> there's a doc referenced that says the problem is that it forgets
> last 
> data word after raising (or clearing?) IRQ and a workaround is to
> avoid 
> checking status until all data transferred. This may explain why
> Linux 
> checks alt status and clears interrupt instead of reading status
> register.
> 
> > According to the CMD646U2 specification:
> > "When an IDE port is in PCI IDE Legacy Mode, the PCI646U2 is
> > compatible
> > with standard ISA IDE. The IDE task file registers are mapped to
> > the
> > standard ISA port addresses, and IDE drive interrupts occur at
> > IRQ14
> > (primary) or IRQ15 (secondary)."
> > 
> > In legacy mode, IRQ14 and IRQ15 mirror the state of INTRQ on each
> > of
> > the selected IDE devices. QEMU appears to emulate this correctly.
> > 
> > In PCI native mode, INTRQ is not mirrored or given a single IRQ.
> > Interrupts are provided by the PCI IDE controller depending on the
> > controller's logic. For instance, an IDE device can raise an
> > interrupt
> > but the CMD646 may not propagate that interrupt if MRDMODE has
> > certain
> > bits set. I'm thinking that maybe the controller does not have
> > logic to
> > unset the interrupt bits in CFR and ARTTIM23 when the IDE device
> > lowers
> > INTRQ. This might mean that the controller will continue to assert
> > an
> > interrupt while bits in CFR and ARTTIM23 remain set, even if the
> > IDE
> > device lowers INTRQ. This would explain why the CMD646
> > documentation
> > instructs developers to lower them explicitly.
> 
> I don't know but sounds plausible that reading the status reg clears
> irq 
> but reading the pci config words that mirrors it won't clear it. But
> the 
> traces you had show that ide_ioport_read was called so driver was
> likely 
> reading status and not the config regs?
> 
> I've found some further logs:
> 
> https://forums.gentoo.org/viewtopic-t-270357.html
> https://www.redhat.com/archives/axp-list/2000-October/msg00070.html
> https://www.linuxtopia.org/online_books/linux_beginner_books/debian_linux_desktop_survival_guide/Docking_Station.shtml
> 
> These show Linux messages for early CMD646 revisions that had bugs
> but 
> what I've noticed is that they say something about not 100% native
> mode 
> which seems to be similar to what I had with via-ide when it uses
> IRQ14-15 
> even in native mode. Could your problem be similar? Maybe you could
> search 
> for more such logs for Linux booting on Sun Ultra machines and see
> what 
> those say and check how it determines which IRQ number it should use.
> This 
> may depend on some setting that's not emulated correctly.
> 
> > The Linux driver code appears to be consistent with the behaviour
> > that
> > I'm seeing from Solaris 10.
> > 
> > The following appears to be used to initialise the CMD646U.
> > 
> > {	/* CMD 646U with broken UDMA */
> > 	.flags = ATA_FLAG_SLAVE_POSS,
> > 	.pio_mask = ATA_PIO4,
> > 	.mwdma_mask = ATA_MWDMA2,
> > 	.port_ops = &cmd646r3_port_ops
> > },
> > 
> > The port operations it uses are defined as so:
> > 
> > static struct ata_port_operations cmd646r3_port_ops = {
> > 	.inherits	= &cmd64x_base_ops,
> > 	.sff_irq_check	= cmd648_sff_irq_check,
> > 	.sff_irq_clear	= cmd648_sff_irq_clear,
> > 	.cable_detect	= ata_cable_40wire,
> > }
> > 
> > As you mention, cmd648_sff_irq_clear clears interrupts explicitly
> > by
> > setting bits in MRDMODE - consistent with the CMD646U2
> > documentation.
> > This behaviour is very similar to Solaris 10.
> 
> I think this may be to avoid bug with CMD646U.
> 
> > > Although if I got 
> > > that correctly Linux thinks revisions over 5 are OK and QEMU has
> > > 7.
> > 
> > I'm not sure how revision numbers work with these chips. Do CMD646
> > and
> > CMD646U2 refer to different revisions of the CMD646 chip?
> 
> I'm not sure either but from what I've seen so far I think CMD646
> either 
> refers to the whole family (i.e. all versions) or early versions
> depending 
> on context while there are at least two more newer versions referred
> to as 
> CMD646U and CMD646U2 but probably there are more revisions within
> these as 
> U2 seems to be rev5. QEMU sets the revision field to 7 but I'm not
> sure 
> that's the same Linux checks for. There's some more info on this
> here:
> 
> https://ata.wiki.kernel.org/index.php/Pata_cmd64x
> 
> Regards,
> BALATON Zoltan
> 

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

[jasper.lowell]

> I'll submit a RFC V2 patch with a proposed fix.

This will have to wait.

Recent commits have caused Solaris 10 to error out of booting much
earlier than previously.

Jasper Lowell.

On Thu, 2020-02-27 at 05:10 +0000, jasper.lowell@bt.com wrote:
> I've since looked at a Ultra 5 board and can confirm that it shipped
> with a CMD646U chip like the Ultra 10.
> 
> > Since both the generic PCI IDE and CMD646 Linux drivers mention irq
> > is 
> > cleared on reading status I think using ide_ioport_read in
> > hw/ide/pci.c 
> > may be correct for the generic case.
> 
> For clarity, the Linux drivers mention that for some chips reading
> CFR
> or ARTTIM23 will clear interrupts. Here in
> /linux/drivers/ata/pata_cmd64x.c, for instance:
> 
> static bool cmd64x_sff_irq_check(struct ata_port *ap)
> {
> 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> 	int irq_mask = ap->port_no ? ARTTIM23_INTR_CH1 : CFR_INTR_CH0;
> 	int irq_reg  = ap->port_no ? ARTTIM23 : CFR;
> 	u8 irq_stat;
> 
> 	/* NOTE: reading the register should clear the interrupt */
> 	pci_read_config_byte(pdev, irq_reg, &irq_stat);
> 
> 	return irq_stat & irq_mask;
> }
> 
> I might be misunderstanding but isn't this a different side effect
> than
> clearing interrupts from the IDE device when the IDE device status
> register is read? This is saying that reading ARTTIM23 or CFR will
> clear INTA#.
> 
> This code is also only used for the CMD643, CMD646, and CMD646 rev 1
> -
> none of which I believe QEMU is attempting to emulate. This doesn't
> look relevant to us.
> 
> > I'm not sure either but from what I've seen so far I think CMD646
> > either 
> > refers to the whole family (i.e. all versions) or early versions
> > depending 
> > on context while there are at least two more newer versions
> > referred
> > to as 
> > CMD646U and CMD646U2 but probably there are more revisions within
> > these as 
> > U2 seems to be rev5. QEMU sets the revision field to 7 but I'm not
> > sure 
> > that's the same Linux checks for. There's some more info on this
> > here:
> > 
> > https://ata.wiki.kernel.org/index.php/Pata_cmd64x
> 
> I've seen CMD646 used to refer to the family as well.
> 
> > QEMU sets the revision field to 7
> 
> In /linux/drivers/ide/cmd64x.c I found the following comment:
> 
> * UltraDMA only supported on PCI646U and PCI646U2, which
> * correspond to revisions 0x03, 0x05 and 0x07 respectively.
> 
> I guess the PCI646U2 is both revisions 0x05 and 0x07.
> 
> According to /linux/drivers/ata/pata_cmd64x.c, interrupt behaviour
> doesn't change across the CMD646U, CMD646U2, CMD648, and the CMD649.
> These chips set the interrupt bit explicitly and do not have the
> comment you highlighted earlier about clearing interrupts when
> reading
> ARTTIM23 or CFR.
> 
> > This may explain why
> > Linux 
> > checks alt status and clears interrupt instead of reading status
> > register.
> 
> I think Linux does this when there is no PCI IDE controller. The code
> in /linux/drivers/ide/ide-io.c acts directly on IDE devices.
> 
> > I don't know but sounds plausible that reading the status reg
> > clears
> > irq 
> > but reading the pci config words that mirrors it won't clear it.
> > But
> > the 
> > traces you had show that ide_ioport_read was called so driver was
> > likely 
> > reading status and not the config regs?
> 
> When in PCI native mode, ide_ioport_read is called because of the
> following code in /qemu/hw/ide/pci.c.
> 
> static uint64_t pci_ide_data_read(void *opaque, hwaddr addr, unsigned
> size)
> {
>     IDEBus *bus = opaque;
> 
>     if (size == 1) {
>         return ide_ioport_read(bus, addr);
>     } else if (addr == 0) {
>         if (size == 2) {
>             return ide_data_readw(bus, addr);
>         } else {
>             return ide_data_readl(bus, addr);
>         }
>     }
>     return ((uint64_t)1 << (size * 8)) - 1;
> }
> 
> ide_ioport_read calls qemu_irq_lower when the IDE device status
> register is read. This will propagate all the way to the root bus. I
> think that when there is a PCI IDE controller inbetween and in PCI
> native mode, this is not always propagated past the PCI IDE
> controller.
> In the case of the CMD646U2, I think the lowering of interrupts is
> only
> propagated when the controller is in legacy/compatibility mode.
> 
> From what I can tell cmd646_set_irq is only ever called from IDE
> device
> code, ie. ide_ioport_read. If the controller is not supposed to
> propagate the lowering of interrupts, I think a possible fix would be
> changing cmd646_set_irq to do nothing when the provided level is 0.
> Interrupts can still be cleared by writing to CFR, ARTTIM23, and
> MRDMODE which leverage cmd646_update_irq instead. This fix is not
> suitable if the emulated CMD646 can be used in compatibility mode but
> I
> don't think we have support for that anyways. 
> 
> I'll submit a RFC V2 patch with a proposed fix.
> 
> Thanks,
> Jasper Lowell.
> 
> On Wed, 2020-02-26 at 12:07 +0100, BALATON Zoltan wrote:
> > On Wed, 26 Feb 2020, jasper.lowell@bt.com wrote:
> > > > 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?
> > > 
> > > This might be it.
> > > 
> > > The patch I provided is definitely incorrect and deviates from
> > > the
> > > specification as Mark mentioned earlier. I misunderstood what
> > > ide_ioport_read/write were for and haven't been thinking about
> > > legacy
> > > mode. 
> > > 
> > > The bug that I believe exists is present when the CMD646 is
> > > operating
> > > in PCI native mode. Yeah, I think a possible solution might be to
> > > avoid
> > > using the ioport_read/write functions from the PCI code if they
> > > have
> > > side effects that assume the device is in legacy mode. I'll have
> > > to
> > > spend more time reading through the code and documentation.
> > 
> > Since both the generic PCI IDE and CMD646 Linux drivers mention irq
> > is 
> > cleared on reading status I think using ide_ioport_read in
> > hw/ide/pci.c 
> > may be correct for the generic case. Not sure if the CMD646 has
> > some 
> > pecularity but maybe the difference in drivers is to avoid bugs
> > not 
> > because of CMD646 not clearing irq. The wikipedia page of CMD640:
> > 
> > https://en.wikipedia.org/wiki/CMD640
> > 
> > mentions some versions of it has a bug similar to RZ-1000 for
> > which 
> > there's a doc referenced that says the problem is that it forgets
> > last 
> > data word after raising (or clearing?) IRQ and a workaround is to
> > avoid 
> > checking status until all data transferred. This may explain why
> > Linux 
> > checks alt status and clears interrupt instead of reading status
> > register.
> > 
> > > According to the CMD646U2 specification:
> > > "When an IDE port is in PCI IDE Legacy Mode, the PCI646U2 is
> > > compatible
> > > with standard ISA IDE. The IDE task file registers are mapped to
> > > the
> > > standard ISA port addresses, and IDE drive interrupts occur at
> > > IRQ14
> > > (primary) or IRQ15 (secondary)."
> > > 
> > > In legacy mode, IRQ14 and IRQ15 mirror the state of INTRQ on each
> > > of
> > > the selected IDE devices. QEMU appears to emulate this correctly.
> > > 
> > > In PCI native mode, INTRQ is not mirrored or given a single IRQ.
> > > Interrupts are provided by the PCI IDE controller depending on
> > > the
> > > controller's logic. For instance, an IDE device can raise an
> > > interrupt
> > > but the CMD646 may not propagate that interrupt if MRDMODE has
> > > certain
> > > bits set. I'm thinking that maybe the controller does not have
> > > logic to
> > > unset the interrupt bits in CFR and ARTTIM23 when the IDE device
> > > lowers
> > > INTRQ. This might mean that the controller will continue to
> > > assert
> > > an
> > > interrupt while bits in CFR and ARTTIM23 remain set, even if the
> > > IDE
> > > device lowers INTRQ. This would explain why the CMD646
> > > documentation
> > > instructs developers to lower them explicitly.
> > 
> > I don't know but sounds plausible that reading the status reg
> > clears
> > irq 
> > but reading the pci config words that mirrors it won't clear it.
> > But
> > the 
> > traces you had show that ide_ioport_read was called so driver was
> > likely 
> > reading status and not the config regs?
> > 
> > I've found some further logs:
> > 
> > https://forums.gentoo.org/viewtopic-t-270357.html
> > https://www.redhat.com/archives/axp-list/2000-October/msg00070.html
> > https://www.linuxtopia.org/online_books/linux_beginner_books/debian_linux_desktop_survival_guide/Docking_Station.shtml
> > 
> > These show Linux messages for early CMD646 revisions that had bugs
> > but 
> > what I've noticed is that they say something about not 100% native
> > mode 
> > which seems to be similar to what I had with via-ide when it uses
> > IRQ14-15 
> > even in native mode. Could your problem be similar? Maybe you could
> > search 
> > for more such logs for Linux booting on Sun Ultra machines and see
> > what 
> > those say and check how it determines which IRQ number it should
> > use.
> > This 
> > may depend on some setting that's not emulated correctly.
> > 
> > > The Linux driver code appears to be consistent with the behaviour
> > > that
> > > I'm seeing from Solaris 10.
> > > 
> > > The following appears to be used to initialise the CMD646U.
> > > 
> > > {	/* CMD 646U with broken UDMA */
> > > 	.flags = ATA_FLAG_SLAVE_POSS,
> > > 	.pio_mask = ATA_PIO4,
> > > 	.mwdma_mask = ATA_MWDMA2,
> > > 	.port_ops = &cmd646r3_port_ops
> > > },
> > > 
> > > The port operations it uses are defined as so:
> > > 
> > > static struct ata_port_operations cmd646r3_port_ops = {
> > > 	.inherits	= &cmd64x_base_ops,
> > > 	.sff_irq_check	= cmd648_sff_irq_check,
> > > 	.sff_irq_clear	= cmd648_sff_irq_clear,
> > > 	.cable_detect	= ata_cable_40wire,
> > > }
> > > 
> > > As you mention, cmd648_sff_irq_clear clears interrupts explicitly
> > > by
> > > setting bits in MRDMODE - consistent with the CMD646U2
> > > documentation.
> > > This behaviour is very similar to Solaris 10.
> > 
> > I think this may be to avoid bug with CMD646U.
> > 
> > > > Although if I got 
> > > > that correctly Linux thinks revisions over 5 are OK and QEMU
> > > > has
> > > > 7.
> > > 
> > > I'm not sure how revision numbers work with these chips. Do
> > > CMD646
> > > and
> > > CMD646U2 refer to different revisions of the CMD646 chip?
> > 
> > I'm not sure either but from what I've seen so far I think CMD646
> > either 
> > refers to the whole family (i.e. all versions) or early versions
> > depending 
> > on context while there are at least two more newer versions
> > referred
> > to as 
> > CMD646U and CMD646U2 but probably there are more revisions within
> > these as 
> > U2 seems to be rev5. QEMU sets the revision field to 7 but I'm not
> > sure 
> > that's the same Linux checks for. There's some more info on this
> > here:
> > 
> > https://ata.wiki.kernel.org/index.php/Pata_cmd64x
> > 
> > Regards,
> > BALATON Zoltan
> > 

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

[BALATON Zoltan]

On Thu, 27 Feb 2020, jasper.lowell@bt.com wrote:
>> I'll submit a RFC V2 patch with a proposed fix.
>
> This will have to wait.
>
> Recent commits have caused Solaris 10 to error out of booting much
> earlier than previously.

Can you bisect which commit broke it? Is it the same that caused slowness 
for arm and ppc? For that one there are patches that should fix it on the 
list.

Regards,
BALATON Zoltan

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

[jasper.lowell]

I haven't.

I did a pull this morning on master and everything seems to be working
again. The problem was likely the same.

Thanks,
Jasper Lowell.

On Thu, 2020-02-27 at 12:35 +0100, BALATON Zoltan wrote:
> On Thu, 27 Feb 2020, jasper.lowell@bt.com wrote:
> > > I'll submit a RFC V2 patch with a proposed fix.
> > 
> > This will have to wait.
> > 
> > Recent commits have caused Solaris 10 to error out of booting much
> > earlier than previously.
> 
> Can you bisect which commit broke it? Is it the same that caused
> slowness 
> for arm and ppc? For that one there are patches that should fix it on
> the 
> list.
> 
> Regards,
> BALATON Zoltan
> 

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

[BALATON Zoltan]

On Thu, 27 Feb 2020, jasper.lowell@bt.com wrote:
> I've since looked at a Ultra 5 board and can confirm that it shipped
> with a CMD646U chip like the Ultra 10.

If you have access to an Ultra 5 maybe you could try testing what it does 
with irqs. That should give the ultimate answer to our guessing. It may 
need patching a Linux driver to log more info and recompile the kernel so 
not sure you have time for that but maybe it would help if you can do it.

Regards,
BALATON Zoltan

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

[jasper.lowell]

I'm happy to do this. It will take a while for me to collect the
results though. I'll chime in once I have the results.

> That should give the ultimate answer to our guessing.
Agreed.

Thanks,
Jasper Lowell.

On Thu, 2020-02-27 at 12:38 +0100, BALATON Zoltan wrote:
> On Thu, 27 Feb 2020, jasper.lowell@bt.com wrote:
> > I've since looked at a Ultra 5 board and can confirm that it
> > shipped
> > with a CMD646U chip like the Ultra 10.
> 
> If you have access to an Ultra 5 maybe you could try testing what it
> does 
> with irqs. That should give the ultimate answer to our guessing. It
> may 
> need patching a Linux driver to log more info and recompile the
> kernel so 
> not sure you have time for that but maybe it would help if you can do
> it.
> 
> Regards,
> BALATON Zoltan

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

[BALATON Zoltan]

Hello,

On Wed, 26 Feb 2020, jasper.lowell@bt.com wrote:
> According to the CMD646U2 specification:
> "When an IDE port is in PCI IDE Legacy Mode, the PCI646U2 is compatible
> with standard ISA IDE. The IDE task file registers are mapped to the
> standard ISA port addresses, and IDE drive interrupts occur at IRQ14
> (primary) or IRQ15 (secondary)."
>
> In legacy mode, IRQ14 and IRQ15 mirror the state of INTRQ on each of
> the selected IDE devices. QEMU appears to emulate this correctly.

So CMD646 also seems to have a legacy mode. I've also seen a CMD PCI0640B 
spec which is proabably a similar chip which says for interrupt handling:

"When DSA1 is pulled low during reset, both IDE ports are in PCI IDE 
Legacy Mode. When DSA1 has no pull-down during reset, each IDE port may 
independently be set to PCI IDE Legacy Mode or Native Mode via the 
Programming Interface Byte (configuration register PROGIF, Index 9h). When 
an IDE port is in PCI IDE Legacy Mode, the PCI-0640B is compatible with 
standard ISA IDE. The IDE task file registers are mapped to the standard 
ISA port addresses, and IDE drive interrupts occur at IRQ14 (primary) or 
IRQ15 (secondary).

When an IDE port is in PCI IDE Native Mode, the IDE task file registers 
may be mapped to non-standard port addresses, and IDE drive interrupts 
occur at PCI INTA. Therefore, if both IDE ports are in PCI IDE Native 
Mode, drive interrupts from both IDE ports are multiplexed into PCI INTA. 
In this case, the interrupt status bits must be polled to determine which 
IDE port generated the interrupt, or whether the interrupt was generated 
by another PCI device sharing INTA on the bus."

This same explanation also appears in CMD646 doc. So what mode is the 
PROG_IF config reg set to and do the interrupts raised match that? 
cmd646_update_irq() only seems to raise PCI interrupt, should it also have 
an option to use INT 14 and 15 in legacy mode similar to what my patch 
does for via-ide?

Additionally Solaris may also get info from the OF device tree so that may 
also have to match the device config.

I'm not sure this helps but I don't have any better idea.

Regards,
BALATON Zoltan

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

[jasper.lowell]

> cmd646_update_irq() only seems to raise PCI interrupt, should it also
> have 
> an option to use INT 14 and 15 in legacy mode similar to what my
> patch 
> does for via-ide?

Looking through /qemu/hw/ide/cmd646.c it doesn't look like QEMU has
support for legacy mode. At the very least, it looks like we default to
PCI native mode:

static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
	...
	pci_conf[PCI_CLASS_PROG] = 0x8f;
	...

To add support for legacy mode it would require changing
cmd646_update_irq() and maybe cmd646_set_irq() so that interrupts are
conditionally raised on IRQ14 and/or IRQ15 when the ports are in legacy
mode.

Thanks,
Jasper Lowell.


On Sun, 2020-03-01 at 19:02 +0100, BALATON Zoltan wrote:
> Hello,
> 
> On Wed, 26 Feb 2020, jasper.lowell@bt.com wrote:
> > According to the CMD646U2 specification:
> > "When an IDE port is in PCI IDE Legacy Mode, the PCI646U2 is
> > compatible
> > with standard ISA IDE. The IDE task file registers are mapped to
> > the
> > standard ISA port addresses, and IDE drive interrupts occur at
> > IRQ14
> > (primary) or IRQ15 (secondary)."
> > 
> > In legacy mode, IRQ14 and IRQ15 mirror the state of INTRQ on each
> > of
> > the selected IDE devices. QEMU appears to emulate this correctly.
> 
> So CMD646 also seems to have a legacy mode. I've also seen a CMD
> PCI0640B 
> spec which is proabably a similar chip which says for interrupt
> handling:
> 
> "When DSA1 is pulled low during reset, both IDE ports are in PCI IDE 
> Legacy Mode. When DSA1 has no pull-down during reset, each IDE port
> may 
> independently be set to PCI IDE Legacy Mode or Native Mode via the 
> Programming Interface Byte (configuration register PROGIF, Index 9h).
> When 
> an IDE port is in PCI IDE Legacy Mode, the PCI-0640B is compatible
> with 
> standard ISA IDE. The IDE task file registers are mapped to the
> standard 
> ISA port addresses, and IDE drive interrupts occur at IRQ14 (primary)
> or 
> IRQ15 (secondary).
> 
> When an IDE port is in PCI IDE Native Mode, the IDE task file
> registers 
> may be mapped to non-standard port addresses, and IDE drive
> interrupts 
> occur at PCI INTA. Therefore, if both IDE ports are in PCI IDE
> Native 
> Mode, drive interrupts from both IDE ports are multiplexed into PCI
> INTA. 
> In this case, the interrupt status bits must be polled to determine
> which 
> IDE port generated the interrupt, or whether the interrupt was
> generated 
> by another PCI device sharing INTA on the bus."
> 
> This same explanation also appears in CMD646 doc. So what mode is
> the 
> PROG_IF config reg set to and do the interrupts raised match that? 
> cmd646_update_irq() only seems to raise PCI interrupt, should it also
> have 
> an option to use INT 14 and 15 in legacy mode similar to what my
> patch 
> does for via-ide?
> 
> Additionally Solaris may also get info from the OF device tree so
> that may 
> also have to match the device config.
> 
> I'm not sure this helps but I don't have any better idea.
> 
> Regards,
> BALATON Zoltan
> 

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

[BALATON Zoltan]

On Wed, 4 Mar 2020, jasper.lowell@bt.com wrote:
>> cmd646_update_irq() only seems to raise PCI interrupt, should it also
>> have
>> an option to use INT 14 and 15 in legacy mode similar to what my
>> patch
>> does for via-ide?
>
> Looking through /qemu/hw/ide/cmd646.c it doesn't look like QEMU has
> support for legacy mode. At the very least, it looks like we default to
> PCI native mode:
>
> static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
> 	...
> 	pci_conf[PCI_CLASS_PROG] = 0x8f;
> 	...
>
> To add support for legacy mode it would require changing
> cmd646_update_irq() and maybe cmd646_set_irq() so that interrupts are
> conditionally raised on IRQ14 and/or IRQ15 when the ports are in legacy
> mode.

Ah yes, same problem as with via-ide. With current ISA IDE and PCI device 
emulation code in QEMU it's not easy to emulate both modes but maybe we 
don't need that either. (So we can avoid changing ISA and PCI QEMU parts 
that are widely used and risk breaking something else by that.) The 
Solaris driver does seem to use native mode because it could find io 
addresses in PCI BARs to talk to the controller just did not get 
interrupts if I got that right. Then maybe PCI interrupts are not routed 
as expected by the driver which could be due to

- difference in PCI bridge emulation,
- where the controller is connected
- or how firmware describes it in device tree
- or how it configures it

compared to real hardware. Unless it also has some pecularity similar to 
pegasos2 with native mode and ISA interrupts. Checking on real hardware 
should reveal these differences so maybe that's the best way to find out. 
As for seeing what the firmware does Sun's OpenBoot is open sourced so 
maybe what that does could be checked but probably not easy to find out in 
Forth.

Here are a few Linux logs I've found that suggest on a Sun Ultra 5/10 IDE 
is on irq 4 for both channels, but not sure how PCI interrupt ends up 
there though. Does that match what QEMU does?

http://gavinduley.org/interests/computing/sundmesg.html
https://lists.debian.org/debian-boot/2004/10/msg00864.html
https://forums.gentoo.org/viewtopic-t-473614-start-0.html

Regards,
BALATON Zoltan

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

[Mark Cave-Ayland]

On 04/03/2020 03:11, jasper.lowell@bt.com wrote:

>> cmd646_update_irq() only seems to raise PCI interrupt, should it also
>> have 
>> an option to use INT 14 and 15 in legacy mode similar to what my
>> patch 
>> does for via-ide?
> 
> Looking through /qemu/hw/ide/cmd646.c it doesn't look like QEMU has
> support for legacy mode. At the very least, it looks like we default to
> PCI native mode:
> 
> static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
> 	...
> 	pci_conf[PCI_CLASS_PROG] = 0x8f;
> 	...
> 
> To add support for legacy mode it would require changing
> cmd646_update_irq() and maybe cmd646_set_irq() so that interrupts are
> conditionally raised on IRQ14 and/or IRQ15 when the ports are in legacy
> mode.

Yes, that's correct. However I'm quite confident from booting other non-Solaris OSs
under qemu-system-sparc64 that PCI native mode is being used, particularly as it is
possible to see the related PCI sabre IRQ routing configuration changes.


ATB,

Mark.

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

[jasper.lowell]

> Yes, that's correct. However I'm quite confident from booting other
> non-Solaris OSs
> under qemu-system-sparc64 that PCI native mode is being used,
> particularly as it is
> possible to see the related PCI sabre IRQ routing configuration
> changes.

Considering that Solaris 10 is accessing CFR and ARTTIM23 I don't think
there is any doubt that it is using the chip in PCI native mode. I
don't think Solaris 10 even has support for legacy mode.

Thanks,
Jasper Lowell.

On Wed, 2020-03-04 at 21:07 +0000, Mark Cave-Ayland wrote:
> On 04/03/2020 03:11, jasper.lowell@bt.com wrote:
> 
> > > cmd646_update_irq() only seems to raise PCI interrupt, should it
> > > also
> > > have 
> > > an option to use INT 14 and 15 in legacy mode similar to what my
> > > patch 
> > > does for via-ide?
> > 
> > Looking through /qemu/hw/ide/cmd646.c it doesn't look like QEMU has
> > support for legacy mode. At the very least, it looks like we
> > default to
> > PCI native mode:
> > 
> > static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
> > 	...
> > 	pci_conf[PCI_CLASS_PROG] = 0x8f;
> > 	...
> > 
> > To add support for legacy mode it would require changing
> > cmd646_update_irq() and maybe cmd646_set_irq() so that interrupts
> > are
> > conditionally raised on IRQ14 and/or IRQ15 when the ports are in
> > legacy
> > mode.
> 
> Yes, that's correct. However I'm quite confident from booting other
> non-Solaris OSs
> under qemu-system-sparc64 that PCI native mode is being used,
> particularly as it is
> possible to see the related PCI sabre IRQ routing configuration
> changes.
> 
> 
> ATB,
> 
> Mark.