| Message ID | 32bb2eab213344151ca342bab5db2cf8c2758fb7.1583017348.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement "non 100% native mode" in via-ide | expand |
On 29/02/2020 23:02, BALATON Zoltan wrote: > Some machines operate in "non 100% native mode" where interrupts are > fixed at legacy IDE interrupts and some guests expect this behaviour > without checking based on knowledge about hardware. Even Linux has > arch specific workarounds for this that are activated on such boards > so this needs to be emulated as well. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/ide/via.c | 60 +++++++++++++++++++++++++++++++++++------ > hw/mips/mips_fulong2e.c | 2 +- > include/hw/ide.h | 3 ++- > 3 files changed, 55 insertions(+), 10 deletions(-) > > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 096de8dba0..17418c5822 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -1,9 +1,10 @@ > /* > - * QEMU IDE Emulation: PCI VIA82C686B support. > + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231) > * > * Copyright (c) 2003 Fabrice Bellard > * Copyright (c) 2006 Openedhand Ltd. > * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com> > + * Copyright (c) 2019-2020 BALATON Zoltan > * > * Permission is hereby granted, free of charge, to any person obtaining a copy > * of this software and associated documentation files (the "Software"), to deal > @@ -25,6 +26,8 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/range.h" > +#include "hw/qdev-properties.h" > #include "hw/pci/pci.h" > #include "migration/vmstate.h" > #include "qemu/module.h" > @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level) > } else { > d->config[0x70 + n * 8] &= ~0x80; > } > - > level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80); > - n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); > - if (n) { > - qemu_set_irq(isa_get_irq(NULL, n), level); > + > + /* > + * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE > + * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode. > + * Some guest drivers expect this, often without checking. > + */ > + if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) || > + PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) { > + qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level); > + } else { > + n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); > + if (n) { > + qemu_set_irq(isa_get_irq(NULL, n), level); > + } > } > } > > +static uint32_t via_ide_config_read(PCIDevice *d, uint32_t address, int len) > +{ > + /* > + * The pegasos2 firmware writes to PCI_INTERRUPT_LINE but on real > + * hardware it's fixed at 14 and won't change. Some guests also expect > + * legacy interrupts, without reading PCI_INTERRUPT_LINE but Linux > + * depends on this to read 14. We set it to 14 in the reset method and > + * also set the wmask to 0 to emulate this but that turns out to be not > + * enough. QEMU resets the PCI bus after this device and > + * pci_do_device_reset() called from pci_device_reset() will zero > + * PCI_INTERRUPT_LINE so this config_read function is to counter that and > + * restore the correct value, otherwise this should not be needed. > + */ > + if (range_covers_byte(address, len, PCI_INTERRUPT_LINE)) { > + pci_set_byte(d->config + PCI_INTERRUPT_LINE, 14); > + } > + return pci_default_read_config(d, address, len); > +} This all seems quite strange: so what you're saying is that the VIA is programmed into PCI native mode, but at least on the Pegasos then it should still be generating interrupts on both the PCI bus and the compatibility IRQs? > static void via_ide_reset(DeviceState *dev) > { > PCIIDEState *d = PCI_IDE(dev); > @@ -169,7 +201,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) > > pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */ > pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0); > - dev->wmask[PCI_INTERRUPT_LINE] = 0xf; > + dev->wmask[PCI_CLASS_PROG] = 5; > + dev->wmask[PCI_INTERRUPT_LINE] = 0; > > memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, > &d->bus[0], "via-ide0-data", 8); > @@ -213,14 +246,23 @@ static void via_ide_exitfn(PCIDevice *dev) > } > } > > -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) > +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn, > + bool legacy_irq) > { > PCIDevice *dev; > > - dev = pci_create_simple(bus, devfn, "via-ide"); > + dev = pci_create(bus, devfn, "via-ide"); > + qdev_prop_set_bit(&dev->qdev, "legacy-irq", legacy_irq); > + qdev_init_nofail(&dev->qdev); > pci_ide_create_devs(dev, hd_table); > } > > +static Property via_ide_properties[] = { > + DEFINE_PROP_BIT("legacy-irq", PCIIDEState, flags, PCI_IDE_LEGACY_IRQ, > + false), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void via_ide_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -229,10 +271,12 @@ static void via_ide_class_init(ObjectClass *klass, void *data) > dc->reset = via_ide_reset; > k->realize = via_ide_realize; > k->exit = via_ide_exitfn; > + k->config_read = via_ide_config_read; > k->vendor_id = PCI_VENDOR_ID_VIA; > k->device_id = PCI_DEVICE_ID_VIA_IDE; > k->revision = 0x06; > k->class_id = PCI_CLASS_STORAGE_IDE; > + device_class_set_props(dc, via_ide_properties); > set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); > } > > diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c > index 4727b1d3a4..150182c62a 100644 > --- a/hw/mips/mips_fulong2e.c > +++ b/hw/mips/mips_fulong2e.c > @@ -257,7 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc, > isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); > > ide_drive_get(hd, ARRAY_SIZE(hd)); > - via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1)); > + via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1), false); > > pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci"); > pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci"); > diff --git a/include/hw/ide.h b/include/hw/ide.h > index d88c5ee695..2a7001ccba 100644 > --- a/include/hw/ide.h > +++ b/include/hw/ide.h > @@ -18,7 +18,8 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux); > -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); > +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn, > + bool legacy_irq); > > /* ide-mmio.c */ > void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1); ATB, Mark.
On 01/03/2020 11:35, Mark Cave-Ayland wrote: > On 29/02/2020 23:02, BALATON Zoltan wrote: > >> Some machines operate in "non 100% native mode" where interrupts are >> fixed at legacy IDE interrupts and some guests expect this behaviour >> without checking based on knowledge about hardware. Even Linux has >> arch specific workarounds for this that are activated on such boards >> so this needs to be emulated as well. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/ide/via.c | 60 +++++++++++++++++++++++++++++++++++------ >> hw/mips/mips_fulong2e.c | 2 +- >> include/hw/ide.h | 3 ++- >> 3 files changed, 55 insertions(+), 10 deletions(-) >> >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index 096de8dba0..17418c5822 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -1,9 +1,10 @@ >> /* >> - * QEMU IDE Emulation: PCI VIA82C686B support. >> + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231) >> * >> * Copyright (c) 2003 Fabrice Bellard >> * Copyright (c) 2006 Openedhand Ltd. >> * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com> >> + * Copyright (c) 2019-2020 BALATON Zoltan >> * >> * Permission is hereby granted, free of charge, to any person obtaining a copy >> * of this software and associated documentation files (the "Software"), to deal >> @@ -25,6 +26,8 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/range.h" >> +#include "hw/qdev-properties.h" >> #include "hw/pci/pci.h" >> #include "migration/vmstate.h" >> #include "qemu/module.h" >> @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level) >> } else { >> d->config[0x70 + n * 8] &= ~0x80; >> } >> - >> level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80); >> - n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); >> - if (n) { >> - qemu_set_irq(isa_get_irq(NULL, n), level); >> + >> + /* >> + * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE >> + * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode. >> + * Some guest drivers expect this, often without checking. >> + */ >> + if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) || >> + PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) { >> + qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level); >> + } else { >> + n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); >> + if (n) { >> + qemu_set_irq(isa_get_irq(NULL, n), level); >> + } >> } >> } The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever raise a native PCI IRQ - comparing with my experience on cmd646, should there not be a pci_set_irq(d, level) at the end? ATB, Mark.
On Sun, 1 Mar 2020, Mark Cave-Ayland wrote: > On 29/02/2020 23:02, BALATON Zoltan wrote: >> Some machines operate in "non 100% native mode" where interrupts are >> fixed at legacy IDE interrupts and some guests expect this behaviour >> without checking based on knowledge about hardware. Even Linux has >> arch specific workarounds for this that are activated on such boards >> so this needs to be emulated as well. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/ide/via.c | 60 +++++++++++++++++++++++++++++++++++------ >> hw/mips/mips_fulong2e.c | 2 +- >> include/hw/ide.h | 3 ++- >> 3 files changed, 55 insertions(+), 10 deletions(-) >> >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index 096de8dba0..17418c5822 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -1,9 +1,10 @@ >> /* >> - * QEMU IDE Emulation: PCI VIA82C686B support. >> + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231) >> * >> * Copyright (c) 2003 Fabrice Bellard >> * Copyright (c) 2006 Openedhand Ltd. >> * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com> >> + * Copyright (c) 2019-2020 BALATON Zoltan >> * >> * Permission is hereby granted, free of charge, to any person obtaining a copy >> * of this software and associated documentation files (the "Software"), to deal >> @@ -25,6 +26,8 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/range.h" >> +#include "hw/qdev-properties.h" >> #include "hw/pci/pci.h" >> #include "migration/vmstate.h" >> #include "qemu/module.h" >> @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level) >> } else { >> d->config[0x70 + n * 8] &= ~0x80; >> } >> - >> level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80); >> - n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); >> - if (n) { >> - qemu_set_irq(isa_get_irq(NULL, n), level); >> + >> + /* >> + * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE >> + * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode. >> + * Some guest drivers expect this, often without checking. >> + */ >> + if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) || >> + PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) { >> + qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level); >> + } else { >> + n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); >> + if (n) { >> + qemu_set_irq(isa_get_irq(NULL, n), level); >> + } >> } >> } >> >> +static uint32_t via_ide_config_read(PCIDevice *d, uint32_t address, int len) >> +{ >> + /* >> + * The pegasos2 firmware writes to PCI_INTERRUPT_LINE but on real >> + * hardware it's fixed at 14 and won't change. Some guests also expect >> + * legacy interrupts, without reading PCI_INTERRUPT_LINE but Linux >> + * depends on this to read 14. We set it to 14 in the reset method and >> + * also set the wmask to 0 to emulate this but that turns out to be not >> + * enough. QEMU resets the PCI bus after this device and >> + * pci_do_device_reset() called from pci_device_reset() will zero >> + * PCI_INTERRUPT_LINE so this config_read function is to counter that and >> + * restore the correct value, otherwise this should not be needed. >> + */ >> + if (range_covers_byte(address, len, PCI_INTERRUPT_LINE)) { >> + pci_set_byte(d->config + PCI_INTERRUPT_LINE, 14); >> + } >> + return pci_default_read_config(d, address, len); >> +} > > This all seems quite strange: so what you're saying is that the VIA is programmed > into PCI native mode, but at least on the Pegasos then it should still be generating > interrupts on both the PCI bus and the compatibility IRQs? In my understanding in this "non 100% native mode" IDE interrupts are using only legacy interrupts 14 and 15 even in native mode and PCI interrupts aren't used at all. Linux has this platform specific code: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353 which says forcing legacy mode but that's not what actually happens. According to docs in legacy mode (which we don't emulate because we can't switch between legacy and native mode) apart from legacy interrupts the io adresses should also be fixed to legacy IDE ports but what I've seen from firmware and Amiga like OSes on pegasos2 they actually use the controller in "half-native" mode where the io addresses are set with the PCI BARs but the irq is still using 14 and 15. Even in true, 100% native mode a PCI interrupt does not seem to be used but instead the PCI_INTERRUPT_LINE config reg selects one of the ISA interrupts which is then used for both ide channels. On pegasos2 the firmware writes 9 to PCI_INTERRUPT_LINE and without this patch Linux detected that as 100% native mode using IRQ 9 for both channels and worked with that but MorphOS and AmigaOS don't check interrupt config reg and just expect interrupts on 14 and 15 despite using PCI BARs to map io and leaving the mode reg in native mode which the firmware also writes there. So this seems to be a kind of half-native mode in which PCI BARs map io registers but interrupts are locked to legacy interrupts and we need to emulate this for MorphOS and AmigaOS at least. Linux could work both ways if regs are set accordingly but when it says "forcing legacy mode" in above fixup func that just seems to activate other compatibility funcs in https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/ide/setup-pci.c?h=v4.14.172#n484 to know that legacy interrupts should be used instead of the one in config reg but PCI BARs are still mapped and io is accessed at those addresses which would not make sense in true legacy mode. I've also got register dumps from a real Pegasos2 which shows PCI_INTERRUPT_LINE set to 14 after booting Linux even if firmware tries to change that to 9 so I think it's fixed on real hardware and guests work if we emulate that in this patch so I think this confirms this matches real hardware at least more or less. This patch fixes interrupt line to 14 but that's not a problem at least for Linux or anything that checks the config reg contents, the only difference is that without legacy-irq set it will use IRQ 14 for both IDE channels that seems to work with the mips_fulong2e which is the only other board using via-ide. All of the above is true for via-ide and the two boards we emulate, I don't know if this applies to CMD646 or that conroller and boards using it have different quirks. Also via-ide is part of a southbridge chip (or several similar southbridges) where connetctions within the chip to IRQs may be peculiar while CMD646 may be a more normal PCI device. Regards, BALATON Zoltan > >> static void via_ide_reset(DeviceState *dev) >> { >> PCIIDEState *d = PCI_IDE(dev); >> @@ -169,7 +201,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) >> >> pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */ >> pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0); >> - dev->wmask[PCI_INTERRUPT_LINE] = 0xf; >> + dev->wmask[PCI_CLASS_PROG] = 5; >> + dev->wmask[PCI_INTERRUPT_LINE] = 0; >> >> memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, >> &d->bus[0], "via-ide0-data", 8); >> @@ -213,14 +246,23 @@ static void via_ide_exitfn(PCIDevice *dev) >> } >> } >> >> -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) >> +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn, >> + bool legacy_irq) >> { >> PCIDevice *dev; >> >> - dev = pci_create_simple(bus, devfn, "via-ide"); >> + dev = pci_create(bus, devfn, "via-ide"); >> + qdev_prop_set_bit(&dev->qdev, "legacy-irq", legacy_irq); >> + qdev_init_nofail(&dev->qdev); >> pci_ide_create_devs(dev, hd_table); >> } >> >> +static Property via_ide_properties[] = { >> + DEFINE_PROP_BIT("legacy-irq", PCIIDEState, flags, PCI_IDE_LEGACY_IRQ, >> + false), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> static void via_ide_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> @@ -229,10 +271,12 @@ static void via_ide_class_init(ObjectClass *klass, void *data) >> dc->reset = via_ide_reset; >> k->realize = via_ide_realize; >> k->exit = via_ide_exitfn; >> + k->config_read = via_ide_config_read; >> k->vendor_id = PCI_VENDOR_ID_VIA; >> k->device_id = PCI_DEVICE_ID_VIA_IDE; >> k->revision = 0x06; >> k->class_id = PCI_CLASS_STORAGE_IDE; >> + device_class_set_props(dc, via_ide_properties); >> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >> } >> >> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c >> index 4727b1d3a4..150182c62a 100644 >> --- a/hw/mips/mips_fulong2e.c >> +++ b/hw/mips/mips_fulong2e.c >> @@ -257,7 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc, >> isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); >> >> ide_drive_get(hd, ARRAY_SIZE(hd)); >> - via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1)); >> + via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1), false); >> >> pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci"); >> pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci"); >> diff --git a/include/hw/ide.h b/include/hw/ide.h >> index d88c5ee695..2a7001ccba 100644 >> --- a/include/hw/ide.h >> +++ b/include/hw/ide.h >> @@ -18,7 +18,8 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); >> PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); >> PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); >> int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux); >> -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); >> +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn, >> + bool legacy_irq); >> >> /* ide-mmio.c */ >> void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1); > > ATB, > > Mark. > >
On Sun, 1 Mar 2020, Mark Cave-Ayland wrote: > On 01/03/2020 11:35, Mark Cave-Ayland wrote: >> On 29/02/2020 23:02, BALATON Zoltan wrote: >> >>> Some machines operate in "non 100% native mode" where interrupts are >>> fixed at legacy IDE interrupts and some guests expect this behaviour >>> without checking based on knowledge about hardware. Even Linux has >>> arch specific workarounds for this that are activated on such boards >>> so this needs to be emulated as well. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> hw/ide/via.c | 60 +++++++++++++++++++++++++++++++++++------ >>> hw/mips/mips_fulong2e.c | 2 +- >>> include/hw/ide.h | 3 ++- >>> 3 files changed, 55 insertions(+), 10 deletions(-) >>> >>> diff --git a/hw/ide/via.c b/hw/ide/via.c >>> index 096de8dba0..17418c5822 100644 >>> --- a/hw/ide/via.c >>> +++ b/hw/ide/via.c >>> @@ -1,9 +1,10 @@ >>> /* >>> - * QEMU IDE Emulation: PCI VIA82C686B support. >>> + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231) >>> * >>> * Copyright (c) 2003 Fabrice Bellard >>> * Copyright (c) 2006 Openedhand Ltd. >>> * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com> >>> + * Copyright (c) 2019-2020 BALATON Zoltan >>> * >>> * Permission is hereby granted, free of charge, to any person obtaining a copy >>> * of this software and associated documentation files (the "Software"), to deal >>> @@ -25,6 +26,8 @@ >>> */ >>> >>> #include "qemu/osdep.h" >>> +#include "qemu/range.h" >>> +#include "hw/qdev-properties.h" >>> #include "hw/pci/pci.h" >>> #include "migration/vmstate.h" >>> #include "qemu/module.h" >>> @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level) >>> } else { >>> d->config[0x70 + n * 8] &= ~0x80; >>> } >>> - >>> level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80); >>> - n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); >>> - if (n) { >>> - qemu_set_irq(isa_get_irq(NULL, n), level); >>> + >>> + /* >>> + * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE >>> + * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode. >>> + * Some guest drivers expect this, often without checking. >>> + */ >>> + if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) || >>> + PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) { >>> + qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level); >>> + } else { >>> + n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); >>> + if (n) { >>> + qemu_set_irq(isa_get_irq(NULL, n), level); >>> + } >>> } >>> } > > The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever > raise a native PCI IRQ - comparing with my experience on cmd646, should there not be > a pci_set_irq(d, level) at the end? According to my tests with several guests it seems the via-ide does not seem to use PCI interrupts as described in the previous reply, only either legacy IRQ14 and 15 or one ISA IRQ line set by a config reg in native mode (except on Pegasos2). This may be due to how it's internally connected in the southbridge chip it's part of or some other platform specific quirk, I'm not sure. The CMD646 may be a more conformant PCI device and use PCI interrupts (unless that also has some kind of legacy mode and similar interconnection with ISA interrupts). If it's really using PCI interrupt and Solaris still does not get expexted IRQ then maybe the routing is not matching real hardware somehow? Maybe it would help finding out what interrupt the Solaris driver is checking to find out why it misses the IRQ. Regards, BALATON Zoltan
On 01/03/2020 16:42, BALATON Zoltan wrote: >> The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever >> raise a native PCI IRQ - comparing with my experience on cmd646, should there not be >> a pci_set_irq(d, level) at the end? > > According to my tests with several guests it seems the via-ide does not seem to use > PCI interrupts as described in the previous reply, only either legacy IRQ14 and 15 or > one ISA IRQ line set by a config reg in native mode (except on Pegasos2). This may be > due to how it's internally connected in the southbridge chip it's part of or some > other platform specific quirk, I'm not sure. I think this is the key part here: how does via-ide switch between legacy and native mode? For CMD646 this is done by setting a bit in PCI configuration space, and I'd expect to see something similar here. It might be that the BIOS sets legacy mode on startup, and unless the OS explicitly switches to native mode then the interrupt routing remains at IRQ 14/15 (or whatever value is in PCI_INTERRUPT_LINE). Is there a datasheet available for the VIA chip to check this? ATB, Mark.
On Sun, 1 Mar 2020, Mark Cave-Ayland wrote: > On 01/03/2020 16:42, BALATON Zoltan wrote: > >>> The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever >>> raise a native PCI IRQ - comparing with my experience on cmd646, should there not be >>> a pci_set_irq(d, level) at the end? >> >> According to my tests with several guests it seems the via-ide does not seem to use >> PCI interrupts as described in the previous reply, only either legacy IRQ14 and 15 or >> one ISA IRQ line set by a config reg in native mode (except on Pegasos2). This may be >> due to how it's internally connected in the southbridge chip it's part of or some >> other platform specific quirk, I'm not sure. > > I think this is the key part here: how does via-ide switch between legacy and native > mode? For CMD646 this is done by setting a bit in PCI configuration space, and I'd > expect to see something similar here. > > It might be that the BIOS sets legacy mode on startup, and unless the OS explicitly > switches to native mode then the interrupt routing remains at IRQ 14/15 (or whatever > value is in PCI_INTERRUPT_LINE). Is there a datasheet available for the VIA chip to > check this? Yes, searching for datasheets for the southbridge chips containing this IDE controller such as VT82C686B and VT8231 might turn up something but the docs are not too detailed regarding IRQ mapping. Switching between modes is done the same way as on CMD646 via bits of the PROG_IF config reg but as it's also noted in Linux fixup func the firmware on pegasos2 does set it up as native mode and also writes to the interrupt line reg to set it to 9, yet it seems to remain set to 14 (this is confirmed by reg dump from Linux on real hardware) and IRQs are expected on 14 and 15 as in legacy mode (shown by expectation of Amiga like OSes, regardless of what's in config regs). Docs say that in legacy mode io addresses should be legacy io ports assigned to IDE, yet firmware and OSes use PCI BARs as in native mode but expect interrupts on legacy IRQ as in legacy mode so I think it's kind of half-native mode. This is what the log shows on real PegasosII (using Debian 5 because later versions seems to have problems with radeon driver so have no display): [ 0.000000] Linux version 2.6.26-2-powerpc (Debian 2.6.26-29) (dannf@debian.org) (gcc version 4.1.3 20080704 (prerelease) (Debian 4.1.2-25)) #1 Sun Mar 4 06:19:00 UTC 2012 [ 0.000000] chrp type = 6 [Genesi Pegasos] [ 1.562960] Fixing VIA IDE, force legacy mode on '0000:00:0c.1' [ 7.203599] VP_IDE: IDE controller (0x1106:0x0571 rev 0x06) at PCI slot 0000:00:0c.1 [ 7.211068] VP_IDE: not 100% native mode: will probe irqs later [ 7.218300] VP_IDE: VIA vt8231 (rev 10) IDE UDMA100 controller on pci0000:00:0c.1 [ 7.225534] ide0: BM-DMA at 0x1060-0x1067 [ 7.232794] ide1: BM-DMA at 0x1068-0x106f [ 7.239927] Probing IDE interface ide0... [here detects devices attached to ide channels] [ 9.013151] ide0 at 0x1040-0x1047,0x104e on irq 14 [ 9.020384] ide1 at 0x1050-0x1057,0x105e on irq 15 lspci says: 0000:00:0c.1 IDE interface: VIA Technologies, Inc. VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master SecP PriP]) Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin ? routed to IRQ 14 Region 0: [virtual] I/O ports at 1040 [size=8] Region 1: [virtual] I/O ports at 104c [size=4] Region 2: [virtual] I/O ports at 1050 [size=8] Region 3: [virtual] I/O ports at 105c [size=4] Region 4: I/O ports at 1060 [size=16] Capabilities: [c0] Power Management version 2 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Kernel driver in use: VIA_IDE Kernel modules: via82cxxx If this was legacy mode io ports should not be what seem to be coming from PCI BARs so either docs are not correct about how legacy mode works or this is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which corresponds to native mode but this is what the Linux fixup function does, firmware sets it to 0x8f which means native mode. Regards, BALATON Zoltan
On Sun, 1 Mar 2020, BALATON Zoltan wrote: > is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a > which corresponds to native mode but this is what the Linux fixup function > does, firmware sets it to 0x8f which means native mode. I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its driver detects legacy interrupts but still uses ioports from PCI BARs. Regards, BALATON Zoltan
On 01/03/2020 18:53, BALATON Zoltan wrote: > On Sun, 1 Mar 2020, BALATON Zoltan wrote: >> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which >> corresponds to native mode but this is what the Linux fixup function does, firmware >> sets it to 0x8f which means native mode. > > I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga > like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its > driver detects legacy interrupts but still uses ioports from PCI BARs. I see. Note that it is also possible to have a prog-if value of 0x80 which is where the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the case for Pegasos, since it would explain why attempts to switch the mode via prog-if are ignored? I don't see the PCI BARs being a problem since native drivers wouldn't touch the memory/IO space enable bits, and the BARs are disabled by default. It could just be that the VIA chipset simply doesn't lock the PCI memory/IO space bits in compatibility mode if an OS does decide to use them and program the BARs as it would in native mode. ATB, Mark.
On Sun, 1 Mar 2020, Mark Cave-Ayland wrote: > On 01/03/2020 18:53, BALATON Zoltan wrote: >> On Sun, 1 Mar 2020, BALATON Zoltan wrote: >>> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which >>> corresponds to native mode but this is what the Linux fixup function does, firmware >>> sets it to 0x8f which means native mode. >> >> I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga >> like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its >> driver detects legacy interrupts but still uses ioports from PCI BARs. > > I see. Note that it is also possible to have a prog-if value of 0x80 which is where > the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the > case for Pegasos, since it would explain why attempts to switch the mode via prog-if > are ignored? I've seen such option in CMD646 docs but couldn't find similar in VT8231. Genesi has published the schematics of Pegasos II (linked from my https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 page) so we could check if you can tell which pin is that. But we get 0x8a in Linux lspci output on real hardware for prog-if which is explained by firmare setting it to 0x8f then Linux fixup function clearing bits 0 and 2 so does not seem it started as 0x80 because then firmware should not be able to set it to 0x8f either. > I don't see the PCI BARs being a problem since native drivers wouldn't touch the > memory/IO space enable bits, and the BARs are disabled by default. It could just be > that the VIA chipset simply doesn't lock the PCI memory/IO space bits in > compatibility mode if an OS does decide to use them and program the BARs as it would > in native mode. I think you mean legacy drivers should not touch BARs. That could also be a way (the docs do say that default values for BARs match legacy ports so it's possible that setting legacy mode resets these and uses them as they were enabled but does not prevent changes) but I don't see how could we implement that in QEMU because we either have legacy ports or PCI IDE in QEMU and BARs are handled by PCI code so to keep those enabled for legacy emulation even if they would be disabled for PCI would need some change in PCI code that's probably not a good idea to touch as a lot of things depend on that. This patch I've come up with is confined to PCI IDE code and the end result is the same for at least the boards and clients we care about so I'd go with this for now. Regards, BALATON Zoltan
On 01/03/2020 21:30, BALATON Zoltan wrote: > On Sun, 1 Mar 2020, Mark Cave-Ayland wrote: >> On 01/03/2020 18:53, BALATON Zoltan wrote: >>> On Sun, 1 Mar 2020, BALATON Zoltan wrote: >>>> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which >>>> corresponds to native mode but this is what the Linux fixup function does, firmware >>>> sets it to 0x8f which means native mode. >>> >>> I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga >>> like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its >>> driver detects legacy interrupts but still uses ioports from PCI BARs. >> >> I see. Note that it is also possible to have a prog-if value of 0x80 which is where >> the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the >> case for Pegasos, since it would explain why attempts to switch the mode via prog-if >> are ignored? > > I've seen such option in CMD646 docs but couldn't find similar in VT8231. Genesi has > published the schematics of Pegasos II (linked from my > https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 page) so we could check if > you can tell which pin is that. But we get 0x8a in Linux lspci output on real > hardware for prog-if which is explained by firmare setting it to 0x8f then Linux > fixup function clearing bits 0 and 2 so does not seem it started as 0x80 because then > firmware should not be able to set it to 0x8f either. I had a quick look at the schematics linked from the page above, and they confirm that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI interrupt pins. So on that basis the best explanation as to what is happening is the comment in the link you provided here: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353 /* Pegasos2 firmware version 20040810 configures the built-in IDE controller * in legacy mode, but sets the PCI registers to PCI native mode. * The chip can only operate in legacy mode, so force the PCI class into legacy * mode as well. The same fixup must be done to the class-code property in * the IDE node /pci@80000000/ide@C,1 */ Given that the DT is wrong, then we should assume that all OSs would have to compensate for this in the same way as Linux, and therefore this should be handled automatically. AFAICT this then only leaves the question: why does the firmware set PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running MorphOS under QEMU. Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE to 14? You should be able to confirm this easily on real hardware using the Forth config-* words on the IDE node and reading the prog-if byte before and after. ATB, Mark.
On Mon, 2 Mar 2020, Mark Cave-Ayland wrote: > On 01/03/2020 21:30, BALATON Zoltan wrote: >> On Sun, 1 Mar 2020, Mark Cave-Ayland wrote: >>> On 01/03/2020 18:53, BALATON Zoltan wrote: >>>> On Sun, 1 Mar 2020, BALATON Zoltan wrote: >>>>> is not legacy mode but "not 100% native mode". The prog-if is set to 0x8a which >>>>> corresponds to native mode but this is what the Linux fixup function does, firmware >>>>> sets it to 0x8f which means native mode. >>>> >>>> I mean, 0x8a legacy mode and 0x8f native mode, I see firmware poking 0x8f and Amiga >>>> like OSes reading that yet expecting legacy interrupts. Linux fixes up prog-if so its >>>> driver detects legacy interrupts but still uses ioports from PCI BARs. >>> >>> I see. Note that it is also possible to have a prog-if value of 0x80 which is where >>> the hardware is locked into legacy mode via a pull-down resistor. Perhaps this is the >>> case for Pegasos, since it would explain why attempts to switch the mode via prog-if >>> are ignored? >> >> I've seen such option in CMD646 docs but couldn't find similar in VT8231. Genesi has >> published the schematics of Pegasos II (linked from my >> https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 page) so we could check if >> you can tell which pin is that. But we get 0x8a in Linux lspci output on real >> hardware for prog-if which is explained by firmare setting it to 0x8f then Linux >> fixup function clearing bits 0 and 2 so does not seem it started as 0x80 because then >> firmware should not be able to set it to 0x8f either. > > I had a quick look at the schematics linked from the page above, and they confirm > that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI > interrupt pins. Where did you see that? What I got from trying to look this up in the schematics was that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and Secondary Channel Interrupt Request in doc) where the interrupt lines of the two IDE ports/channels are connected but how they are routed within the chip after that was not clear. The chip doc says that in native mode the interrupt should be configurable and use a single interrupt for both channels and in legacy mode they use the usual 14 and 15 but this is not what guest drivers expect so I think not how really works on PegasosII. It is true however that connection to PCI interrupts aren't mentioned so it always uses ISA IRQ numbers, it just depends on legacy vs. native mode which line is raised. But that was never really a question for VT8231 and maybe only CMD646 could have such interconnection with PCI interrupts. (Proabable reason is that via-ide is part of a southbridge chip where it has connections to ISA bus while CMD646 is a PCI IDE controller but I could be wrong as my knowledge is limited about these.) > So on that basis the best explanation as to what is happening is the > comment in the link you provided here: > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353 > > /* Pegasos2 firmware version 20040810 configures the built-in IDE controller > * in legacy mode, but sets the PCI registers to PCI native mode. > * The chip can only operate in legacy mode, so force the PCI class into legacy > * mode as well. The same fixup must be done to the class-code property in > * the IDE node /pci@80000000/ide@C,1 > */ I'm not sure that it makes much sense that the firmware configures the chip one way then puts info about a different way in the device tree. There could be bugs but this is not likely. Especially because I see in traces that the firmware does try to configure the device in native mode. These are the first few accesses of the firmware to via-ide: pci_cfg_write via-ide 12:1 @0x9 <- 0xf pci_cfg_write via-ide 12:1 @0x40 <- 0xb pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 pci_cfg_write via-ide 12:1 @0x43 <- 0x35 pci_cfg_write via-ide 12:1 @0x44 <- 0x18 pci_cfg_write via-ide 12:1 @0x45 <- 0x1c pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 pci_cfg_write via-ide 12:1 @0x54 <- 0x14 pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 pci_cfg_read via-ide 12:1 @0xc -> 0x0 pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 pci_cfg_read via-ide 12:1 @0x3c -> 0x10e pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 pci_cfg_read via-ide 12:1 @0x3c -> 0x10e pci_cfg_write via-ide 12:1 @0x3c <- 0x109 The very first write is to turn on native mode, so I think it's not about what the firmware does but something about how hardware is wired on Pegasos II or the VT8231 chip itself that only allows legacy interrupts instead of 100% native mode for IDE. > Given that the DT is wrong, then we should assume that all OSs would have to > compensate for this in the same way as Linux, and therefore this should be handled > automatically. > > AFAICT this then only leaves the question: why does the firmware set > PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running > MorphOS under QEMU. Linux does try to handle both true native mode and half-native mode. It only uses half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup and uses true native mode setup. I don't know what MorphOS does but I think it justs knows that Pegasos2 has this quirk and does not look at the device tree at all. > Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE > to 14? You should be able to confirm this easily on real hardware using the Forth > config-* words on the IDE node and reading the prog-if byte before and after. I don't have direct access to real hardware and would also need to come up with some Forth to verify that but given the above trace that the firmware does before we can enter any Forth we would likely find @0x9 = 0x8f and @0x3c = 0x0e because after booting Linux we see 0x8a and 0x0e and Linux only touches the two mode bits. So I don't know the ultimate reason why it works that way but I'm quite sure that this emulation is close enough and guests are happy with it too. Regards, BALATON Zoltan
On 02/03/2020 21:40, BALATON Zoltan wrote: >> I had a quick look at the schematics linked from the page above, and they confirm >> that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI >> interrupt pins. > > Where did you see that? What I got from trying to look this up in the schematics was > that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and > Secondary Channel Interrupt Request in doc) where the interrupt lines of the two IDE > ports/channels are connected but how they are routed within the chip after that was > not clear. The chip doc says that in native mode the interrupt should be configurable > and use a single interrupt for both channels and in legacy mode they use the usual 14 > and 15 but this is not what guest drivers expect so I think not how really works on > PegasosII. It is true however that connection to PCI interrupts aren't mentioned so > it always uses ISA IRQ numbers, it just depends on legacy vs. native mode which line > is raised. But that was never really a question for VT8231 and maybe only CMD646 > could have such interconnection with PCI interrupts. (Proabable reason is that > via-ide is part of a southbridge chip where it has connections to ISA bus while > CMD646 is a PCI IDE controller but I could be wrong as my knowledge is limited about > these.) Presumably the VIA southbridge has its own internal pair of cascaded 8259s so the IRQ line from the drive is connected to IRQ14/15 as per an typical ISA PC. You can see this in the "Common Hardware Reference Platform: I/O Device Reference" PDF section 4.1. >> So on that basis the best explanation as to what is happening is the >> comment in the link you provided here: >> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353 >> >> >> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller >> * in legacy mode, but sets the PCI registers to PCI native mode. >> * The chip can only operate in legacy mode, so force the PCI class into legacy >> * mode as well. The same fixup must be done to the class-code property in >> * the IDE node /pci@80000000/ide@C,1 >> */ > > I'm not sure that it makes much sense that the firmware configures the chip one way > then puts info about a different way in the device tree. There could be bugs but this > is not likely. Especially because I see in traces that the firmware does try to > configure the device in native mode. These are the first few accesses of the firmware > to via-ide: But that is exactly what is happening! The comment above clearly indicates the firmware incorrectly sets the IDE controller in native mode which is in exact agreement with the trace you provide below. And in fact if you look at https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc hack that was released in order to fix the device tree to boot Linux which alters the class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good enough here). > pci_cfg_write via-ide 12:1 @0x9 <- 0xf > pci_cfg_write via-ide 12:1 @0x40 <- 0xb > pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 > pci_cfg_write via-ide 12:1 @0x43 <- 0x35 > pci_cfg_write via-ide 12:1 @0x44 <- 0x18 > pci_cfg_write via-ide 12:1 @0x45 <- 0x1c > pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 > pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 > pci_cfg_write via-ide 12:1 @0x54 <- 0x14 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 > pci_cfg_read via-ide 12:1 @0xc -> 0x0 > pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 > pci_cfg_read via-ide 12:1 @0x3c -> 0x10e > pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 > pci_cfg_read via-ide 12:1 @0x3c -> 0x10e > pci_cfg_write via-ide 12:1 @0x3c <- 0x109 > > The very first write is to turn on native mode, so I think it's not about what the > firmware does but something about how hardware is wired on Pegasos II or the VT8231 > chip itself that only allows legacy interrupts instead of 100% native mode for IDE. > >> Given that the DT is wrong, then we should assume that all OSs would have to >> compensate for this in the same way as Linux, and therefore this should be handled >> automatically. >> >> AFAICT this then only leaves the question: why does the firmware set >> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running >> MorphOS under QEMU. > > Linux does try to handle both true native mode and half-native mode. It only uses > half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup > and uses true native mode setup. I don't know what MorphOS does but I think it justs > knows that Pegasos2 has this quirk and does not look at the device tree at all. > >> Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE >> to 14? You should be able to confirm this easily on real hardware using the Forth >> config-* words on the IDE node and reading the prog-if byte before and after. > > I don't have direct access to real hardware and would also need to come up with some > Forth to verify that but given the above trace that the firmware does before we can > enter any Forth we would likely find @0x9 = 0x8f and @0x3c = 0x0e because after > booting Linux we see 0x8a and 0x0e and Linux only touches the two mode bits. Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a standard chip, so let's try and figure out exactly what is happening using a real firmware and emulate that behaviour in QEMU. This should then make all guests happy, regardless of architecture, without requiring the introduction of feature bits or risk of introducing other incompatibilities. ATB, Mark.
On Tue, 3 Mar 2020, Mark Cave-Ayland wrote: > On 02/03/2020 21:40, BALATON Zoltan wrote: >>> I had a quick look at the schematics linked from the page above, and they confirm >>> that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to the PCI >>> interrupt pins. >> >> Where did you see that? What I got from trying to look this up in the schematics was >> that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and >> Secondary Channel Interrupt Request in doc) where the interrupt lines of the two IDE >> ports/channels are connected but how they are routed within the chip after that was >> not clear. The chip doc says that in native mode the interrupt should be configurable >> and use a single interrupt for both channels and in legacy mode they use the usual 14 >> and 15 but this is not what guest drivers expect so I think not how really works on >> PegasosII. It is true however that connection to PCI interrupts aren't mentioned so >> it always uses ISA IRQ numbers, it just depends on legacy vs. native mode which line >> is raised. But that was never really a question for VT8231 and maybe only CMD646 >> could have such interconnection with PCI interrupts. (Proabable reason is that >> via-ide is part of a southbridge chip where it has connections to ISA bus while >> CMD646 is a PCI IDE controller but I could be wrong as my knowledge is limited about >> these.) > > Presumably the VIA southbridge has its own internal pair of cascaded 8259s so the IRQ > line from the drive is connected to IRQ14/15 as per an typical ISA PC. You can see > this in the "Common Hardware Reference Platform: I/O Device Reference" PDF section 4.1. Yes the VIA southbridge chip integrates all usual PC devices including PICs so these may have connection internally. I haven't checked these docs before but now I think another chapter in the doc you referenced and its companion "Common Hardware Reference Platform: A System Architecture" may explain the unusal combination of PCI like io regs with legacy interrupts the Pegasos2 seems to have. In the I/O Device Reference, chapter 11 about IDE says that the device must be addressed only with PCI I/O addresses where io addresses are completely relocatable and that when OpenFirmware passes control to the OS IDE must be configured as a PCI device. So this probably means io addresses coming from PCI BARs. But the CHRP System Architecture, chapter 9.2 about ISA devices says that "ISA devices included in a PCI part must route their interrupt signals to the legacy interrupt controller" and this is what the Pegasos2 seems to do. This may be a misinterpretation of the spec because elsewhere (I've lost the exact reference and have no time to find it again) it also says something about native devices must be using OpenPIC but Pegasos2 does not have OpenPIC so that part surely cannot apply anyway. >>> So on that basis the best explanation as to what is happening is the >>> comment in the link you provided here: >>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353 >>> >>> >>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller >>> * in legacy mode, but sets the PCI registers to PCI native mode. >>> * The chip can only operate in legacy mode, so force the PCI class into legacy >>> * mode as well. The same fixup must be done to the class-code property in >>> * the IDE node /pci@80000000/ide@C,1 >>> */ >> >> I'm not sure that it makes much sense that the firmware configures the chip one way >> then puts info about a different way in the device tree. There could be bugs but this >> is not likely. Especially because I see in traces that the firmware does try to >> configure the device in native mode. These are the first few accesses of the firmware >> to via-ide: > > But that is exactly what is happening! The comment above clearly indicates the > firmware incorrectly sets the IDE controller in native mode which is in exact > agreement with the trace you provide below. And in fact if you look at I may be reading the comment wrong but to me that says that "firmware configures IDE in _legacy_ mode" whereas the trace shows it actually configures it in _native_ mode which is complying to the CHRP doc above. But since it cannot comply to the "native devices using OpenPIC" part it probably tries to apply the "ISA devices embedded in PCI" part and locks IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much as possible and the designers decided they will use IRQ14 and 15 for IDE. > https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc > hack that was released in order to fix the device tree to boot Linux which alters the > class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good > enough here). Isn't this the same fixup that newer Linux kernels already include? Maybe this was needed before Linux properly supported Pegasos2 but later kernels will do this anyway. This does not give us any new info we did not have before I think maybe just easier to see all fixups in one place. >> pci_cfg_write via-ide 12:1 @0x9 <- 0xf >> pci_cfg_write via-ide 12:1 @0x40 <- 0xb >> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 >> pci_cfg_write via-ide 12:1 @0x43 <- 0x35 >> pci_cfg_write via-ide 12:1 @0x44 <- 0x18 >> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c >> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 >> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 >> pci_cfg_write via-ide 12:1 @0x54 <- 0x14 >> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 >> pci_cfg_read via-ide 12:1 @0xc -> 0x0 >> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 >> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 >> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >> pci_cfg_write via-ide 12:1 @0x3c <- 0x109 >> >> The very first write is to turn on native mode, so I think it's not about what the >> firmware does but something about how hardware is wired on Pegasos II or the VT8231 >> chip itself that only allows legacy interrupts instead of 100% native mode for IDE. >> >>> Given that the DT is wrong, then we should assume that all OSs would have to >>> compensate for this in the same way as Linux, and therefore this should be handled >>> automatically. >>> >>> AFAICT this then only leaves the question: why does the firmware set >>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running >>> MorphOS under QEMU. >> >> Linux does try to handle both true native mode and half-native mode. It only uses >> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup >> and uses true native mode setup. I don't know what MorphOS does but I think it justs >> knows that Pegasos2 has this quirk and does not look at the device tree at all. >> >>> Could it be that setting prog-if to 0x8a legacy mode also resets PCI_INTERRUPT_LINE >>> to 14? You should be able to confirm this easily on real hardware using the Forth >>> config-* words on the IDE node and reading the prog-if byte before and after. >> >> I don't have direct access to real hardware and would also need to come up with some >> Forth to verify that but given the above trace that the firmware does before we can >> enter any Forth we would likely find @0x9 = 0x8f and @0x3c = 0x0e because after >> booting Linux we see 0x8a and 0x0e and Linux only touches the two mode bits. > > Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a > standard chip, so let's try and figure out exactly what is happening using a real > firmware and emulate that behaviour in QEMU. This should then make all guests happy, > regardless of architecture, without requiring the introduction of feature bits or > risk of introducing other incompatibilities. I think I've already done that with this patch (within the limits possible in QEMU). The Pegasos2 seems to always use IRQ14 and 15 even when IDE is set to native mode (which the firmware does immediately, I'm using a real firmware to test) and all guests are happy with this. This behaviour is confirmed by excpectations of AmigaOS and MorphOS drivers and also Linux comments (although those comments may get the reason wrong, they confirm the behaviour). I'm not sure how real hardware implements this behaviour and also not sure if it's a bug in the firmware or rather a peculiar design choice for Pegasos2. But that probably does not matter for the fact that it's how it works which is all we need to emulate it. Also consider that QEMU via-ide is only emulating native mode of the chip because we can't switch between the two modes so it's either legacy only or native only because all other implemented controllers are either ISA or native PCI so QEMU does not have way to deregister ISA IDE and PCI code does not have way to use io BARs despite not being enabled via PCI config which could be needed to use them when device is in legacy mode. Previously via-ide was emulating legacy only and that worked with Linux but not with anything else. I'm not planning to rewrite large parts of the IDE and PCI code to allow switching back and forth which is not even needed. Unless you know a better way to implement this I think the proposed patch is achieving this with minimal changes. I don't see a need to more exactly emulate some kind of hardware bug or peculiar design choices of a board than what's needed to make clients happy and boot. Is this series good enough now to be merged or are any changes needed? I'd like to not miss the deadline for the freeze and get this delayed for months for not good reason because I'm not sure when will I have time to work on it again. Regards, BALATON Zoltan
On 04/03/2020 00:22, BALATON Zoltan wrote: >>>> So on that basis the best explanation as to what is happening is the >>>> comment in the link you provided here: >>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353 >>>> >>>> >>>> >>>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller >>>> * in legacy mode, but sets the PCI registers to PCI native mode. >>>> * The chip can only operate in legacy mode, so force the PCI class into legacy >>>> * mode as well. The same fixup must be done to the class-code property in >>>> * the IDE node /pci@80000000/ide@C,1 >>>> */ >>> >>> I'm not sure that it makes much sense that the firmware configures the chip one way >>> then puts info about a different way in the device tree. There could be bugs but this >>> is not likely. Especially because I see in traces that the firmware does try to >>> configure the device in native mode. These are the first few accesses of the firmware >>> to via-ide: >> >> But that is exactly what is happening! The comment above clearly indicates the >> firmware incorrectly sets the IDE controller in native mode which is in exact >> agreement with the trace you provide below. And in fact if you look at > > I may be reading the comment wrong but to me that says that "firmware configures IDE > in _legacy_ mode" whereas the trace shows it actually configures it in _native_ mode > which is complying to the CHRP doc above. But since it cannot comply to the "native > devices using OpenPIC" part it probably tries to apply the "ISA devices embedded in > PCI" part and locks IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much > as possible and the designers decided they will use IRQ14 and 15 for IDE. Interesting. My interpretation of the comment was that the hardware can only operate in legacy mode, even though the firmware configures the PCI registers to enable native mode (which is why the class-code and IRQ routing are wrong). >> https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc >> hack that was released in order to fix the device tree to boot Linux which alters the >> class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good >> enough here). > > Isn't this the same fixup that newer Linux kernels already include? Maybe this was > needed before Linux properly supported Pegasos2 but later kernels will do this > anyway. This does not give us any new info we did not have before I think maybe just > easier to see all fixups in one place. > >>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf >>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb >>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 >>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35 >>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18 >>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c >>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 >>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 >>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14 >>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 >>> pci_cfg_read via-ide 12:1 @0xc -> 0x0 >>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 >>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 >>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109 >>> >>> The very first write is to turn on native mode, so I think it's not about what the >>> firmware does but something about how hardware is wired on Pegasos II or the VT8231 >>> chip itself that only allows legacy interrupts instead of 100% native mode for IDE. >>> >>>> Given that the DT is wrong, then we should assume that all OSs would have to >>>> compensate for this in the same way as Linux, and therefore this should be handled >>>> automatically. >>>> >>>> AFAICT this then only leaves the question: why does the firmware set >>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running >>>> MorphOS under QEMU. >>> >>> Linux does try to handle both true native mode and half-native mode. It only uses >>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup >>> and uses true native mode setup. I don't know what MorphOS does but I think it justs >>> knows that Pegasos2 has this quirk and does not look at the device tree at all. I think this is the other way around? From the code above: if (viaide->irq != 14) return; Doesn't this suggest that chrp_pci_fixup_vt8231_ata() exits without applying the fix if it finds PCI_INTERRUPT_LINE set to 9 from the firmware above? Do you have a copy of the full DT and the firmware revision number that was used to generate your Linux boot output on real hardware? >> Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a >> standard chip, so let's try and figure out exactly what is happening using a real >> firmware and emulate that behaviour in QEMU. This should then make all guests happy, >> regardless of architecture, without requiring the introduction of feature bits or >> risk of introducing other incompatibilities. > > I think I've already done that with this patch (within the limits possible in QEMU). > The Pegasos2 seems to always use IRQ14 and 15 even when IDE is set to native mode > (which the firmware does immediately, I'm using a real firmware to test) and all > guests are happy with this. This behaviour is confirmed by excpectations of AmigaOS > and MorphOS drivers and also Linux comments (although those comments may get the > reason wrong, they confirm the behaviour). I'm not sure how real hardware implements > this behaviour and also not sure if it's a bug in the firmware or rather a peculiar > design choice for Pegasos2. But that probably does not matter for the fact that it's > how it works which is all we need to emulate it. > > Also consider that QEMU via-ide is only emulating native mode of the chip because we > can't switch between the two modes so it's either legacy only or native only because > all other implemented controllers are either ISA or native PCI so QEMU does not have > way to deregister ISA IDE and PCI code does not have way to use io BARs despite not > being enabled via PCI config which could be needed to use them when device is in > legacy mode. Previously via-ide was emulating legacy only and that worked with Linux > but not with anything else. I'm not planning to rewrite large parts of the IDE and > PCI code to allow switching back and forth which is not even needed. Unless you know > a better way to implement this I think the proposed patch is achieving this with > minimal changes. I don't see a need to more exactly emulate some kind of hardware bug > or peculiar design choices of a board than what's needed to make clients happy and boot. > > Is this series good enough now to be merged or are any changes needed? I'd like to > not miss the deadline for the freeze and get this delayed for months for not good > reason because I'm not sure when will I have time to work on it again. I think there's still time to get something done before freeze, however I'm not convinced that we understand the actual problem correctly (and also the use of feature bits feels somewhat odd to me). One more thing I don't understand: I had a glance over the logs you posted over at https://osdn.net/projects/qmiga/ticket/38949 and you mention that everything works up to the point where BMDMA is enabled. From memory working with cmd646 both the BMDMA and non-BMDMA interrupts end up calling into the same *_set_irq() function in the emulated controller. So what is the difference between the initial state where IRQs function enough to start to load an OS, and at the point where BMDMA is enabled by the OS driver and things stop working? How does the IRQ routing compare? ATB, Mark.
On Wed, 4 Mar 2020, Mark Cave-Ayland wrote: > On 04/03/2020 00:22, BALATON Zoltan wrote: >>>>> So on that basis the best explanation as to what is happening is the >>>>> comment in the link you provided here: >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353 >>>>> >>>>> /* Pegasos2 firmware version 20040810 configures the built-in IDE controller >>>>> * in legacy mode, but sets the PCI registers to PCI native mode. >>>>> * The chip can only operate in legacy mode, so force the PCI class into legacy >>>>> * mode as well. The same fixup must be done to the class-code property in >>>>> * the IDE node /pci@80000000/ide@C,1 >>>>> */ >>>> >>>> I'm not sure that it makes much sense that the firmware configures the chip one way >>>> then puts info about a different way in the device tree. There could be bugs but this >>>> is not likely. Especially because I see in traces that the firmware does try to >>>> configure the device in native mode. These are the first few accesses of the firmware >>>> to via-ide: >>> >>> But that is exactly what is happening! The comment above clearly indicates the >>> firmware incorrectly sets the IDE controller in native mode which is in exact >>> agreement with the trace you provide below. And in fact if you look at >> >> I may be reading the comment wrong but to me that says that "firmware configures IDE >> in _legacy_ mode" whereas the trace shows it actually configures it in _native_ mode >> which is complying to the CHRP doc above. But since it cannot comply to the "native >> devices using OpenPIC" part it probably tries to apply the "ISA devices embedded in >> PCI" part and locks IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much >> as possible and the designers decided they will use IRQ14 and 15 for IDE. > > Interesting. My interpretation of the comment was that the hardware can only operate > in legacy mode, even though the firmware configures the PCI registers to enable > native mode (which is why the class-code and IRQ routing are wrong). I think you may give more significance to this comment than it really has. I don't know who put it there but maybe was also guessing what really happens and it's not really an authorative answer why it behaves like that. It seems to come from this commit and not from Genesi/bPlan but sounds more like a bugfix from some user: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/powerpc/platforms/chrp/pci.c?h=v3.16.82&id=556ecf9be66f4d493e19bc71a7ce84366d512b71 I give more credibility to what MorphOS expects because the developers of that were closely cooperating with the board designers: https://en.wikipedia.org/wiki/Bplan#Community_support https://en.wikipedia.org/wiki/Pegasos#Operating_system_support >>> https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the nvramrc >>> hack that was released in order to fix the device tree to boot Linux which alters the >>> class-code and sets interrupts to 14 (which I believe is invalid, but seemingly good >>> enough here). >> >> Isn't this the same fixup that newer Linux kernels already include? Maybe this was >> needed before Linux properly supported Pegasos2 but later kernels will do this >> anyway. This does not give us any new info we did not have before I think maybe just >> easier to see all fixups in one place. >> >>>> pci_cfg_write via-ide 12:1 @0x9 <- 0xf >>>> pci_cfg_write via-ide 12:1 @0x40 <- 0xb >>>> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 >>>> pci_cfg_write via-ide 12:1 @0x43 <- 0x35 >>>> pci_cfg_write via-ide 12:1 @0x44 <- 0x18 >>>> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c >>>> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 >>>> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 >>>> pci_cfg_write via-ide 12:1 @0x54 <- 0x14 >>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >>>> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >>>> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 >>>> pci_cfg_read via-ide 12:1 @0xc -> 0x0 >>>> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 >>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >>>> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 >>>> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >>>> pci_cfg_write via-ide 12:1 @0x3c <- 0x109 >>>> >>>> The very first write is to turn on native mode, so I think it's not about what the >>>> firmware does but something about how hardware is wired on Pegasos II or the VT8231 >>>> chip itself that only allows legacy interrupts instead of 100% native mode for IDE. >>>> >>>>> Given that the DT is wrong, then we should assume that all OSs would have to >>>>> compensate for this in the same way as Linux, and therefore this should be handled >>>>> automatically. >>>>> >>>>> AFAICT this then only leaves the question: why does the firmware set >>>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running >>>>> MorphOS under QEMU. >>>> >>>> Linux does try to handle both true native mode and half-native mode. It only uses >>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup >>>> and uses true native mode setup. I don't know what MorphOS does but I think it justs >>>> knows that Pegasos2 has this quirk and does not look at the device tree at all. > > I think this is the other way around? From the code above: > > if (viaide->irq != 14) > return; > > Doesn't this suggest that chrp_pci_fixup_vt8231_ata() exits without applying the fix > if it finds PCI_INTERRUPT_LINE set to 9 from the firmware above? Yes but the fixup is clearing the bits saying the controller is in native mode so code detecting IRQ lines later will use the legacy 14 and 15 because that decides based on this config value. This corresponds to half-native mode, as io addresses are still from BARs not the legacy IDE ports as can be seen from the dmesg output about ide addresses. If irq is not set to 14, fixup function leaves config in native mode and Linux will try to use the single IRQ line set by PCI_INTERRUPT_LINE (which the firmware sets to 9) for both channels. This works with Linux both ways as long as we emulate the same but doesn't with other OSes which always use PCI BARs to access io regs but expect interrupts on 14 and 15 regardless of the mode (and they leave it in native mode don't try setting to legacy) so I think they just know Pegasos2 has this half-native mode and use that without trying to fix up anything. From this I think we know how it works on real hardware and this patch tries to emulate that which also seems to work with all OSes. What else is needed? > Do you have a copy of the full DT and the firmware revision number that was used to > generate your Linux boot output on real hardware? Firmware dumps are also linked from https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2 in the section about firmware and also show mode set to 0x8f by firmware 20040810. >>> Again to summarise: this is a known bug in Pegasos2 firmware and the VIA is a >>> standard chip, so let's try and figure out exactly what is happening using a real >>> firmware and emulate that behaviour in QEMU. This should then make all guests happy, >>> regardless of architecture, without requiring the introduction of feature bits or >>> risk of introducing other incompatibilities. >> >> I think I've already done that with this patch (within the limits possible in QEMU). >> The Pegasos2 seems to always use IRQ14 and 15 even when IDE is set to native mode >> (which the firmware does immediately, I'm using a real firmware to test) and all >> guests are happy with this. This behaviour is confirmed by excpectations of AmigaOS >> and MorphOS drivers and also Linux comments (although those comments may get the >> reason wrong, they confirm the behaviour). I'm not sure how real hardware implements >> this behaviour and also not sure if it's a bug in the firmware or rather a peculiar >> design choice for Pegasos2. But that probably does not matter for the fact that it's >> how it works which is all we need to emulate it. >> >> Also consider that QEMU via-ide is only emulating native mode of the chip because we >> can't switch between the two modes so it's either legacy only or native only because >> all other implemented controllers are either ISA or native PCI so QEMU does not have >> way to deregister ISA IDE and PCI code does not have way to use io BARs despite not >> being enabled via PCI config which could be needed to use them when device is in >> legacy mode. Previously via-ide was emulating legacy only and that worked with Linux >> but not with anything else. I'm not planning to rewrite large parts of the IDE and >> PCI code to allow switching back and forth which is not even needed. Unless you know >> a better way to implement this I think the proposed patch is achieving this with >> minimal changes. I don't see a need to more exactly emulate some kind of hardware bug >> or peculiar design choices of a board than what's needed to make clients happy and boot. >> >> Is this series good enough now to be merged or are any changes needed? I'd like to >> not miss the deadline for the freeze and get this delayed for months for not good >> reason because I'm not sure when will I have time to work on it again. > > I think there's still time to get something done before freeze, however I'm not > convinced that we understand the actual problem correctly (and also the use of > feature bits feels somewhat odd to me). There may be some time until the freeze but not sure I'll have much time until maybe Easter or later so I'd rather get over this now if possible than do it in the last minute. The feature bit was needed because the fulong2e does not seem to have this behaviour (although Linux has a different set of fixups for that board but not about IDE IRQs) so we need to emulate two different behaviours: half-native mode for pegasos2 (which I think is confirmed by evidence in multiple OSes even if we don't know the exact reason behind it) and 100% native mode for fulong2e hence we need a way to tell the device emulation which board's expectations it should fulfil for which introducing a flag seemed to be the simplest solution. > One more thing I don't understand: I had a glance over the logs you posted over at > https://osdn.net/projects/qmiga/ticket/38949 and you mention that everything works up > to the point where BMDMA is enabled. > > From memory working with cmd646 both the BMDMA and non-BMDMA interrupts end up > calling into the same *_set_irq() function in the emulated controller. So what is the > difference between the initial state where IRQs function enough to start to load an > OS, and at the point where BMDMA is enabled by the OS driver and things stop working? > How does the IRQ routing compare? I'm not sure either why that's the case but maybe the firmware does not use interrupts and MorphOS also only enables it when enabling BMDMA. Before that they could just poll the PIO regs? What other OSes are we interested in running on pegasos2 than Linux, MorphOS and AmigaOS? Once these are happy do we need to spend more time on this until an actual problem comes up? I think there's no need to further refine this relatively rarely used device model now. Unless you can propose a better solution that works with these OSes, otherwise that could be addressed in a later patch and the current one is good enough to enable these guests for now. Regards, BALATON Zoltan
On 04/03/2020 22:33, BALATON Zoltan wrote: >>>>>> AFAICT this then only leaves the question: why does the firmware set >>>>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running >>>>>> MorphOS under QEMU. >>>>> >>>>> Linux does try to handle both true native mode and half-native mode. It only uses >>>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup >>>>> and uses true native mode setup. I don't know what MorphOS does but I think it >>>>> justs >>>>> knows that Pegasos2 has this quirk and does not look at the device tree at all. I just a quick look at the PCI specification and found this interesting paragraph in the section about "Interrupt Line": "The Interrupt Line register is an eight-bit register used to communicate interrupt line routing information. The register is read/write and must be implemented by any device (or device function) that uses an interrupt pin. POST software will write the routing information into this register as it initializes and configures the system." "The value in this register tells which input of the system interrupt controller(s) the device's interrupt pin is connected to. The device itself does not use this value, rather it is used by device drivers and operating systems. Device drivers and operating systems can use this information to determine priority and vector information. Values in this register are architecture-specific [43]." [43] For x86 based PCs, the values in this register correspond to IRQ numbers (0-15) of the standard dual 8259 configuration. The value 255 is defined as meaning "unknown" or "no connection" to the interrupt controller. Values between 15 and 254 are reserved. The key part here is "The device itself does not use this value, rather it is used by device drivers and operating systems" since this immediately tells us that the existing code in hw/ide/via.c which uses the interrupt line value for IRQ routing is incorrect and should be removed. If we do that the next question is how does the VIA know whether the use the PCI interrupt or the legacy interrupt? Another look at the datasheet showed that there is another possibility: PCI configuration space register 0x3d (Interrupt pin) is documented as having value 0 == Legacy IRQ routing which should be the initial value on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing. In your previous email you included a trace of the PCI configuration accesses to the via-ide device. Can you try this again with the following diff and post the same output once again? diff --git a/hw/ide/via.c b/hw/ide/via.c index 096de8dba0..db9f4af861 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -139,7 +139,7 @@ static void via_ide_reset(DeviceState *dev) pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170); pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374); pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */ - pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e); + pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e); /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/ pci_set_long(pci_conf + 0x40, 0x0a090600); ATB, Mark.
On Thu, 5 Mar 2020, Mark Cave-Ayland wrote: > On 04/03/2020 22:33, BALATON Zoltan wrote: >>>>>>> AFAICT this then only leaves the question: why does the firmware set >>>>>>> PCI_INTERRUPT_LINE to 9, which is presumably why you are seeing problems running >>>>>>> MorphOS under QEMU. >>>>>> >>>>>> Linux does try to handle both true native mode and half-native mode. It only uses >>>>>> half-native mode if finds IRQ14 on Pegasos, otherwise skips Pegasos specific fixup >>>>>> and uses true native mode setup. I don't know what MorphOS does but I think it >>>>>> justs >>>>>> knows that Pegasos2 has this quirk and does not look at the device tree at all. > > I just a quick look at the PCI specification and found this interesting paragraph in > the section about "Interrupt Line": > > > "The Interrupt Line register is an eight-bit register used to communicate interrupt > line routing information. The register is read/write and must be implemented by any > device (or device function) that uses an interrupt pin. POST software will write the > routing information into this register as it initializes and configures the system." > > "The value in this register tells which input of the system interrupt controller(s) > the device's interrupt pin is connected to. The device itself does not use this > value, rather it is used by device drivers and operating systems. Device drivers and > operating systems can use this information to determine priority and vector > information. Values in this register are architecture-specific [43]." > > [43] For x86 based PCs, the values in this register correspond to IRQ numbers (0-15) > of the standard dual 8259 configuration. The value 255 is defined as meaning > "unknown" or "no connection" to the interrupt controller. Values between 15 and 254 > are reserved. > > > The key part here is "The device itself does not use this value, rather it is used by > device drivers and operating systems" since this immediately tells us that the > existing code in hw/ide/via.c which uses the interrupt line value for IRQ routing is > incorrect and should be removed. On real hardware this may be true but in QEMU how would it otherwise raise the correct interrupt line the guest expects? This probably does not matter for pegasos2 but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ it expects. > If we do that the next question is how does the VIA know whether the use the PCI > interrupt or the legacy interrupt? Another look at the datasheet showed that there is I don't think via-ide ever uses a PCI interrupt, if you look at its datasheet the description of the prog-if reg (0x9) says in native mode irq is programmable via config reg 0x3c which then lists all the ISA IRQs as possible values, default 14 and 0 meaning disable. > another possibility: PCI configuration space register 0x3d (Interrupt pin) is > documented as having value 0 == Legacy IRQ routing which should be the initial value > on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing. The VT8231 docs say this should always read 1 but may be this is somehow set to 0 on the Pegasos2. What does that mean? Should we use this value instead of the feature bit to force using legacy interrupts? We'd still need a property in via-ide to set this reg or is it possible to set it from board code overriding the default after device is created? That would allow to drop patch 1. I can try this. > In your previous email you included a trace of the PCI configuration accesses to the > via-ide device. Can you try this again with the following diff and post the same > output once again? > > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 096de8dba0..db9f4af861 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -139,7 +139,7 @@ static void via_ide_reset(DeviceState *dev) > pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170); > pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374); > pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */ > - pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e); > + pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e); > > /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/ > pci_set_long(pci_conf + 0x40, 0x0a090600); This does not change much: pci_cfg_write via-ide 12:1 @0x9 <- 0xf pci_cfg_write via-ide 12:1 @0x40 <- 0xb pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 pci_cfg_write via-ide 12:1 @0x43 <- 0x35 pci_cfg_write via-ide 12:1 @0x44 <- 0x18 pci_cfg_write via-ide 12:1 @0x45 <- 0x1c pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 pci_cfg_write via-ide 12:1 @0x54 <- 0x14 pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 pci_cfg_read via-ide 12:1 @0xc -> 0x0 pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 pci_cfg_read via-ide 12:1 @0x3c -> 0xe pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 pci_cfg_read via-ide 12:1 @0x3c -> 0xe pci_cfg_write via-ide 12:1 @0x3c <- 0x9 compared to > pci_cfg_write via-ide 12:1 @0x9 <- 0xf > pci_cfg_write via-ide 12:1 @0x40 <- 0xb > pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 > pci_cfg_write via-ide 12:1 @0x43 <- 0x35 > pci_cfg_write via-ide 12:1 @0x44 <- 0x18 > pci_cfg_write via-ide 12:1 @0x45 <- 0x1c > pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 > pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 > pci_cfg_write via-ide 12:1 @0x54 <- 0x14 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 > pci_cfg_read via-ide 12:1 @0xc -> 0x0 > pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 > pci_cfg_read via-ide 12:1 @0x3c -> 0x10e > pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 > pci_cfg_read via-ide 12:1 @0x3c -> 0x10e > pci_cfg_write via-ide 12:1 @0x3c <- 0x109 firmware does not seem to care about this value. Regards, BALATON Zoltan
On Fri, 6 Mar 2020, BALATON Zoltan wrote: > On Thu, 5 Mar 2020, Mark Cave-Ayland wrote: >> On 04/03/2020 22:33, BALATON Zoltan wrote: >> another possibility: PCI configuration space register 0x3d (Interrupt pin) >> is >> documented as having value 0 == Legacy IRQ routing which should be the >> initial value >> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ >> routing. > > The VT8231 docs say this should always read 1 but may be this is somehow set > to 0 on the Pegasos2. What does that mean? Should we use this value instead > of the feature bit to force using legacy interrupts? We'd still need a > property in via-ide to set this reg or is it possible to set it from board > code overriding the default after device is created? That would allow to drop > patch 1. I can try this. This seemed like it could simplify patches a bit but it does not work. Setting this reg to 0 breaks Linux and MorphOS which then think the device does not have an interrupt at all and fail as before waiting for the irq. So we still need the feature bit, cant use this reg to force legacy interrupts. I've spent considerable time testing different OSes before I've ended up with this patch series I've submitted and I could not find a simpler way that works with everything. Regards, BALATON Zoltan >> In your previous email you included a trace of the PCI configuration >> accesses to the >> via-ide device. Can you try this again with the following diff and post the >> same >> output once again? >> >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index 096de8dba0..db9f4af861 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -139,7 +139,7 @@ static void via_ide_reset(DeviceState *dev) >> pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170); >> pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374); >> pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: >> 20-23h */ >> - pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e); >> + pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e); >> >> /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/ >> pci_set_long(pci_conf + 0x40, 0x0a090600); > > This does not change much: > > pci_cfg_write via-ide 12:1 @0x9 <- 0xf > pci_cfg_write via-ide 12:1 @0x40 <- 0xb > pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 > pci_cfg_write via-ide 12:1 @0x43 <- 0x35 > pci_cfg_write via-ide 12:1 @0x44 <- 0x18 > pci_cfg_write via-ide 12:1 @0x45 <- 0x1c > pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 > pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 > pci_cfg_write via-ide 12:1 @0x54 <- 0x14 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 > pci_cfg_read via-ide 12:1 @0xc -> 0x0 > pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 > pci_cfg_read via-ide 12:1 @0x3c -> 0xe > pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 > pci_cfg_read via-ide 12:1 @0x3c -> 0xe > pci_cfg_write via-ide 12:1 @0x3c <- 0x9 > > compared to > >> pci_cfg_write via-ide 12:1 @0x9 <- 0xf >> pci_cfg_write via-ide 12:1 @0x40 <- 0xb >> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 >> pci_cfg_write via-ide 12:1 @0x43 <- 0x35 >> pci_cfg_write via-ide 12:1 @0x44 <- 0x18 >> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c >> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 >> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 >> pci_cfg_write via-ide 12:1 @0x54 <- 0x14 >> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 >> pci_cfg_read via-ide 12:1 @0xc -> 0x0 >> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 >> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 >> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >> pci_cfg_write via-ide 12:1 @0x3c <- 0x109 > > firmware does not seem to care about this value. > > Regards, > BALATON Zoltan >
On 05/03/2020 23:35, BALATON Zoltan wrote: >> I just a quick look at the PCI specification and found this interesting paragraph in >> the section about "Interrupt Line": >> >> >> "The Interrupt Line register is an eight-bit register used to communicate interrupt >> line routing information. The register is read/write and must be implemented by any >> device (or device function) that uses an interrupt pin. POST software will write the >> routing information into this register as it initializes and configures the system." >> >> "The value in this register tells which input of the system interrupt controller(s) >> the device's interrupt pin is connected to. The device itself does not use this >> value, rather it is used by device drivers and operating systems. Device drivers and >> operating systems can use this information to determine priority and vector >> information. Values in this register are architecture-specific [43]." >> >> [43] For x86 based PCs, the values in this register correspond to IRQ numbers (0-15) >> of the standard dual 8259 configuration. The value 255 is defined as meaning >> "unknown" or "no connection" to the interrupt controller. Values between 15 and 254 >> are reserved. >> >> >> The key part here is "The device itself does not use this value, rather it is used by >> device drivers and operating systems" since this immediately tells us that the >> existing code in hw/ide/via.c which uses the interrupt line value for IRQ routing is >> incorrect and should be removed. > > On real hardware this may be true but in QEMU how would it otherwise raise the > correct interrupt line the guest expects? This probably does not matter for pegasos2 > but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ > it expects. That's easy - remember that both the PCI and IRQ interrupts are separate pins on the chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to wire it up to your interrupt controller. >> If we do that the next question is how does the VIA know whether the use the PCI >> interrupt or the legacy interrupt? Another look at the datasheet showed that there is > > I don't think via-ide ever uses a PCI interrupt, if you look at its datasheet the > description of the prog-if reg (0x9) says in native mode irq is programmable via > config reg 0x3c which then lists all the ISA IRQs as possible values, default 14 and > 0 meaning disable. > >> another possibility: PCI configuration space register 0x3d (Interrupt pin) is >> documented as having value 0 == Legacy IRQ routing which should be the initial value >> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing. > > The VT8231 docs say this should always read 1 but may be this is somehow set to 0 on > the Pegasos2. What does that mean? Should we use this value instead of the feature > bit to force using legacy interrupts? We'd still need a property in via-ide to set > this reg or is it possible to set it from board code overriding the default after > device is created? That would allow to drop patch 1. I can try this. Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B datasheets, and there is a difference here. You are correct in what you say above in that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not the case. What is rather unusual here is that both the 8231 and 686B have exactly the same device and vendor ids, so I'm not sure how you'd distinguish between them? >> In your previous email you included a trace of the PCI configuration accesses to the >> via-ide device. Can you try this again with the following diff and post the same >> output once again? >> >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index 096de8dba0..db9f4af861 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -139,7 +139,7 @@ static void via_ide_reset(DeviceState *dev) >> Â Â Â pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x00000170); >> Â Â Â pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x00000374); >> Â Â Â pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0x0000cc01); /* BMIBA: 20-23h */ >> -Â Â Â pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e); >> +Â Â Â pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000000e); >> >> Â Â Â /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/ >> Â Â Â pci_set_long(pci_conf + 0x40, 0x0a090600); > > This does not change much: > > pci_cfg_write via-ide 12:1 @0x9 <- 0xf > pci_cfg_write via-ide 12:1 @0x40 <- 0xb > pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 > pci_cfg_write via-ide 12:1 @0x43 <- 0x35 > pci_cfg_write via-ide 12:1 @0x44 <- 0x18 > pci_cfg_write via-ide 12:1 @0x45 <- 0x1c > pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 > pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 > pci_cfg_write via-ide 12:1 @0x54 <- 0x14 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 > pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 > pci_cfg_read via-ide 12:1 @0xc -> 0x0 > pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 > pci_cfg_read via-ide 12:1 @0x3c -> 0xe > pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 > pci_cfg_read via-ide 12:1 @0x3c -> 0xe > pci_cfg_write via-ide 12:1 @0x3c <- 0x9 > > compared to > >> pci_cfg_write via-ide 12:1 @0x9 <- 0xf >> pci_cfg_write via-ide 12:1 @0x40 <- 0xb >> pci_cfg_write via-ide 12:1 @0x41 <- 0xf2 >> pci_cfg_write via-ide 12:1 @0x43 <- 0x35 >> pci_cfg_write via-ide 12:1 @0x44 <- 0x18 >> pci_cfg_write via-ide 12:1 @0x45 <- 0x1c >> pci_cfg_write via-ide 12:1 @0x46 <- 0xc0 >> pci_cfg_write via-ide 12:1 @0x50 <- 0x17171717 >> pci_cfg_write via-ide 12:1 @0x54 <- 0x14 >> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >> pci_cfg_read via-ide 12:1 @0x0 -> 0x5711106 >> pci_cfg_read via-ide 12:1 @0x8 -> 0x1018f06 >> pci_cfg_read via-ide 12:1 @0xc -> 0x0 >> pci_cfg_read via-ide 12:1 @0x2c -> 0x11001af4 >> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >> pci_cfg_read via-ide 12:1 @0x4 -> 0x2800080 >> pci_cfg_read via-ide 12:1 @0x3c -> 0x10e >> pci_cfg_write via-ide 12:1 @0x3c <- 0x109 > > firmware does not seem to care about this value. I think this proves what I was saying about the legacy interrupt routing, but of course this is based upon the 686B rather than the 8231. ATB, Mark.
On 06/03/2020 00:21, BALATON Zoltan wrote: > On Fri, 6 Mar 2020, BALATON Zoltan wrote: >> On Thu, 5 Mar 2020, Mark Cave-Ayland wrote: >>> On 04/03/2020 22:33, BALATON Zoltan wrote: >>> another possibility: PCI configuration space register 0x3d (Interrupt pin) is >>> documented as having value 0 == Legacy IRQ routing which should be the initial value >>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing. >> >> The VT8231 docs say this should always read 1 but may be this is somehow set to 0 >> on the Pegasos2. What does that mean? Should we use this value instead of the >> feature bit to force using legacy interrupts? We'd still need a property in via-ide >> to set this reg or is it possible to set it from board code overriding the default >> after device is created? That would allow to drop patch 1. I can try this. > > This seemed like it could simplify patches a bit but it does not work. Setting this > reg to 0 breaks Linux and MorphOS which then think the device does not have an > interrupt at all and fail as before waiting for the irq. So we still need the feature > bit, cant use this reg to force legacy interrupts. I've spent considerable time > testing different OSes before I've ended up with this patch series I've submitted and > I could not find a simpler way that works with everything. I appreciate that testing these things can take a lot of time, but what is important thing to ask here is whether these hacks are attempting to work around something in QEMU that doesn't match the hardware specification, and to me it feels that this is what is happening here. Obviously this thread has become quite long (and even I'm struggling to find previous discussions) but here is my summary below: - I don't think the patch in its current form is the right way to do this. Instead of adding a feature bit to fudge the existing IRQ routing when the existing IRQ routing is wrong, let's fix the existing IRQ routing instead. - There is no mention of "non-100%" native mode in the 8231 or 686B datasheet: this is simply a term used within the Linux patches. The controller is either in native mode, or legacy mode. It may be that guests are making use of some undefined behaviour here. - The code that uses the value of PCI_INTERRUPT_LINE in via-ide is incorrect (as your patch comment points out, some guests ignore it anyway). - There is different behaviour here between the 8231 and 686B in this area, despite having the same vendor/device id. The first thing you need to fix is the PCI_INTERRUPT_LINE part; I would start by removing the existing code and instead expose a qdev named gpio "legacy-irq" and wiring that up to your interrupt controller. Note you'd have to do the same for fulong2e, but that is reasonably trivial. ATB, Mark.
On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: > On 05/03/2020 23:35, BALATON Zoltan wrote: >> On real hardware this may be true but in QEMU how would it otherwise raise the >> correct interrupt line the guest expects? This probably does not matter for pegasos2 >> but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ >> it expects. > > That's easy - remember that both the PCI and IRQ interrupts are separate pins on the > chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to > wire it up to your interrupt controller. This "chip" is part of an integrated southbridge/superio/everything chip the also includes the two PICs and how they are internally connected is not known so we would be guessing here anyway. I don't see a need to make it more complicated than it is now by modeling internal pins but how would I wire up gpio to the i8259 model and where should I connect the PCI irq? > Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B > datasheets, and there is a difference here. You are correct in what you say above in > that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not > the case. The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is native mode. Given that we only model native mode of the chip it does not make sense to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and fulong2e even if that may not completely match how it's implemented in hardware. > What is rather unusual here is that both the 8231 and 686B have exactly the same > device and vendor ids, so I'm not sure how you'd distinguish between them? Guests distinguish by looking at the parent device (function 0) which is the chip this IDE device is part of (on function 1). Regards, BALATON Zoltan
Hi guys, On Sun, Mar 1, 2020 at 6:54 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 01/03/2020 16:42, BALATON Zoltan wrote: > > >> The other part I'm not sure about is that I can't see how via_ide_set_irq() can ever > >> raise a native PCI IRQ - comparing with my experience on cmd646, should there not be > >> a pci_set_irq(d, level) at the end? > > > > According to my tests with several guests it seems the via-ide does not seem to use > > PCI interrupts as described in the previous reply, only either legacy IRQ14 and 15 or > > one ISA IRQ line set by a config reg in native mode (except on Pegasos2). This may be > > due to how it's internally connected in the southbridge chip it's part of or some > > other platform specific quirk, I'm not sure. > > I think this is the key part here: how does via-ide switch between legacy and native > mode? For CMD646 this is done by setting a bit in PCI configuration space, and I'd > expect to see something similar here. I haven't read the complete discussion yet, but checked how it's done in OFW. OFW did definitely work on via boards. Surprisingly OFW Switches all IDE boards into native mode the same way: my-space 9 + dup " config-b@" $call-parent 05 or swap " config-b!" $call-parent HTH
On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: > On 06/03/2020 00:21, BALATON Zoltan wrote: >> On Fri, 6 Mar 2020, BALATON Zoltan wrote: >>> On Thu, 5 Mar 2020, Mark Cave-Ayland wrote: >>>> On 04/03/2020 22:33, BALATON Zoltan wrote: >>>> another possibility: PCI configuration space register 0x3d (Interrupt pin) is >>>> documented as having value 0 == Legacy IRQ routing which should be the initial value >>>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing. >>> >>> The VT8231 docs say this should always read 1 but may be this is somehow set to 0 >>> on the Pegasos2. What does that mean? Should we use this value instead of the >>> feature bit to force using legacy interrupts? We'd still need a property in via-ide >>> to set this reg or is it possible to set it from board code overriding the default >>> after device is created? That would allow to drop patch 1. I can try this. >> >> This seemed like it could simplify patches a bit but it does not work. Setting this >> reg to 0 breaks Linux and MorphOS which then think the device does not have an >> interrupt at all and fail as before waiting for the irq. So we still need the feature >> bit, cant use this reg to force legacy interrupts. I've spent considerable time >> testing different OSes before I've ended up with this patch series I've submitted and >> I could not find a simpler way that works with everything. > > I appreciate that testing these things can take a lot of time, but what is important > thing to ask here is whether these hacks are attempting to work around something in > QEMU that doesn't match the hardware specification, and to me it feels that this is > what is happening here. It may be we need to work around some incomplete modelling of devices in QEMU, e.g. we only model the native mode of these IDE interfaces so anything involving legacy mode is out of scope. To also emulate legacy mode we'd need changing common ISA code and maybe PIC code as well. As those parts are also used by other more commonly used machine models I'd avoid breaking those and rather implement it confined to these machines that are not yet finished or complete anyway than try to change all dependent devices that would need even more testing. These "hacks" could be cleaned up later and this would not be the only hack in QEMU, I don't have time to fix everything and it's unreasonable to demand it I think. I'd suggest to take this patch as it is now and if you don't like it you can submit patches that clean it up the way you think is correct or submit an alternative patch now that shows how do you think it can be done in a cleaner way because I don't see it and don't have more time for it now. > Obviously this thread has become quite long (and even I'm struggling to find previous > discussions) but here is my summary below: > > - I don't think the patch in its current form is the right way to do this. Instead of > adding a feature bit to fudge the existing IRQ routing when the existing IRQ routing > is wrong, let's fix the existing IRQ routing instead. I think that would involve changing parts which could break other machines so I'd rather go with a featute bit only affecting pegasos2 and fulonge2 than touch i8259 or ISA emulation basing that on some guess how the real chip may be implemented. Is it possible to implement what you propose without changing common IDE, ISA and PIC emulation only in via-ide and fulong2e code? > - There is no mention of "non-100%" native mode in the 8231 or 686B datasheet: this > is simply a term used within the Linux patches. The controller is either in native > mode, or legacy mode. It may be that guests are making use of some undefined > behaviour here. Yes, this is a Linux term and Linux also uses a feature bit to enable this workaround. If that's good enough for Linux why isn't it good enough for you? > - The code that uses the value of PCI_INTERRUPT_LINE in via-ide is incorrect (as your > patch comment points out, some guests ignore it anyway). You're misunderstanding the comment. The via_ide_config_read function is needed to restore value in interrupt line that common PCI reset code deletes. Linux depends on this value to be the same as on real hardware so this is needed to work around QEMU and Linux pecularities. I've tried using PCI_INTERRUPT_PIN in place of the feature bit but setting that to 0 breaks Linux and MorphOS on pegasos2 because these apparently expect this to be set to 1 corresponding to native mode. (Firmware only sets native mode enable bits in prog-if but datasheet says this reg should be 1 by default and other PCI docs say 0 here means no interrupt used so maybe that's why Linux and MorphOS don't like it.) > - There is different behaviour here between the 8231 and 686B in this area, despite > having the same vendor/device id. > > > The first thing you need to fix is the PCI_INTERRUPT_LINE part; I would start by > removing the existing code and instead expose a qdev named gpio "legacy-irq" and > wiring that up to your interrupt controller. Note you'd have to do the same for > fulong2e, but that is reasonably trivial. Please go ahead and do it but if you don't submit an alternative patch before the freeze I'd ask John and Peter to make a judgement here and tell if my series is acceptable or not in its current form and if it is then please merge it and leave clean ups for subsequent patches. This is blocking my further patches to implement pegasos2 emulation. Regards, BALATON Zoltan
On 06/03/2020 12:06, BALATON Zoltan wrote: > On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: >> On 05/03/2020 23:35, BALATON Zoltan wrote: >>> On real hardware this may be true but in QEMU how would it otherwise raise the >>> correct interrupt line the guest expects? This probably does not matter for pegasos2 >>> but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ >>> it expects. >> >> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the >> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to >> wire it up to your interrupt controller. > > This "chip" is part of an integrated southbridge/superio/everything chip the also > includes the two PICs and how they are internally connected is not known so we would > be guessing here anyway. I don't see a need to make it more complicated than it is > now by modeling internal pins but how would I wire up gpio to the i8259 model and > where should I connect the PCI irq? For now I would say not to worry about the PCI IRQ: the reason for discussing this before was because we believed that if the controller was in native mode it must be using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that the existing via-ide device doesn't currently attempt to use the PCI IRQ in via_ide_set_irq() then we should be good. If someone had a machine somewhere that did use the PCI IRQ then it would need investigation, but since there isn't then I don't see any need to do this now. >> Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B >> datasheets, and there is a difference here. You are correct in what you say above in >> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not >> the case. > > The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is > native mode. Given that we only model native mode of the chip it does not make sense > to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on > pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and > fulong2e even if that may not completely match how it's implemented in hardware. > >> What is rather unusual here is that both the 8231 and 686B have exactly the same >> device and vendor ids, so I'm not sure how you'd distinguish between them? > > Guests distinguish by looking at the parent device (function 0) which is the chip > this IDE device is part of (on function 1). Okay thanks, that's useful to know. I've done a quick grep of the source tree and AFAICT the only IDE controller that tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be fairly easy. In short: 1) Add qemu_irq legacy_irqs[2] into PCIIDEState (You could argue that it should belong in a separate VIAIDEState, however quite a few of the BMDMA controllers in QEMU don't have their own device state and just use PCIIDEState. And whilst via-ide is the only one that currently needs support for legacy IRQs, I think it's good to put it there in case other controllers need it in future) 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a name of "legacy-irq" to it 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple() to pci_create() because the device shouldn't be realised immediately 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the device 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it seems that both drives on MIPS and Pegasos both use IRQ 14). ATB, Mark.
On 06/03/2020 12:40, BALATON Zoltan wrote: > On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: >> On 06/03/2020 00:21, BALATON Zoltan wrote: >>> On Fri, 6 Mar 2020, BALATON Zoltan wrote: >>>> On Thu, 5 Mar 2020, Mark Cave-Ayland wrote: >>>>> On 04/03/2020 22:33, BALATON Zoltan wrote: >>>>> another possibility: PCI configuration space register 0x3d (Interrupt pin) is >>>>> documented as having value 0 == Legacy IRQ routing which should be the initial >>>>> value >>>>> on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing. >>>> >>>> The VT8231 docs say this should always read 1 but may be this is somehow set to 0 >>>> on the Pegasos2. What does that mean? Should we use this value instead of the >>>> feature bit to force using legacy interrupts? We'd still need a property in via-ide >>>> to set this reg or is it possible to set it from board code overriding the default >>>> after device is created? That would allow to drop patch 1. I can try this. >>> >>> This seemed like it could simplify patches a bit but it does not work. Setting this >>> reg to 0 breaks Linux and MorphOS which then think the device does not have an >>> interrupt at all and fail as before waiting for the irq. So we still need the feature >>> bit, cant use this reg to force legacy interrupts. I've spent considerable time >>> testing different OSes before I've ended up with this patch series I've submitted and >>> I could not find a simpler way that works with everything. >> >> I appreciate that testing these things can take a lot of time, but what is important >> thing to ask here is whether these hacks are attempting to work around something in >> QEMU that doesn't match the hardware specification, and to me it feels that this is >> what is happening here. > > It may be we need to work around some incomplete modelling of devices in QEMU, e.g. > we only model the native mode of these IDE interfaces so anything involving legacy > mode is out of scope. To also emulate legacy mode we'd need changing common ISA code > and maybe PIC code as well. As those parts are also used by other more commonly used > machine models I'd avoid breaking those and rather implement it confined to these > machines that are not yet finished or complete anyway than try to change all > dependent devices that would need even more testing. These "hacks" could be cleaned > up later and this would not be the only hack in QEMU, I don't have time to fix > everything and it's unreasonable to demand it I think. I'd suggest to take this patch > as it is now and if you don't like it you can submit patches that clean it up the way > you think is correct or submit an alternative patch now that shows how do you think > it can be done in a cleaner way because I don't see it and don't have more time for > it now. > >> Obviously this thread has become quite long (and even I'm struggling to find previous >> discussions) but here is my summary below: >> >> - I don't think the patch in its current form is the right way to do this. Instead of >> adding a feature bit to fudge the existing IRQ routing when the existing IRQ routing >> is wrong, let's fix the existing IRQ routing instead. > > I think that would involve changing parts which could break other machines so I'd > rather go with a featute bit only affecting pegasos2 and fulonge2 than touch i8259 or > ISA emulation basing that on some guess how the real chip may be implemented. Is it > possible to implement what you propose without changing common IDE, ISA and PIC > emulation only in via-ide and fulong2e code? > >> - There is no mention of "non-100%" native mode in the 8231 or 686B datasheet: this >> is simply a term used within the Linux patches. The controller is either in native >> mode, or legacy mode. It may be that guests are making use of some undefined >> behaviour here. > > Yes, this is a Linux term and Linux also uses a feature bit to enable this > workaround. If that's good enough for Linux why isn't it good enough for you? > >> - The code that uses the value of PCI_INTERRUPT_LINE in via-ide is incorrect (as your >> patch comment points out, some guests ignore it anyway). > > You're misunderstanding the comment. The via_ide_config_read function is needed to > restore value in interrupt line that common PCI reset code deletes. Linux depends on > this value to be the same as on real hardware so this is needed to work around QEMU > and Linux pecularities. > > I've tried using PCI_INTERRUPT_PIN in place of the feature bit but setting that to 0 > breaks Linux and MorphOS on pegasos2 because these apparently expect this to be set > to 1 corresponding to native mode. (Firmware only sets native mode enable bits in > prog-if but datasheet says this reg should be 1 by default and other PCI docs say 0 > here means no interrupt used so maybe that's why Linux and MorphOS don't like it.) > >> - There is different behaviour here between the 8231 and 686B in this area, despite >> having the same vendor/device id. >> >> >> The first thing you need to fix is the PCI_INTERRUPT_LINE part; I would start by >> removing the existing code and instead expose a qdev named gpio "legacy-irq" and >> wiring that up to your interrupt controller. Note you'd have to do the same for >> fulong2e, but that is reasonably trivial. > > Please go ahead and do it but if you don't submit an alternative patch before the > freeze I'd ask John and Peter to make a judgement here and tell if my series is > acceptable or not in its current form and if it is then please merge it and leave > clean ups for subsequent patches. This is blocking my further patches to implement > pegasos2 emulation. I believe I've answered this in detail in my previous email, so I suggest we keep the discussion there so it's all in one place. ATB, Mark.
On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: > On 06/03/2020 12:06, BALATON Zoltan wrote: >> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: >>> On 05/03/2020 23:35, BALATON Zoltan wrote: >>>> On real hardware this may be true but in QEMU how would it otherwise raise the >>>> correct interrupt line the guest expects? This probably does not matter for pegasos2 >>>> but I think is needed for 100% native mode used with the fulong2e so it gets the IRQ >>>> it expects. >>> >>> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the >>> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to >>> wire it up to your interrupt controller. >> >> This "chip" is part of an integrated southbridge/superio/everything chip the also >> includes the two PICs and how they are internally connected is not known so we would >> be guessing here anyway. I don't see a need to make it more complicated than it is >> now by modeling internal pins but how would I wire up gpio to the i8259 model and >> where should I connect the PCI irq? > > For now I would say not to worry about the PCI IRQ: the reason for discussing this > before was because we believed that if the controller was in native mode it must be > using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification > we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that > the existing via-ide device doesn't currently attempt to use the PCI IRQ in > via_ide_set_irq() then we should be good. > > If someone had a machine somewhere that did use the PCI IRQ then it would need > investigation, but since there isn't then I don't see any need to do this now. > >>> Okay so this is interesting: I've been switching between the VT8231 and the VT82C686B >>> datasheets, and there is a difference here. You are correct in what you say above in >>> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly not >>> the case. >> >> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is >> native mode. Given that we only model native mode of the chip it does not make sense >> to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on >> pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and >> fulong2e even if that may not completely match how it's implemented in hardware. >> >>> What is rather unusual here is that both the 8231 and 686B have exactly the same >>> device and vendor ids, so I'm not sure how you'd distinguish between them? >> >> Guests distinguish by looking at the parent device (function 0) which is the chip >> this IDE device is part of (on function 1). > > Okay thanks, that's useful to know. > > I've done a quick grep of the source tree and AFAICT the only IDE controller that > tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be > fairly easy. In short: > > 1) Add qemu_irq legacy_irqs[2] into PCIIDEState > > (You could argue that it should belong in a separate VIAIDEState, however quite a few > of the BMDMA controllers in QEMU don't have their own device state and just use > PCIIDEState. And whilst via-ide is the only one that currently needs support for > legacy IRQs, I think it's good to put it there in case other controllers need it in > future) > > 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a > name of "legacy-irq" to it I don't like this. This adds two via-ide specific data to to common PCI IDE code where it does not belong and subclassing it just for this also seems to be more changes than really needed. Reusing the existing CMD646 field and generalising it to allow implementation specific feature flags seems much less intrusive and not less clean than your proposal. > 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple() > to pci_create() because the device shouldn't be realised immediately > > 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect > legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the > device How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259 that I'm not sure how to access cleanly from code instantiating it. Is this better than my patch? It seems it achieves the same via-ide specific behaviour just in a more complicated way and would still need the feature bit to know when to use legacy_irq[1]. > 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do > qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it > seems that both drives on MIPS and Pegasos both use IRQ 14). According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode) the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B datasheet does not detail this but I believe it works the same. Since we currently fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI reset interactions using legacy_irq[0] might work but is not correct, the current way using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion. Before I spend time modifying this patch I'd really like to hear what John as the IDE maintainer prefers for this. John could you chime in and share your views please? Regards, BALATON Zoltan
On 06/03/2020 19:38, BALATON Zoltan wrote: > On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: >> On 06/03/2020 12:06, BALATON Zoltan wrote: >>> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: >>>> On 05/03/2020 23:35, BALATON Zoltan wrote: >>>>> On real hardware this may be true but in QEMU how would it otherwise raise the >>>>> correct interrupt line the guest expects? This probably does not matter for >>>>> pegasos2 >>>>> but I think is needed for 100% native mode used with the fulong2e so it gets the >>>>> IRQ >>>>> it expects. >>>> >>>> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the >>>> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to >>>> wire it up to your interrupt controller. >>> >>> This "chip" is part of an integrated southbridge/superio/everything chip the also >>> includes the two PICs and how they are internally connected is not known so we would >>> be guessing here anyway. I don't see a need to make it more complicated than it is >>> now by modeling internal pins but how would I wire up gpio to the i8259 model and >>> where should I connect the PCI irq? >> >> For now I would say not to worry about the PCI IRQ: the reason for discussing this >> before was because we believed that if the controller was in native mode it must be >> using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification >> we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that >> the existing via-ide device doesn't currently attempt to use the PCI IRQ in >> via_ide_set_irq() then we should be good. >> >> If someone had a machine somewhere that did use the PCI IRQ then it would need >> investigation, but since there isn't then I don't see any need to do this now. >> >>>> Okay so this is interesting: I've been switching between the VT8231 and the >>>> VT82C686B >>>> datasheets, and there is a difference here. You are correct in what you say above in >>>> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly >>>> not >>>> the case. >>> >>> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is >>> native mode. Given that we only model native mode of the chip it does not make sense >>> to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on >>> pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and >>> fulong2e even if that may not completely match how it's implemented in hardware. >>> >>>> What is rather unusual here is that both the 8231 and 686B have exactly the same >>>> device and vendor ids, so I'm not sure how you'd distinguish between them? >>> >>> Guests distinguish by looking at the parent device (function 0) which is the chip >>> this IDE device is part of (on function 1). >> >> Okay thanks, that's useful to know. >> >> I've done a quick grep of the source tree and AFAICT the only IDE controller that >> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be >> fairly easy. In short: >> >> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState >> >> (You could argue that it should belong in a separate VIAIDEState, however quite a few >> of the BMDMA controllers in QEMU don't have their own device state and just use >> PCIIDEState. And whilst via-ide is the only one that currently needs support for >> legacy IRQs, I think it's good to put it there in case other controllers need it in >> future) >> >> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a >> name of "legacy-irq" to it > > I don't like this. This adds two via-ide specific data to to common PCI IDE code > where it does not belong and subclassing it just for this also seems to be more > changes than really needed. Reusing the existing CMD646 field and generalising it to > allow implementation specific feature flags seems much less intrusive and not less > clean than your proposal. It's not VIA-specific though: the ISA legacy and PCI buses have different electrical characteristics and so by definition their signals must be driven by separate physical pins. Have a look at the CMD646 datasheet for example, and you will see that separate pins exist for legacy and PCI native IRQs. >> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple() >> to pci_create() because the device shouldn't be realised immediately >> >> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect >> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the >> device > > How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259 > that I'm not sure how to access cleanly from code instantiating it. Is this better > than my patch? It seems it achieves the same via-ide specific behaviour just in a > more complicated way and would still need the feature bit to know when to use > legacy_irq[1]. We know from the PCI specification that the existing code for via-ide is incorrect, and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in this way then both of these strongly suggest that current via-ide implementation is wrong. Rather than add a hack on top of a hack then the simplest solution is to physically wire the IRQ pin using qdev at the board level, as is done on real hardware. Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should just be a case of returning the nth entry and using that with qdev_init_gpio_out_named(). >> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do >> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it >> seems that both drives on MIPS and Pegasos both use IRQ 14). > > According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode) > the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native > mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B > datasheet does not detail this but I believe it works the same. Since we currently > fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI > reset interactions using legacy_irq[0] might work but is not correct, the current way > using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion. This is not correct though: please re-read my previous email which quotes from the PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be placing a value other than 14 there, but if it does then that suggests the board IRQs are physically wired differently which again should be handled at board level by qdev. ATB, Mark.
On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: > On 06/03/2020 19:38, BALATON Zoltan wrote: >> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: >>> On 06/03/2020 12:06, BALATON Zoltan wrote: >>>> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote: >>>>> On 05/03/2020 23:35, BALATON Zoltan wrote: >>>>>> On real hardware this may be true but in QEMU how would it otherwise raise the >>>>>> correct interrupt line the guest expects? This probably does not matter for >>>>>> pegasos2 >>>>>> but I think is needed for 100% native mode used with the fulong2e so it gets the >>>>>> IRQ >>>>>> it expects. >>>>> >>>>> That's easy - remember that both the PCI and IRQ interrupts are separate pins on the >>>>> chip, so all that needs to be done is expose the legacy IRQ via qdev and use that to >>>>> wire it up to your interrupt controller. >>>> >>>> This "chip" is part of an integrated southbridge/superio/everything chip the also >>>> includes the two PICs and how they are internally connected is not known so we would >>>> be guessing here anyway. I don't see a need to make it more complicated than it is >>>> now by modeling internal pins but how would I wire up gpio to the i8259 model and >>>> where should I connect the PCI irq? >>> >>> For now I would say not to worry about the PCI IRQ: the reason for discussing this >>> before was because we believed that if the controller was in native mode it must be >>> using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the specification >>> we know that PCI_INTERRUPT_LINE is never used by the device itself, and so given that >>> the existing via-ide device doesn't currently attempt to use the PCI IRQ in >>> via_ide_set_irq() then we should be good. >>> >>> If someone had a machine somewhere that did use the PCI IRQ then it would need >>> investigation, but since there isn't then I don't see any need to do this now. >>> >>>>> Okay so this is interesting: I've been switching between the VT8231 and the >>>>> VT82C686B >>>>> datasheets, and there is a difference here. You are correct in what you say above in >>>>> that the 8231 docs specify that this is set to 1, but on the 686B this is clearly >>>>> not >>>>> the case. >>>> >>>> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt routing and 1 is >>>> native mode. Given that we only model native mode of the chip it does not make sense >>>> to set it to anything else than 1 and setting it to 0 confuses MorphOS and Linux on >>>> pegasos2 while setting it to 1 works with everything I've tried both on pegasos2 and >>>> fulong2e even if that may not completely match how it's implemented in hardware. >>>> >>>>> What is rather unusual here is that both the 8231 and 686B have exactly the same >>>>> device and vendor ids, so I'm not sure how you'd distinguish between them? >>>> >>>> Guests distinguish by looking at the parent device (function 0) which is the chip >>>> this IDE device is part of (on function 1). >>> >>> Okay thanks, that's useful to know. >>> >>> I've done a quick grep of the source tree and AFAICT the only IDE controller that >>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be >>> fairly easy. In short: >>> >>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState >>> >>> (You could argue that it should belong in a separate VIAIDEState, however quite a few >>> of the BMDMA controllers in QEMU don't have their own device state and just use >>> PCIIDEState. And whilst via-ide is the only one that currently needs support for >>> legacy IRQs, I think it's good to put it there in case other controllers need it in >>> future) >>> >>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a >>> name of "legacy-irq" to it >> >> I don't like this. This adds two via-ide specific data to to common PCI IDE code >> where it does not belong and subclassing it just for this also seems to be more >> changes than really needed. Reusing the existing CMD646 field and generalising it to >> allow implementation specific feature flags seems much less intrusive and not less >> clean than your proposal. > > It's not VIA-specific though: the ISA legacy and PCI buses have different electrical > characteristics and so by definition their signals must be driven by separate > physical pins. Have a look at the CMD646 datasheet for example, and you will see that > separate pins exist for legacy and PCI native IRQs. For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy mode and thus those pins are not modelled so not needed now. For via-ide we only use ISA interrupts because even if we don't model legacy mode, boards expect ISA interrupts also in native mode maybe because this controller is not a separate PCI device only found embedded in southbridge/superio chips where they connect to the also embedded ISA PICs so even in native mode it should raise one of the ISA IRQs. My patch accesses ISA irqs with isa_get_irq() so no gpios and legacy irqs in PCIIDEState is neeeded and I don't see the need to introduce this complexity here. Also newer PCI ATA and SATA controllers such as sii3112 do not have a legacy mode so I'd keep things related to that out of common PCI IDE code and model it instead in the controllers that have this as this does not seem to belong to PCI IDE. >>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple() >>> to pci_create() because the device shouldn't be realised immediately >>> >>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect >>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the >>> device >> >> How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259 >> that I'm not sure how to access cleanly from code instantiating it. Is this better >> than my patch? It seems it achieves the same via-ide specific behaviour just in a >> more complicated way and would still need the feature bit to know when to use >> legacy_irq[1]. > > We know from the PCI specification that the existing code for via-ide is incorrect, > and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in > this way then both of these strongly suggest that current via-ide implementation is > wrong. Rather than add a hack on top of a hack then the simplest solution is to > physically wire the IRQ pin using qdev at the board level, as is done on real hardware. But it's not done that way on real hardware and via-ide is not a PCI device but all this is internal to the VT8231 chip, the PICs, via-ide and a lot of other things which are modelled in QEMU by reusing existing components but I think we don't want to model the internal of the chip down to the smallest detail. In via-ide's case the PCI_INTERRUPT_LINE is probably not used by the IDE controller function but is used by some other function of the southbridge chip that implements interrupt controller but we don't have a separate model of that in QEMU so we can just emulate this function within via-ide which I think is OK as this IDE controller is part of these southbridges and not used anywhere else. This partly mixes IDE controller function and interrupt controller function but probably OK until we want to model this southbridge in more detail which could be done in later clean up patches. The piix model seems to do the same embedding a 8259 which even less belongs to an IDE controller and acceses irqs array directly. > Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should > just be a case of returning the nth entry and using that with qdev_init_gpio_out_named(). > >>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do >>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it >>> seems that both drives on MIPS and Pegasos both use IRQ 14). >> >> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode) >> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native >> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B >> datasheet does not detail this but I believe it works the same. Since we currently >> fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI >> reset interactions using legacy_irq[0] might work but is not correct, the current way >> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion. > > This is not correct though: please re-read my previous email which quotes from the > PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the > device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be > placing a value other than 14 there, but if it does then that suggests the board IRQs > are physically wired differently which again should be handled at board level by qdev. Correct or not or follows the spec or not this is how it works on real hardware and to get guests running we need to emulate this. Again, this controller is embedded in the 868B and 8231 southbridge chips so they may not completely confirm to PCI spec but their own datasheets. We can argue in how much detail do we want to model the internals of these chips (which we don't know for sure) but I think this patch is good enough for now and could be refined later or we likely won't be able to finish this before the freeze. Another way to look at it is that this patch gets some guests running and does not break anything as far as I know so is there anything in it that's unacceptable now and needs to be changed for it to be merged? Unless you can propose a way to achieve the same in a simpler way now I think we could go with this and you can submit follow up patches to clean it up as you like later. Regards, BALATON Zoltan
On 06/03/2020 22:59, BALATON Zoltan wrote: >>>> I've done a quick grep of the source tree and AFAICT the only IDE controller that >>>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be >>>> fairly easy. In short: >>>> >>>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState >>>> >>>> (You could argue that it should belong in a separate VIAIDEState, however quite a >>>> few >>>> of the BMDMA controllers in QEMU don't have their own device state and just use >>>> PCIIDEState. And whilst via-ide is the only one that currently needs support for >>>> legacy IRQs, I think it's good to put it there in case other controllers need it in >>>> future) >>>> >>>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a >>>> name of "legacy-irq" to it >>> >>> I don't like this. This adds two via-ide specific data to to common PCI IDE code >>> where it does not belong and subclassing it just for this also seems to be more >>> changes than really needed. Reusing the existing CMD646 field and generalising it to >>> allow implementation specific feature flags seems much less intrusive and not less >>> clean than your proposal. >> It's not VIA-specific though: the ISA legacy and PCI buses have different electrical >> characteristics and so by definition their signals must be driven by separate >> physical pins. Have a look at the CMD646 datasheet for example, and you will see that >> separate pins exist for legacy and PCI native IRQs. > > For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy mode and thus > those pins are not modelled so not needed now. Correct. It seems to me that having it there in PCIIDEState provides a convenient place for all controllers that support legacy mode to store the IRQ in case someone would like to add legacy support to the device at a later date. If you really object to this then as I mentioned above, you'll need to add a VIAIDEState or similar and have the legacy IRQs there for the qdev gpio connector. > For via-ide we only use ISA interrupts > because even if we don't model legacy mode, boards expect ISA interrupts also in > native mode maybe because this controller is not a separate PCI device only found > embedded in southbridge/superio chips where they connect to the also embedded ISA > PICs so even in native mode it should raise one of the ISA IRQs. I certainly agree with your analysis here, and that is borne out by the fact that you are able to boot your OSs using your current hack with no PCI interrupts. > My patch accesses > ISA irqs with isa_get_irq() so no gpios and legacy irqs in PCIIDEState is neeeded and > I don't see the need to introduce this complexity here. This isn't complexity though, it is just normal qdev usage. In fact if you look at isa_get_irq() it contains this large comment above the function itself: /* * isa_get_irq() returns the corresponding qemu_irq entry for the i8259. * * This function is only for special cases such as the 'ferr', and * temporary use for normal devices until they are converted to qdev. */ It was a temporary hack to allow old devices to access the 8259 IRQs before devices were converted to qdev. And via-ide is a qdev device, so that's how you need to wire it up. > Also newer PCI ATA and SATA > controllers such as sii3112 do not have a legacy mode so I'd keep things related to > that out of common PCI IDE code and model it instead in the controllers that have > this as this does not seem to belong to PCI IDE. Like I said above: if you really don't want to put the legacy IRQs there then you need to create a VIAIDEState for the qdev gpio connector. >>>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple() >>>> to pci_create() because the device shouldn't be realised immediately >>>> >>>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect >>>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the >>>> device >>> >>> How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259 >>> that I'm not sure how to access cleanly from code instantiating it. Is this better >>> than my patch? It seems it achieves the same via-ide specific behaviour just in a >>> more complicated way and would still need the feature bit to know when to use >>> legacy_irq[1]. >> >> We know from the PCI specification that the existing code for via-ide is incorrect, >> and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in >> this way then both of these strongly suggest that current via-ide implementation is >> wrong. Rather than add a hack on top of a hack then the simplest solution is to >> physically wire the IRQ pin using qdev at the board level, as is done on real >> hardware. > > But it's not done that way on real hardware and via-ide is not a PCI device but all > this is internal to the VT8231 chip, the PICs, via-ide and a lot of other things > which are modelled in QEMU by reusing existing components but I think we don't want > to model the internal of the chip down to the smallest detail. That's not what I'm suggesting though. > In via-ide's case the > PCI_INTERRUPT_LINE is probably not used by the IDE controller function but is used by > some other function of the southbridge chip that implements interrupt controller What evidence do you have for this? > but > we don't have a separate model of that in QEMU so we can just emulate this function > within via-ide which I think is OK as this IDE controller is part of these > southbridges and not used anywhere else. This partly mixes IDE controller function > and interrupt controller function but probably OK until we want to model this > southbridge in more detail which could be done in later clean up patches. The piix > model seems to do the same embedding a 8259 which even less belongs to an IDE > controller and acceses irqs array directly. > >> Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should >> just be a case of returning the nth entry and using that with >> qdev_init_gpio_out_named(). >> >>>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do >>>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it >>>> seems that both drives on MIPS and Pegasos both use IRQ 14). >>> >>> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode) >>> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native >>> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B >>> datasheet does not detail this but I believe it works the same. Since we currently >>> fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI >>> reset interactions using legacy_irq[0] might work but is not correct, the current way >>> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion. >> >> This is not correct though: please re-read my previous email which quotes from the >> PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the >> device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be >> placing a value other than 14 there, but if it does then that suggests the board IRQs >> are physically wired differently which again should be handled at board level by qdev. > > Correct or not or follows the spec or not this is how it works on real hardware and > to get guests running we need to emulate this. This may be a separate issue. As you mentioned in your email most guest OSs on Pegasos blindly ignore the PCI_INTERRUPT_LINE register since they know that the IRQs are hardwired. However you may also be getting caught out by the bug that the current implementation will try and use the PCI_INTERRUPT_LINE IRQ for routing which is incorrect. Both the 8231 and 636B datasheets mark PCI_INTERRUPT_LINE as read-only with a default value of 0xe, so I believe that once you've done the qdev gpio part, keeping this part of your patchset as a separate patch should fix this. > Again, this controller is embedded in > the 868B and 8231 southbridge chips so they may not completely confirm to PCI spec > but their own datasheets. We can argue in how much detail do we want to model the > internals of these chips (which we don't know for sure) but I think this patch is > good enough for now and could be refined later or we likely won't be able to finish > this before the freeze. > > Another way to look at it is that this patch gets some guests running and does not > break anything as far as I know so is there anything in it that's unacceptable now > and needs to be changed for it to be merged? Unless you can propose a way to achieve > the same in a simpler way now I think we could go with this and you can submit follow > up patches to clean it up as you like later. I don't know what else to say except other than what I have already said: both the current via-ide IRQ implementation and the changes in your patchset are incorrect. I've given you the outline of what you need to do to switch the device over to qdev which agrees with the datasheet, the PCI specification, comments within the QEMU code itself and the results of your experiments. The proposed change is also simple to implement (less than an hour's work) using standard APIs. At this time I don't see any point in continuing to repeat myself over and over again: if you can make the qdev change and post an updated patchset then I'm happy to review and continue this discussion, but otherwise I don't see any purpose in continuing this thread. ATB, Mark.
On Sat, 7 Mar 2020, Mark Cave-Ayland wrote: > On 06/03/2020 22:59, BALATON Zoltan wrote: >>>>> I've done a quick grep of the source tree and AFAICT the only IDE controller that >>>>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this should be >>>>> fairly easy. In short: >>>>> >>>>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState >>>>> >>>>> (You could argue that it should belong in a separate VIAIDEState, however quite a >>>>> few >>>>> of the BMDMA controllers in QEMU don't have their own device state and just use >>>>> PCIIDEState. And whilst via-ide is the only one that currently needs support for >>>>> legacy IRQs, I think it's good to put it there in case other controllers need it in >>>>> future) >>>>> >>>>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with a >>>>> name of "legacy-irq" to it >>>> >>>> I don't like this. This adds two via-ide specific data to to common PCI IDE code >>>> where it does not belong and subclassing it just for this also seems to be more >>>> changes than really needed. Reusing the existing CMD646 field and generalising it to >>>> allow implementation specific feature flags seems much less intrusive and not less >>>> clean than your proposal. >>> It's not VIA-specific though: the ISA legacy and PCI buses have different electrical >>> characteristics and so by definition their signals must be driven by separate >>> physical pins. Have a look at the CMD646 datasheet for example, and you will see that >>> separate pins exist for legacy and PCI native IRQs. >> >> For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy mode and thus >> those pins are not modelled so not needed now. > > Correct. It seems to me that having it there in PCIIDEState provides a convenient > place for all controllers that support legacy mode to store the IRQ in case someone > would like to add legacy support to the device at a later date. Why introduce it now and not only when someone would like to add legacy mode? I guess nobody needs legacy mode so we can avoid complexity modelling that unused detail now and only add it when needed. > If you really object to this then as I mentioned above, you'll need to add a > VIAIDEState or similar and have the legacy IRQs there for the qdev gpio connector. > >> For via-ide we only use ISA interrupts >> because even if we don't model legacy mode, boards expect ISA interrupts also in >> native mode maybe because this controller is not a separate PCI device only found >> embedded in southbridge/superio chips where they connect to the also embedded ISA >> PICs so even in native mode it should raise one of the ISA IRQs. > > I certainly agree with your analysis here, and that is borne out by the fact that you > are able to boot your OSs using your current hack with no PCI interrupts. > >> My patch accesses >> ISA irqs with isa_get_irq() so no gpios and legacy irqs in PCIIDEState is neeeded and >> I don't see the need to introduce this complexity here. > > This isn't complexity though, it is just normal qdev usage. In fact if you look at Then qdev is probably overly complex for simple things. > isa_get_irq() it contains this large comment above the function itself: > > /* > * isa_get_irq() returns the corresponding qemu_irq entry for the i8259. > * > * This function is only for special cases such as the 'ferr', and > * temporary use for normal devices until they are converted to qdev. > */ > > It was a temporary hack to allow old devices to access the 8259 IRQs before devices > were converted to qdev. And via-ide is a qdev device, so that's how you need to wire > it up. > >> Also newer PCI ATA and SATA >> controllers such as sii3112 do not have a legacy mode so I'd keep things related to >> that out of common PCI IDE code and model it instead in the controllers that have >> this as this does not seem to belong to PCI IDE. > > Like I said above: if you really don't want to put the legacy IRQs there then you > need to create a VIAIDEState for the qdev gpio connector. I still think what you propose is more complex than my patch and does not achieve any cleaner model. If you object to using isa_get_irq here I can go the piix way and embed the PIC array as well (or set a pointer to that with a property somehow) so I can use isa_irq[n] rather than adding non-existing pins for this. I won't do anything though until IDE and QEMU maintainers indicate what their preference is. >>>>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing pci_create_simple() >>>>> to pci_create() because the device shouldn't be realised immediately >>>>> >>>>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect >>>>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then realise the >>>>> device >>>> >>>> How do I connect gpios to 8259 interrupts? That seems to be internal state of 8259 >>>> that I'm not sure how to access cleanly from code instantiating it. Is this better >>>> than my patch? It seems it achieves the same via-ide specific behaviour just in a >>>> more complicated way and would still need the feature bit to know when to use >>>> legacy_irq[1]. >>> >>> We know from the PCI specification that the existing code for via-ide is incorrect, >>> and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE in >>> this way then both of these strongly suggest that current via-ide implementation is >>> wrong. Rather than add a hack on top of a hack then the simplest solution is to >>> physically wire the IRQ pin using qdev at the board level, as is done on real >>> hardware. >> >> But it's not done that way on real hardware and via-ide is not a PCI device but all >> this is internal to the VT8231 chip, the PICs, via-ide and a lot of other things >> which are modelled in QEMU by reusing existing components but I think we don't want >> to model the internal of the chip down to the smallest detail. > > That's not what I'm suggesting though. I think you did not like emulating the interrupt selection in via-ide that decides which irq to raise (which is really just one if() ) and would like to push it out to board code instead connecting it via gpios. But that's also not quite correct and would not even get rid of the feature bit you also disliked. In fact we don't actually have a qdev model for the VT82C686B where all this probably belongs but I don't plan to make a detailed qdevified model for that now (could be done later in a clean up patch maybe when I'll clean up pegasos2 patches for inclusion) and since via-ide is considered part of that southbridge (and reused in similar VT8231 model) we can include the interrupt controller emulation (this one if() ) as well for now and not worry about PCI specs that don't really apply to a "chip" that's not a standalone PCI IDE controller but part of a bigger southbridge chip. It's just reuses PCI IDE emulation from QEMU where appropriate. >> In via-ide's case the >> PCI_INTERRUPT_LINE is probably not used by the IDE controller function but is used by >> some other function of the southbridge chip that implements interrupt controller > > What evidence do you have for this? The datasheet (of the IDE function) says that PCI_INTERRUPT_LINE selects the ISA interrupt that's raised in native mode for both channels and guests work if we emulate that. Since via-ide is part of the southbridge that also includes the PICs they are somehow connected inside and something selects the interrupt line within the chip but I don't know how exactly that's implemented in hardware. If not the ide part directly as you propose then some other interrupt controller part has to do this and we need to do the same somewhere in our model. Since we don't emulate the southbridge in more detail I've just put it in via-ide which seems to be a good place as it's only used in these southbridges and this could be changed later if via-ide were used elsewhere. No need to be more complex now than that I think. >> but >> we don't have a separate model of that in QEMU so we can just emulate this function >> within via-ide which I think is OK as this IDE controller is part of these >> southbridges and not used anywhere else. This partly mixes IDE controller function >> and interrupt controller function but probably OK until we want to model this >> southbridge in more detail which could be done in later clean up patches. The piix >> model seems to do the same embedding a 8259 which even less belongs to an IDE >> controller and acceses irqs array directly. >> >>> Looking at the code it seems that i8259_init() returns the PIC IRQ array so it should >>> just be a case of returning the nth entry and using that with >>> qdev_init_gpio_out_named(). >>> >>>>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just do >>>>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be legacy_irq[n] but it >>>>> seems that both drives on MIPS and Pegasos both use IRQ 14). >>>> >>>> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native mode) >>>> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% native >>>> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 686B >>>> datasheet does not detail this but I believe it works the same. Since we currently >>>> fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU PCI >>>> reset interactions using legacy_irq[0] might work but is not correct, the current way >>>> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion. >>> >>> This is not correct though: please re-read my previous email which quotes from the >>> PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon the >>> device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be >>> placing a value other than 14 there, but if it does then that suggests the board IRQs >>> are physically wired differently which again should be handled at board level by qdev. >> >> Correct or not or follows the spec or not this is how it works on real hardware and >> to get guests running we need to emulate this. > > This may be a separate issue. As you mentioned in your email most guest OSs on > Pegasos blindly ignore the PCI_INTERRUPT_LINE register since they know that the IRQs > are hardwired. However you may also be getting caught out by the bug that the current > implementation will try and use the PCI_INTERRUPT_LINE IRQ for routing which is > incorrect. It's not incorrect. It follows what the datasheet says at least for VT8231, the 686B does not detail this but the Linux driver also works on fulong2e with this so I think it's correct for that as well. > Both the 8231 and 636B datasheets mark PCI_INTERRUPT_LINE as read-only with a default > value of 0xe, so I believe that once you've done the qdev gpio part, keeping this > part of your patchset as a separate patch should fix this. I don't get this. My patch emulates that already, what do you want to be changed here? >> Again, this controller is embedded in >> the 868B and 8231 southbridge chips so they may not completely confirm to PCI spec >> but their own datasheets. We can argue in how much detail do we want to model the >> internals of these chips (which we don't know for sure) but I think this patch is >> good enough for now and could be refined later or we likely won't be able to finish >> this before the freeze. >> >> Another way to look at it is that this patch gets some guests running and does not >> break anything as far as I know so is there anything in it that's unacceptable now >> and needs to be changed for it to be merged? Unless you can propose a way to achieve >> the same in a simpler way now I think we could go with this and you can submit follow >> up patches to clean it up as you like later. > > I don't know what else to say except other than what I have already said: both the > current via-ide IRQ implementation and the changes in your patchset are incorrect. Why do you think so? You base this on PCI docs but those don't necessarily apply to via-ide which is not a standalone PCI device but embedded in integrated southbridges whose datasheets say what I've implemented and guests are happy with that. So I don't think it's incorrect maybe just not to your taste. > I've given you the outline of what you need to do to switch the device over to qdev > which agrees with the datasheet, the PCI specification, comments within the QEMU code > itself and the results of your experiments. The proposed change is also simple to > implement (less than an hour's work) using standard APIs. Then please do that and submit alternative patches. I don't understand all of your proposals so it's easier if you can do it in less than an hour. It would probably take me more time. I'll test them with the clients if you provide patches or I've pushed my working version now to https://osdn.net/projects/qmiga/scm/git/qemu/tree/pegasos2/ so you can try as well. > At this time I don't see any point in continuing to repeat myself over and over > again: if you can make the qdev change and post an updated patchset then I'm happy to > review and continue this discussion, but otherwise I don't see any purpose in > continuing this thread. I also think we're not getting anywhere so I hope John and Peter can give some advice here what is needed to get these patches merged. I've run out of time for now so I won't have time to make extensive changes to these patches and can only make reasonable changes or test what you provide. You seem to want to force me again to qdevify everything now which I don't have time for. You did the same to my OpenBIOS patches too where you wanted me to clean up PCI code and implement handling of multiple PCI buses when I just wanted to include a simple workaround to get MorphOS running until the bigger clean up is eventually done. Even though some years have passed you did still not come around to do the OpenBIOS PCI clean up but also not accepted my workaround in the meantime so I need to use patched OpenBIOS for MorphOS ever since. I think we'd end up in the same situation with these patches. (It would be OK if OpenBIOS were otherwise very clean but it's full of other workarounds like patching device tree and installing QEMU VGA driver to any VGA card it sees and others that are much worse than what I've proposed and you don't seem to mind those which come from you just those that someone else propose.) Don't get this wrong, we don't have any personal problem with each other, at least I don't have anything against you but your habit of forcing people to do much more additional work just to satisfy your pedantry is not something I have time to comply with. If you propose changes that make the patches simpler or in some way better I'd do that but otherwise I'll only do what QEMU maintainers also think is necessary. Regards, BALATON Zoltan
diff --git a/hw/ide/via.c b/hw/ide/via.c index 096de8dba0..17418c5822 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -1,9 +1,10 @@ /* - * QEMU IDE Emulation: PCI VIA82C686B support. + * QEMU VIA southbridge IDE emulation (VT82C686B, VT8231) * * Copyright (c) 2003 Fabrice Bellard * Copyright (c) 2006 Openedhand Ltd. * Copyright (c) 2010 Huacai Chen <zltjiangshi@gmail.com> + * Copyright (c) 2019-2020 BALATON Zoltan * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -25,6 +26,8 @@ */ #include "qemu/osdep.h" +#include "qemu/range.h" +#include "hw/qdev-properties.h" #include "hw/pci/pci.h" #include "migration/vmstate.h" #include "qemu/module.h" @@ -111,14 +114,43 @@ static void via_ide_set_irq(void *opaque, int n, int level) } else { d->config[0x70 + n * 8] &= ~0x80; } - level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80); - n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); - if (n) { - qemu_set_irq(isa_get_irq(NULL, n), level); + + /* + * Some machines operate in "non 100% native mode" where PCI_INTERRUPT_LINE + * is not used but IDE always uses ISA IRQ 14 and 15 even in native mode. + * Some guest drivers expect this, often without checking. + */ + if (!(pci_get_byte(d->config + PCI_CLASS_PROG) & (n ? 4 : 1)) || + PCI_IDE(d)->flags & BIT(PCI_IDE_LEGACY_IRQ)) { + qemu_set_irq(isa_get_irq(NULL, (n ? 15 : 14)), level); + } else { + n = pci_get_byte(d->config + PCI_INTERRUPT_LINE); + if (n) { + qemu_set_irq(isa_get_irq(NULL, n), level); + } } } +static uint32_t via_ide_config_read(PCIDevice *d, uint32_t address, int len) +{ + /* + * The pegasos2 firmware writes to PCI_INTERRUPT_LINE but on real + * hardware it's fixed at 14 and won't change. Some guests also expect + * legacy interrupts, without reading PCI_INTERRUPT_LINE but Linux + * depends on this to read 14. We set it to 14 in the reset method and + * also set the wmask to 0 to emulate this but that turns out to be not + * enough. QEMU resets the PCI bus after this device and + * pci_do_device_reset() called from pci_device_reset() will zero + * PCI_INTERRUPT_LINE so this config_read function is to counter that and + * restore the correct value, otherwise this should not be needed. + */ + if (range_covers_byte(address, len, PCI_INTERRUPT_LINE)) { + pci_set_byte(d->config + PCI_INTERRUPT_LINE, 14); + } + return pci_default_read_config(d, address, len); +} + static void via_ide_reset(DeviceState *dev) { PCIIDEState *d = PCI_IDE(dev); @@ -169,7 +201,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */ pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0); - dev->wmask[PCI_INTERRUPT_LINE] = 0xf; + dev->wmask[PCI_CLASS_PROG] = 5; + dev->wmask[PCI_INTERRUPT_LINE] = 0; memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, &d->bus[0], "via-ide0-data", 8); @@ -213,14 +246,23 @@ static void via_ide_exitfn(PCIDevice *dev) } } -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn, + bool legacy_irq) { PCIDevice *dev; - dev = pci_create_simple(bus, devfn, "via-ide"); + dev = pci_create(bus, devfn, "via-ide"); + qdev_prop_set_bit(&dev->qdev, "legacy-irq", legacy_irq); + qdev_init_nofail(&dev->qdev); pci_ide_create_devs(dev, hd_table); } +static Property via_ide_properties[] = { + DEFINE_PROP_BIT("legacy-irq", PCIIDEState, flags, PCI_IDE_LEGACY_IRQ, + false), + DEFINE_PROP_END_OF_LIST(), +}; + static void via_ide_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -229,10 +271,12 @@ static void via_ide_class_init(ObjectClass *klass, void *data) dc->reset = via_ide_reset; k->realize = via_ide_realize; k->exit = via_ide_exitfn; + k->config_read = via_ide_config_read; k->vendor_id = PCI_VENDOR_ID_VIA; k->device_id = PCI_DEVICE_ID_VIA_IDE; k->revision = 0x06; k->class_id = PCI_CLASS_STORAGE_IDE; + device_class_set_props(dc, via_ide_properties); set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); } diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c index 4727b1d3a4..150182c62a 100644 --- a/hw/mips/mips_fulong2e.c +++ b/hw/mips/mips_fulong2e.c @@ -257,7 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc, isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO); ide_drive_get(hd, ARRAY_SIZE(hd)); - via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1)); + via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1), false); pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci"); pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci"); diff --git a/include/hw/ide.h b/include/hw/ide.h index d88c5ee695..2a7001ccba 100644 --- a/include/hw/ide.h +++ b/include/hw/ide.h @@ -18,7 +18,8 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux); -void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); +void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn, + bool legacy_irq); /* ide-mmio.c */ void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
Some machines operate in "non 100% native mode" where interrupts are fixed at legacy IDE interrupts and some guests expect this behaviour without checking based on knowledge about hardware. Even Linux has arch specific workarounds for this that are activated on such boards so this needs to be emulated as well. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/ide/via.c | 60 +++++++++++++++++++++++++++++++++++------ hw/mips/mips_fulong2e.c | 2 +- include/hw/ide.h | 3 ++- 3 files changed, 55 insertions(+), 10 deletions(-)