| Message ID | 20231024224056.842607-4-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | ide: implement simple legacy/native mode switching for PCI IDE controllers | expand |
Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben: > Allow the VIA IDE controller to switch between both legacy and native modes by > calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG > is updated. > > This patch moves the initial setting of PCI_CLASS_PROG from via_ide_realize() to > via_ide_reset(), and removes the direct setting of PCI_INTERRUPT_PIN during PCI > bus reset since this is now managed by pci_ide_update_mode(). This ensures that > the device configuration is always consistent with respect to the currently > selected mode. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > Tested-by: Bernhard Beschow <shentey@gmail.com> As I already noted in patch 1, the interrupt handling seems to be wrong here, it continues to use the ISA IRQ in via_ide_set_irq() even after switching to native mode. Other than this, the patch looks good to me. Kevin
On Mon, 6 Nov 2023, Kevin Wolf wrote: > Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben: >> Allow the VIA IDE controller to switch between both legacy and native modes by >> calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG >> is updated. >> >> This patch moves the initial setting of PCI_CLASS_PROG from via_ide_realize() to >> via_ide_reset(), and removes the direct setting of PCI_INTERRUPT_PIN during PCI >> bus reset since this is now managed by pci_ide_update_mode(). This ensures that >> the device configuration is always consistent with respect to the currently >> selected mode. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> Tested-by: BALATON Zoltan <balaton@eik.bme.hu> >> Tested-by: Bernhard Beschow <shentey@gmail.com> > > As I already noted in patch 1, the interrupt handling seems to be wrong > here, it continues to use the ISA IRQ in via_ide_set_irq() even after > switching to native mode. That's a peculiarity of this via-ide device. It always uses 14/15 legacy interrupts even in native mode and guests expect that so using native interrupts would break pegasos2 guests. This was discussed and tested extensively before. Regards, BALATON Zoltan > Other than this, the patch looks good to me. > > Kevin > >
Am 06.11.2023 um 17:13 hat BALATON Zoltan geschrieben: > On Mon, 6 Nov 2023, Kevin Wolf wrote: > > Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben: > > > Allow the VIA IDE controller to switch between both legacy and native modes by > > > calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG > > > is updated. > > > > > > This patch moves the initial setting of PCI_CLASS_PROG from via_ide_realize() to > > > via_ide_reset(), and removes the direct setting of PCI_INTERRUPT_PIN during PCI > > > bus reset since this is now managed by pci_ide_update_mode(). This ensures that > > > the device configuration is always consistent with respect to the currently > > > selected mode. > > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> > > > Tested-by: Bernhard Beschow <shentey@gmail.com> > > > > As I already noted in patch 1, the interrupt handling seems to be wrong > > here, it continues to use the ISA IRQ in via_ide_set_irq() even after > > switching to native mode. > > That's a peculiarity of this via-ide device. It always uses 14/15 legacy > interrupts even in native mode and guests expect that so using native > interrupts would break pegasos2 guests. This was discussed and tested > extensively before. This definitely needs a comment to explain the situation then because this is in violation of the spec. If real hardware behaves like this, it's what we should do, of course, but it's certainly unexpected and we should explicitly document it to avoid breaking it later when someone touches the code who doesn't know about this peculiarity. Kevin
On 07/11/2023 10:43, Kevin Wolf wrote: > Am 06.11.2023 um 17:13 hat BALATON Zoltan geschrieben: >> On Mon, 6 Nov 2023, Kevin Wolf wrote: >>> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben: >>>> Allow the VIA IDE controller to switch between both legacy and native modes by >>>> calling pci_ide_update_mode() to reconfigure the device whenever PCI_CLASS_PROG >>>> is updated. >>>> >>>> This patch moves the initial setting of PCI_CLASS_PROG from via_ide_realize() to >>>> via_ide_reset(), and removes the direct setting of PCI_INTERRUPT_PIN during PCI >>>> bus reset since this is now managed by pci_ide_update_mode(). This ensures that >>>> the device configuration is always consistent with respect to the currently >>>> selected mode. >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> Tested-by: Bernhard Beschow <shentey@gmail.com> >>> >>> As I already noted in patch 1, the interrupt handling seems to be wrong >>> here, it continues to use the ISA IRQ in via_ide_set_irq() even after >>> switching to native mode. >> >> That's a peculiarity of this via-ide device. It always uses 14/15 legacy >> interrupts even in native mode and guests expect that so using native >> interrupts would break pegasos2 guests. This was discussed and tested >> extensively before. > > This definitely needs a comment to explain the situation then because > this is in violation of the spec. If real hardware behaves like this, > it's what we should do, of course, but it's certainly unexpected and we > should explicitly document it to avoid breaking it later when someone > touches the code who doesn't know about this peculiarity. It's a little bit more complicated than this: in native mode it is possible to route the IRQs for each individual channel to a small select number of IRQs by configuring special registers on the VIA. The complication here is that it isn't immediately obvious how the QEMU PCI routing code can do this - I did post about this at https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg10552.html asking the best way to resolve this, but haven't had any replies yet. Fortunately it seems that all the guests tested so far stick with the IRQ 14/15 defaults which is why this happens to work, so short-term this is a lower priority when looking at consolidating the switching logic. ATB, Mark.
On Mon, 13 Nov 2023, Mark Cave-Ayland wrote: > On 07/11/2023 10:43, Kevin Wolf wrote: >> Am 06.11.2023 um 17:13 hat BALATON Zoltan geschrieben: >>> On Mon, 6 Nov 2023, Kevin Wolf wrote: >>>> Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben: >>>>> Allow the VIA IDE controller to switch between both legacy and native >>>>> modes by >>>>> calling pci_ide_update_mode() to reconfigure the device whenever >>>>> PCI_CLASS_PROG >>>>> is updated. >>>>> >>>>> This patch moves the initial setting of PCI_CLASS_PROG from >>>>> via_ide_realize() to >>>>> via_ide_reset(), and removes the direct setting of PCI_INTERRUPT_PIN >>>>> during PCI >>>>> bus reset since this is now managed by pci_ide_update_mode(). This >>>>> ensures that >>>>> the device configuration is always consistent with respect to the >>>>> currently >>>>> selected mode. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> Tested-by: BALATON Zoltan <balaton@eik.bme.hu> >>>>> Tested-by: Bernhard Beschow <shentey@gmail.com> >>>> >>>> As I already noted in patch 1, the interrupt handling seems to be wrong >>>> here, it continues to use the ISA IRQ in via_ide_set_irq() even after >>>> switching to native mode. >>> >>> That's a peculiarity of this via-ide device. It always uses 14/15 legacy >>> interrupts even in native mode and guests expect that so using native >>> interrupts would break pegasos2 guests. This was discussed and tested >>> extensively before. >> >> This definitely needs a comment to explain the situation then because >> this is in violation of the spec. If real hardware behaves like this, >> it's what we should do, of course, but it's certainly unexpected and we >> should explicitly document it to avoid breaking it later when someone >> touches the code who doesn't know about this peculiarity. > > It's a little bit more complicated than this: in native mode it is possible > to route the IRQs for each individual channel to a small select number of > IRQs by configuring special registers on the VIA. That's documented for VT82c686B but VT8231 doc says other values are reserved and only IRQ 14/15 is valid. So even if it worked nothing uses it and we don't have to be concerned about it and just using these hard coded 14/15 is enough. It probably does not worth trying to emulate chip functions that no guest uses, especially when we're not sure how the real chip works as we can't test on real machine. Regards, BALATON Zoltan > The complication here is that it isn't immediately obvious how the QEMU PCI > routing code can do this - I did post about this at > https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg10552.html asking > the best way to resolve this, but haven't had any replies yet. > > Fortunately it seems that all the guests tested so far stick with the IRQ > 14/15 defaults which is why this happens to work, so short-term this is a > lower priority when looking at consolidating the switching logic. > > > ATB, > > Mark. > >
diff --git a/hw/ide/via.c b/hw/ide/via.c index 87b134083a..0cb65c8a3d 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -28,6 +28,7 @@ #include "hw/pci/pci.h" #include "migration/vmstate.h" #include "qemu/module.h" +#include "qemu/range.h" #include "sysemu/dma.h" #include "hw/isa/vt82c686.h" #include "hw/ide/pci.h" @@ -128,11 +129,14 @@ static void via_ide_reset(DeviceState *dev) ide_bus_reset(&d->bus[i]); } + pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */ + pci_ide_update_mode(d); + pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT); pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM); - pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x0000010e); + pci_set_byte(pci_conf + PCI_INTERRUPT_LINE, 0xe); /* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/ pci_set_long(pci_conf + 0x40, 0x0a090600); @@ -154,6 +158,18 @@ static void via_ide_reset(DeviceState *dev) pci_set_long(pci_conf + 0xc0, 0x00020001); } +static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr, + uint32_t val, int len) +{ + PCIIDEState *d = PCI_IDE(pd); + + pci_default_write_config(pd, addr, val, len); + + if (range_covers_byte(addr, len, PCI_CLASS_PROG)) { + pci_ide_update_mode(d); + } +} + static void via_ide_realize(PCIDevice *dev, Error **errp) { PCIIDEState *d = PCI_IDE(dev); @@ -161,7 +177,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) uint8_t *pci_conf = dev->config; int i; - pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */ pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0); dev->wmask[PCI_INTERRUPT_LINE] = 0; dev->wmask[PCI_CLASS_PROG] = 5; @@ -216,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data) /* Reason: only works as function of VIA southbridge */ dc->user_creatable = false; + k->config_write = via_ide_cfg_write; k->realize = via_ide_realize; k->exit = via_ide_exitfn; k->vendor_id = PCI_VENDOR_ID_VIA;