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, address@hidden 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
>