| Message ID | 20210518215545.1793947-3-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/isa: Remove dependencies on ISA bus singleton | expand |
On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote: > IDE bus depends on ISA bus for IRQ/DMA. > > Add an ISABus reference in PCIIDEState, and add link properties > to it in the PIIX and VIA objects (which inherit PCI_IDE). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/hw/ide/pci.h | 1 + > hw/ide/piix.c | 11 ++++++++++- > hw/ide/via.c | 10 +++++++++- > 3 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index d8384e1c422..e790722ed14 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -47,6 +47,7 @@ struct PCIIDEState { > PCIDevice parent_obj; > /*< public >*/ > > + ISABus *isa_bus; I'm not sure that this belongs here. Previously we managed to remove device specific fields from this structure so it's now really just holds stuff related to PCI IDE (except the remaining "secondary" field specific to CMD646). PCI IDE normaly has nothing to do with ISA except for those south bridges that have IDE with legacy mode. So this ISABus reference should be in those south bridges instead. But that may need a new subclass just for this so putting it here is just avoiding boilerplate of declaring new subclasses in piix and via-ide. I can sympathise with that but I'd still prefer to keep it off here but I wonder if there's a way to do that without subclassing and storing an ISABus ref? If I understand correctly this ISABus ref is just needed to get appropriate ISA irqs. But could we just store a ref to those irqs directly so we don't need to keep the ref to the ISA bus? There's already a qemu_irq in BMDMAState but I'm not sure how those are set and if you could store an isa irq there to simplify this. I don't know the details and could not detangle it by a brief look so not sure it can be done but conceptually it feels better to keep PCI IDE separate from ISA and let it raise either PCI irq or ISA irq as needed. For that a ref to the irq should be enough and that can either come from a PCI bus (which is normaly expected for PCI IDE) or an ISA bridge for legacy modes. Hope it makes sense and you get what I'm trying to say. (Longer term we may want to make it changeable also after the device is created to allow switching between legacy and PCI mode but so far we could get away without emulating that so it's not a requirement just something to consider when you're changing this. The real problem that prevents switching modes is not irq I think but ioports and that ISA devices are not configurable after creating them but that would need QOM'ifying ISA emulation which probably does not worth the effort unless we come across some guest that needs this.) Regards, BALATON Zoltan > IDEBus bus[2]; > BMDMAState bmdma[2]; > uint32_t secondary; /* used only for cmd646 */ > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index b9860e35a5c..48da68da37f 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -30,8 +30,9 @@ > #include "sysemu/block-backend.h" > #include "sysemu/blockdev.h" > #include "sysemu/dma.h" > - > +#include "qapi/error.h" > #include "hw/ide/pci.h" > +#include "hw/isa/isa.h" > #include "trace.h" > > static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size) > @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev) > } > } > > +static Property piix_ide_properties[] = { > + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, > + TYPE_ISA_BUS, ISABus *), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */ > static void piix3_ide_class_init(ObjectClass *klass, void *data) > { > @@ -221,6 +228,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data) > k->class_id = PCI_CLASS_STORAGE_IDE; > set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); > dc->hotpluggable = false; > + device_class_set_props(dc, piix_ide_properties); > } > > static const TypeInfo piix3_ide_info = { > @@ -249,6 +257,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data) > k->class_id = PCI_CLASS_STORAGE_IDE; > set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); > dc->hotpluggable = false; > + device_class_set_props(dc, piix_ide_properties); > } > > static const TypeInfo piix4_ide_info = { > diff --git a/hw/ide/via.c b/hw/ide/via.c > index be09912b334..65fdca6dcf4 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -28,8 +28,9 @@ > #include "hw/pci/pci.h" > #include "migration/vmstate.h" > #include "qemu/module.h" > +#include "qapi/error.h" > #include "sysemu/dma.h" > - > +#include "hw/isa/isa.h" > #include "hw/ide/pci.h" > #include "trace.h" > > @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev) > } > } > > +static Property via_ide_properties[] = { > + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, > + TYPE_ISA_BUS, ISABus *), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > static void via_ide_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data) > k->revision = 0x06; > k->class_id = PCI_CLASS_STORAGE_IDE; > set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); > + device_class_set_props(dc, via_ide_properties); > } > > static const TypeInfo via_ide_info = { >
On 5/18/21 7:05 PM, BALATON Zoltan wrote: > On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote: >> IDE bus depends on ISA bus for IRQ/DMA. >> >> Add an ISABus reference in PCIIDEState, and add link properties >> to it in the PIIX and VIA objects (which inherit PCI_IDE). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/hw/ide/pci.h | 1 + >> hw/ide/piix.c | 11 ++++++++++- >> hw/ide/via.c | 10 +++++++++- >> 3 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >> index d8384e1c422..e790722ed14 100644 >> --- a/include/hw/ide/pci.h >> +++ b/include/hw/ide/pci.h >> @@ -47,6 +47,7 @@ struct PCIIDEState { >> PCIDevice parent_obj; >> /*< public >*/ >> >> + ISABus *isa_bus; > > I'm not sure that this belongs here. Previously we managed to remove > device specific fields from this structure so it's now really just holds > stuff related to PCI IDE (except the remaining "secondary" field > specific to CMD646). PCI IDE normaly has nothing to do with ISA except > for those south bridges that have IDE with legacy mode. So this ISABus > reference should be in those south bridges instead. But that may need a by "those south bridges" I assume you are referring to the integrated PIIX and VIA controller implementations. > new subclass just for this so putting it here is just avoiding > boilerplate of declaring new subclasses in piix and via-ide. I can > sympathise with that but I'd still prefer to keep it off here but I > wonder if there's a way to do that without subclassing and storing an > ISABus ref? If I understand correctly this ISABus ref is just needed to > get appropriate ISA irqs. But could we just store a ref to those irqs It looks like it's just the IRQs, yeah. > directly so we don't need to keep the ref to the ISA bus? There's I think the idea actually is to formalize the dependency of these models on the ISA bus instead of hacking / faking one. I think we DO want the dependency. > already a qemu_irq in BMDMAState but I'm not sure how those are set and > if you could store an isa irq there to simplify this. I don't know the > details and could not detangle it by a brief look so not sure it can be > done but conceptually it feels better to keep PCI IDE separate from ISA > and let it raise either PCI irq or ISA irq as needed. For that a ref to > the irq should be enough and that can either come from a PCI bus (which > is normaly expected for PCI IDE) or an ISA bridge for legacy modes. Hope > it makes sense and you get what I'm trying to say. (Longer term we may > want to make it changeable also after the device is created to allow > switching between legacy and PCI mode but so far we could get away > without emulating that so it's not a requirement just something to > consider when you're changing this. The real problem that prevents > switching modes is not irq I think but ioports and that ISA devices are > not configurable after creating them but that would need QOM'ifying ISA > emulation which probably does not worth the effort unless we come across > some guest that needs this.) > > Regards, > BALATON Zoltan > I assume the idea here is that PCIIDE does not technically need "ISA" to provide ioport access to the ATA drives, so taxonomically it's odd for the generic/abstract PCIIDE device to require an ISA bus. Am I understanding correctly? >> IDEBus bus[2]; >> BMDMAState bmdma[2]; >> uint32_t secondary; /* used only for cmd646 */ >> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >> index b9860e35a5c..48da68da37f 100644 >> --- a/hw/ide/piix.c >> +++ b/hw/ide/piix.c >> @@ -30,8 +30,9 @@ >> #include "sysemu/block-backend.h" >> #include "sysemu/blockdev.h" >> #include "sysemu/dma.h" >> - >> +#include "qapi/error.h" >> #include "hw/ide/pci.h" >> +#include "hw/isa/isa.h" >> #include "trace.h" >> >> static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size) >> @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev) >> } >> } >> >> +static Property piix_ide_properties[] = { >> + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, >> + TYPE_ISA_BUS, ISABus *), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */ >> static void piix3_ide_class_init(ObjectClass *klass, void *data) >> { >> @@ -221,6 +228,7 @@ static void piix3_ide_class_init(ObjectClass >> *klass, void *data) >> k->class_id = PCI_CLASS_STORAGE_IDE; >> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >> dc->hotpluggable = false; >> + device_class_set_props(dc, piix_ide_properties); >> } >> >> static const TypeInfo piix3_ide_info = { >> @@ -249,6 +257,7 @@ static void piix4_ide_class_init(ObjectClass >> *klass, void *data) >> k->class_id = PCI_CLASS_STORAGE_IDE; >> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >> dc->hotpluggable = false; >> + device_class_set_props(dc, piix_ide_properties); >> } >> >> static const TypeInfo piix4_ide_info = { >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index be09912b334..65fdca6dcf4 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -28,8 +28,9 @@ >> #include "hw/pci/pci.h" >> #include "migration/vmstate.h" >> #include "qemu/module.h" >> +#include "qapi/error.h" >> #include "sysemu/dma.h" >> - >> +#include "hw/isa/isa.h" >> #include "hw/ide/pci.h" >> #include "trace.h" >> >> @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev) >> } >> } >> >> +static Property via_ide_properties[] = { >> + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, >> + TYPE_ISA_BUS, ISABus *), >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> static void via_ide_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, >> void *data) >> k->revision = 0x06; >> k->class_id = PCI_CLASS_STORAGE_IDE; >> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >> + device_class_set_props(dc, via_ide_properties); >> } >> >> static const TypeInfo via_ide_info = { >>
On Wed, 19 May 2021, John Snow wrote: > On 5/18/21 7:05 PM, BALATON Zoltan wrote: >> On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote: >>> IDE bus depends on ISA bus for IRQ/DMA. >>> >>> Add an ISABus reference in PCIIDEState, and add link properties >>> to it in the PIIX and VIA objects (which inherit PCI_IDE). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> include/hw/ide/pci.h | 1 + >>> hw/ide/piix.c | 11 ++++++++++- >>> hw/ide/via.c | 10 +++++++++- >>> 3 files changed, 20 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>> index d8384e1c422..e790722ed14 100644 >>> --- a/include/hw/ide/pci.h >>> +++ b/include/hw/ide/pci.h >>> @@ -47,6 +47,7 @@ struct PCIIDEState { >>> PCIDevice parent_obj; >>> /*< public >*/ >>> >>> + ISABus *isa_bus; >> >> I'm not sure that this belongs here. Previously we managed to remove device >> specific fields from this structure so it's now really just holds stuff >> related to PCI IDE (except the remaining "secondary" field specific to >> CMD646). PCI IDE normaly has nothing to do with ISA except for those south >> bridges that have IDE with legacy mode. So this ISABus reference should be >> in those south bridges instead. But that may need a > > by "those south bridges" I assume you are referring to the integrated PIIX > and VIA controller implementations. Yes, those are that also have an ISA bridge so the IDE in those can use either ISA or PCI IRQs but we probably only emulate one mode. At least that's the case for via-ide which we have gone into great detail before and concluded we can't cleanly switch between legacy ISA or PCI mode and the pegasos2 needs hard coded ISA interrupts even when in PCI mode so we only emulate that. Apart from that this PCI IDE is also used by CMD646 and sii3112 that are pure PCI IDE devices without any ISA dependency so that's why I think we should not need this ISABus here to keep this implementing PCI IDE and only keep ISA in the south bridge models. >> new subclass just for this so putting it here is just avoiding boilerplate >> of declaring new subclasses in piix and via-ide. I can sympathise with that >> but I'd still prefer to keep it off here but I wonder if there's a way to >> do that without subclassing and storing an ISABus ref? If I understand >> correctly this ISABus ref is just needed to get appropriate ISA irqs. But >> could we just store a ref to those irqs > > It looks like it's just the IRQs, yeah. > >> directly so we don't need to keep the ref to the ISA bus? There's > > I think the idea actually is to formalize the dependency of these models on > the ISA bus instead of hacking / faking one. I think we DO want the > dependency. Right, but only piix and via depend on ISA so the dependency should be in those not in PCI IDE in my opinion. But I don't mind too much so if it would be too difficult to put it elsewhere I don't mind introducing this ISABus field but we should at least look if it could be avoided first. >> already a qemu_irq in BMDMAState but I'm not sure how those are set and if >> you could store an isa irq there to simplify this. I don't know the details >> and could not detangle it by a brief look so not sure it can be done but >> conceptually it feels better to keep PCI IDE separate from ISA and let it >> raise either PCI irq or ISA irq as needed. For that a ref to the irq should >> be enough and that can either come from a PCI bus (which is normaly >> expected for PCI IDE) or an ISA bridge for legacy modes. Hope it makes >> sense and you get what I'm trying to say. (Longer term we may want to make >> it changeable also after the device is created to allow switching between >> legacy and PCI mode but so far we could get away without emulating that so >> it's not a requirement just something to consider when you're changing >> this. The real problem that prevents switching modes is not irq I think but >> ioports and that ISA devices are not configurable after creating them but >> that would need QOM'ifying ISA emulation which probably does not worth the >> effort unless we come across some guest that needs this.) >> >> Regards, >> BALATON Zoltan >> > > I assume the idea here is that PCIIDE does not technically need "ISA" to > provide ioport access to the ATA drives, so taxonomically it's odd for the > generic/abstract PCIIDE device to require an ISA bus. > > Am I understanding correctly? I'm not sure I understand all of the IDE emulation but in my view PCI IDE should be independent of ISA so instead of adding a reference to an ISA bus to PCIIDEState maybe it's enough to set the irqs used by PCI IDE to the appropriate irq to raise which could be an ISA interrupt for the south bridges in legacy mode or a PCI irq for PCI cards and that way we don't need a dependency on ISABus in PCI IDE. But I'm not sure how IDE interrupts are set so don't know if that would work so it's just an idea to avoid introducing ISA into PCI IDE where it does not seem to belong. A simpler way keeping the current code may be to subclass PCI IDE in piix and via and put the ISABus ref in those subclasses but that's more boiler plate and the result may not be simpler so while conceptually may be cleaner the code may be longer and harder to understand that way. So cleaning up the interrupt handling could make it simpler and also avoid the subclasses but that needs more work to detangle how IDE interrupts are emulated and add some clean way to set them if that's not yet available. But I don't completely understand what the qemu_irqs are in BMDMAState and if those could be connected to an ISA interrupt or some more changes would be needed. Regards, BALATON Zoltan >>> IDEBus bus[2]; >>> BMDMAState bmdma[2]; >>> uint32_t secondary; /* used only for cmd646 */ >>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >>> index b9860e35a5c..48da68da37f 100644 >>> --- a/hw/ide/piix.c >>> +++ b/hw/ide/piix.c >>> @@ -30,8 +30,9 @@ >>> #include "sysemu/block-backend.h" >>> #include "sysemu/blockdev.h" >>> #include "sysemu/dma.h" >>> - >>> +#include "qapi/error.h" >>> #include "hw/ide/pci.h" >>> +#include "hw/isa/isa.h" >>> #include "trace.h" >>> >>> static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size) >>> @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev) >>> } >>> } >>> >>> +static Property piix_ide_properties[] = { >>> + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, >>> + TYPE_ISA_BUS, ISABus *), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */ >>> static void piix3_ide_class_init(ObjectClass *klass, void *data) >>> { >>> @@ -221,6 +228,7 @@ static void piix3_ide_class_init(ObjectClass *klass, >>> void *data) >>> k->class_id = PCI_CLASS_STORAGE_IDE; >>> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >>> dc->hotpluggable = false; >>> + device_class_set_props(dc, piix_ide_properties); >>> } >>> >>> static const TypeInfo piix3_ide_info = { >>> @@ -249,6 +257,7 @@ static void piix4_ide_class_init(ObjectClass *klass, >>> void *data) >>> k->class_id = PCI_CLASS_STORAGE_IDE; >>> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >>> dc->hotpluggable = false; >>> + device_class_set_props(dc, piix_ide_properties); >>> } >>> >>> static const TypeInfo piix4_ide_info = { >>> diff --git a/hw/ide/via.c b/hw/ide/via.c >>> index be09912b334..65fdca6dcf4 100644 >>> --- a/hw/ide/via.c >>> +++ b/hw/ide/via.c >>> @@ -28,8 +28,9 @@ >>> #include "hw/pci/pci.h" >>> #include "migration/vmstate.h" >>> #include "qemu/module.h" >>> +#include "qapi/error.h" >>> #include "sysemu/dma.h" >>> - >>> +#include "hw/isa/isa.h" >>> #include "hw/ide/pci.h" >>> #include "trace.h" >>> >>> @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev) >>> } >>> } >>> >>> +static Property via_ide_properties[] = { >>> + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, >>> + TYPE_ISA_BUS, ISABus *), >>> + DEFINE_PROP_END_OF_LIST(), >>> +}; >>> + >>> static void via_ide_class_init(ObjectClass *klass, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(klass); >>> @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, >>> void *data) >>> k->revision = 0x06; >>> k->class_id = PCI_CLASS_STORAGE_IDE; >>> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); >>> + device_class_set_props(dc, via_ide_properties); >>> } >>> >>> static const TypeInfo via_ide_info = { >>> > >
On 19/05/2021 00:05, BALATON Zoltan wrote: > On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote: >> IDE bus depends on ISA bus for IRQ/DMA. >> >> Add an ISABus reference in PCIIDEState, and add link properties >> to it in the PIIX and VIA objects (which inherit PCI_IDE). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/hw/ide/pci.h | 1 + >> hw/ide/piix.c | 11 ++++++++++- >> hw/ide/via.c | 10 +++++++++- >> 3 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >> index d8384e1c422..e790722ed14 100644 >> --- a/include/hw/ide/pci.h >> +++ b/include/hw/ide/pci.h >> @@ -47,6 +47,7 @@ struct PCIIDEState { >> PCIDevice parent_obj; >> /*< public >*/ >> >> + ISABus *isa_bus; > > I'm not sure that this belongs here. Previously we managed to remove device specific > fields from this structure so it's now really just holds stuff related to PCI IDE > (except the remaining "secondary" field specific to CMD646). PCI IDE normaly has > nothing to do with ISA except for those south bridges that have IDE with legacy mode. > So this ISABus reference should be in those south bridges instead. But that may need > a new subclass just for this so putting it here is just avoiding boilerplate of > declaring new subclasses in piix and via-ide. I can sympathise with that but I'd > still prefer to keep it off here but I wonder if there's a way to do that without > subclassing and storing an ISABus ref? If I understand correctly this ISABus ref is > just needed to get appropriate ISA irqs. But could we just store a ref to those irqs > directly so we don't need to keep the ref to the ISA bus? There's already a qemu_irq > in BMDMAState but I'm not sure how those are set and if you could store an isa irq > there to simplify this. I don't know the details and could not detangle it by a brief > look so not sure it can be done but conceptually it feels better to keep PCI IDE > separate from ISA and let it raise either PCI irq or ISA irq as needed. For that a > ref to the irq should be enough and that can either come from a PCI bus (which is > normaly expected for PCI IDE) or an ISA bridge for legacy modes. Hope it makes sense > and you get what I'm trying to say. (Longer term we may want to make it changeable > also after the device is created to allow switching between legacy and PCI mode but > so far we could get away without emulating that so it's not a requirement just > something to consider when you're changing this. The real problem that prevents > switching modes is not irq I think but ioports and that ISA devices are not > configurable after creating them but that would need QOM'ifying ISA emulation which > probably does not worth the effort unless we come across some guest that needs this.) Right. I've had a quick look over the patchset and the IRQ changes look good: the part I'm not keen on is adding the property links for the ISABus directly to these devices, since as Zoltan correctly points out this is handled by a PCI-ISA bridge in the PCI host bridge and not the PCI device itself. One of the better diagrams to explain how this wires together is on the QEMU wiki at https://wiki.qemu.org/Features/Q35. Bear in mind that the PCI-ISA bridge is optional and not all PCI bridges have them. I spent a bit of time trying to figure how we know which devices have a PCI-ISA bridge and then realised that it is those devices with a PCI class id of PCI_CLASS_BRIDGE_ISA. If you grep for this then it is possible to see that there are a couple of places that check for whether a PCI device is a PCI-ISA bridge by checking the PCI class id for PCI_CLASS_BRIDGE_ISA instead of using a QOM type. This gives me the following idea: 1) Create an abstract TYPE_PCI_ISA_BRIDGE QOM type with parent PCI_DEVICE and change the existing devices with PCI_CLASS_BRIDGE_ISA so that they have TYPE_PCI_ISA_BRIDGE as a parent 2) In the PCI_ISA_BRIDGE realize function add the following common code to instantiate the ISABus and remove it from the individual PCI_CLASS_BRIDGE_ISA devices: static void pci_isa_bridge_realize(DeviceState *dev) { PCIISABridge *s = PCI_ISA_BRIDGE(dev); s->isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev), pci_address_space_io(dev), errp); /* FIXME: this should probably use errp */ if (!s->isa_bus) { return; } } You may also be able to do something similar with the 8259 IRQ initialisation although I haven't really looked at that. 3) Add a new pci_isa_bridge_get_isabus(PCIDevice *d) function that the devices such as via-ide can use to obtain a reference to the ISABus from their own PCIDevice. It should hopefully be quite simple like this: ISABus *pci_isa_bridge_get_isabus(PCIDevice *d) { PCIISABridge *s = PCI_ISA_BRIDGE(d); return s->isa_bus; } I think this is the best solution since it avoids either 1) having to embed ISABus unconditionally into PCIHostState and therefore bringing in all ISA devices for all PCI builds and 2) it avoids having to manually link the ISABus directly for the affected devices. It also allows for a future cleanup where PCI-ISA bridges can be detected by checking QOM type rather than PCI class id directly. Once that is done the follow on work could be to investigate how TYPE_PCI_ISA_BRIDGE devices can use the ISABus to handle the switch between legacy mode and PCI mode, but that's certainly a job for another day. ATB, Mark.
On 20/05/2021 08:41, Mark Cave-Ayland wrote: > 3) Add a new pci_isa_bridge_get_isabus(PCIDevice *d) function that the devices such > as via-ide can use to obtain a reference to the ISABus from their own PCIDevice. It > should hopefully be quite simple like this: > > ISABus *pci_isa_bridge_get_isabus(PCIDevice *d) > { > PCIISABridge *s = PCI_ISA_BRIDGE(d); > > return s->isa_bus; > } Oops - have just realised that PCIDevice in this case is the PCI/ISA device and not the bridge itself. This means there is a bit more work to do, perhaps something like: ISABus *pci_device_get_isabus(PCIDevice *d) { PCIBus *bus = pci_get_bus(d); PCIDeviceClass *k; int devfn; for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) { PCIDevice *pd = bus->devices[devfn]; PCIDeviceClass *k = PCI_DEVICE_GET_CLASS(d); PCIISABridge *s; if (object_dynamic_cast(OBJECT(pd), TYPE_PCI_ISA_BRIDGE)) { s = PCI_ISA_BRIDGE(pd); return s->isa_bus; } } return NULL; } Given that a PCI-ISA bridge effectively manages the bottom part of the single IO address space then I believe there can only be one PCI-ISA bridge per PCI host bridge, and therefore bus. ATB, Mark.
On Thu, May 20, 2021 at 02:46:47AM +0200, BALATON Zoltan wrote: > On Wed, 19 May 2021, John Snow wrote: > > On 5/18/21 7:05 PM, BALATON Zoltan wrote: > > > On Tue, 18 May 2021, Philippe Mathieu-Daudé wrote: > > > > IDE bus depends on ISA bus for IRQ/DMA. > > > > > > > > Add an ISABus reference in PCIIDEState, and add link properties > > > > to it in the PIIX and VIA objects (which inherit PCI_IDE). > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > --- > > > > include/hw/ide/pci.h | 1 + > > > > hw/ide/piix.c | 11 ++++++++++- > > > > hw/ide/via.c | 10 +++++++++- > > > > 3 files changed, 20 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > > > > index d8384e1c422..e790722ed14 100644 > > > > --- a/include/hw/ide/pci.h > > > > +++ b/include/hw/ide/pci.h > > > > @@ -47,6 +47,7 @@ struct PCIIDEState { > > > > PCIDevice parent_obj; > > > > /*< public >*/ > > > > > > > > + ISABus *isa_bus; > > > > > > I'm not sure that this belongs here. Previously we managed to remove > > > device specific fields from this structure so it's now really just > > > holds stuff related to PCI IDE (except the remaining "secondary" > > > field specific to CMD646). PCI IDE normaly has nothing to do with > > > ISA except for those south bridges that have IDE with legacy mode. > > > So this ISABus reference should be in those south bridges instead. > > > But that may need a > > > > by "those south bridges" I assume you are referring to the integrated > > PIIX and VIA controller implementations. > > Yes, those are that also have an ISA bridge so the IDE in those can use > either ISA or PCI IRQs but we probably only emulate one mode. At least > that's the case for via-ide which we have gone into great detail before and > concluded we can't cleanly switch between legacy ISA or PCI mode and the > pegasos2 needs hard coded ISA interrupts even when in PCI mode so we only > emulate that. > > Apart from that this PCI IDE is also used by CMD646 and sii3112 that are > pure PCI IDE devices without any ISA dependency so that's why I think we > should not need this ISABus here to keep this implementing PCI IDE and only > keep ISA in the south bridge models. > > > > new subclass just for this so putting it here is just avoiding > > > boilerplate of declaring new subclasses in piix and via-ide. I can > > > sympathise with that but I'd still prefer to keep it off here but I > > > wonder if there's a way to do that without subclassing and storing > > > an ISABus ref? If I understand correctly this ISABus ref is just > > > needed to get appropriate ISA irqs. But could we just store a ref to > > > those irqs > > > > It looks like it's just the IRQs, yeah. > > > > > directly so we don't need to keep the ref to the ISA bus? There's > > > > I think the idea actually is to formalize the dependency of these models > > on the ISA bus instead of hacking / faking one. I think we DO want the > > dependency. > > Right, but only piix and via depend on ISA so the dependency should be in > those not in PCI IDE in my opinion. But I don't mind too much so if it would > be too difficult to put it elsewhere I don't mind introducing this ISABus > field but we should at least look if it could be avoided first. > > > > already a qemu_irq in BMDMAState but I'm not sure how those are set > > > and if you could store an isa irq there to simplify this. I don't > > > know the details and could not detangle it by a brief look so not > > > sure it can be done but conceptually it feels better to keep PCI IDE > > > separate from ISA and let it raise either PCI irq or ISA irq as > > > needed. For that a ref to the irq should be enough and that can > > > either come from a PCI bus (which is normaly expected for PCI IDE) > > > or an ISA bridge for legacy modes. Hope it makes sense and you get > > > what I'm trying to say. (Longer term we may want to make it > > > changeable also after the device is created to allow switching > > > between legacy and PCI mode but so far we could get away without > > > emulating that so it's not a requirement just something to consider > > > when you're changing this. The real problem that prevents switching > > > modes is not irq I think but ioports and that ISA devices are not > > > configurable after creating them but that would need QOM'ifying ISA > > > emulation which probably does not worth the effort unless we come > > > across some guest that needs this.) > > > > > > Regards, > > > BALATON Zoltan > > > > > > > I assume the idea here is that PCIIDE does not technically need "ISA" to > > provide ioport access to the ATA drives, so taxonomically it's odd for > > the generic/abstract PCIIDE device to require an ISA bus. > > > > Am I understanding correctly? > > I'm not sure I understand all of the IDE emulation but in my view PCI IDE > should be independent of ISA so instead of adding a reference to an ISA bus > to PCIIDEState maybe it's enough to set the irqs used by PCI IDE to the > appropriate irq to raise which could be an ISA interrupt for the south > bridges in legacy mode or a PCI irq for PCI cards and that way we don't need > a dependency on ISABus in PCI IDE. But I'm not sure how IDE interrupts are > set so don't know if that would work so it's just an idea to avoid > introducing ISA into PCI IDE where it does not seem to belong. > > A simpler way keeping the current code may be to subclass PCI IDE in piix > and via and put the ISABus ref in those subclasses but that's more boiler > plate and the result may not be simpler so while conceptually may be cleaner > the code may be longer and harder to understand that way. So cleaning up the > interrupt handling could make it simpler and also avoid the subclasses but > that needs more work to detangle how IDE interrupts are emulated and add > some clean way to set them if that's not yet available. But I don't > completely understand what the qemu_irqs are in BMDMAState and if those > could be connected to an ISA interrupt or some more changes would be needed. I realized I don't really understand how ISA IDE and PCI IDE interact in PIIX3: - ISA IDE has well-known PIO registers that are always present? - PCI IDE has the same registers, but the BAR must be mapped and PCI IO space access must be enabled? - ISA IDE has a hardcoded ISA irq number? - PCI IDE has a normal PCI irq that is routed like any legacy PCI INTx irq? - What combinations of ISA enabled/disabled and PCI enabled/disabled need to be supported? Stefan
On 20/05/2021 09:35, Stefan Hajnoczi wrote: > I realized I don't really understand how ISA IDE and PCI IDE interact in > PIIX3: > > - ISA IDE has well-known PIO registers that are always present? > > - PCI IDE has the same registers, but the BAR must be mapped and PCI IO > space access must be enabled? > > - ISA IDE has a hardcoded ISA irq number? > > - PCI IDE has a normal PCI irq that is routed like any legacy PCI INTx > irq? > > - What combinations of ISA enabled/disabled and PCI enabled/disabled > need to be supported? Yeah a lot of this discussion happened several months back in the Pegasos threads, but here is my understanding: - Older legacy PCI devices such as IDE controllers connected via a host containing a PCI-ISA bridge can be switched by the guest OS into PCI legacy (also known as compatibility mode) via a PCI config space register so that IO space accesses, IRQs (and possible DMA?) are done via the ISA bus - QEMU handles the IO memory accesses fine, since in these cases isa_bus_new() is given the IO space by pci_address_space_io(dev) so IO space access generally "just works" - Currently it is the responsibility of these older PCI devices to determine how they have been configured and either use e.g. pci_set_irq() or qemu_raise_irq() on the ISA IRQ for interrupts - Generally ISA IRQs are fixed as per the old AT-style PCs so IDE would be 14/15 My thoughts above were about how to allow a PCIDevice to locate its ISABus if it is connected to a bus with a PCI-ISA bridge to potentially allow access to ISA IRQs and DMA if configured in PCI legacy mode. ATB, Mark.
On Thu, 20 May 2021, Mark Cave-Ayland wrote: > On 20/05/2021 09:35, Stefan Hajnoczi wrote: >> I realized I don't really understand how ISA IDE and PCI IDE interact in >> PIIX3: You're not alone with that. :-) >> - ISA IDE has well-known PIO registers that are always present? >> >> - PCI IDE has the same registers, but the BAR must be mapped and PCI IO >> space access must be enabled? >> >> - ISA IDE has a hardcoded ISA irq number? >> >> - PCI IDE has a normal PCI irq that is routed like any legacy PCI INTx >> irq? >> >> - What combinations of ISA enabled/disabled and PCI enabled/disabled >> need to be supported? > > Yeah a lot of this discussion happened several months back in the Pegasos > threads, but here is my understanding: > > - Older legacy PCI devices such as IDE controllers connected via a host > containing a PCI-ISA bridge can be switched by the guest OS into PCI legacy > (also known as compatibility mode) via a PCI config space register so that IO > space accesses, IRQs (and possible DMA?) are done via the ISA bus Maybe you can look at the VIA VT82C686B and VT8231 docs that have some info on how this works for these integrated "super south bridges" (superio + PCI bridge). The concept for PIIX may be similar but registers may be different. There are at least two modes: a legacy mode that uses normal ISA IDE ioports and IRQs so that older drivers work without change and some native mode that may be full PCI mode with BARs and PCI irq-s or some strange non-100& native mode (as Linux calls it) on some systems such as pegasos2 where in this mode port addresses can be set indepently but IRQs are still hard coded to use ISA IRQs regardless of what the documented IRQ reg is set to. (I'm still not sure how this is implemented in hardware but that's how guests expect it to work and this caused some problem with implementing this as another machine using via-ide (the MIPS fuloong2e) has either legacy or a real native mode with IRQ also set by a register (it's still not a PCI IRQ I think as an ISA IRQ is selected by this register but instead of the legacy IRQ 14+15 in this mode it's using a single interrupt for both channels set by a reg e.g. 9 while normal PCI interrupts may be connected somewhere else). On pegasos2 where setting this IRQ reg does not change the IRQ 14+15 mapping, there native mode only changes ports to use configured port numbers instead of the legacy 1f0-170 ones but keeping the legacy ISA IRQs. We have to model this otherwise guests don't work because they often expect things to work a certain way without checking. Maybe the IDE in these integrated south bridges are not really PCI IDE but in native mode behaves more or less like a real PCI IDE card so we just reusing the QEMU PCI IDE model to emulate them but we also need to emulate the quirks of their native mode in some cases. Currently we likely only emulate one of the possible modes that work with the guests and not fully emulate all modes due to ISA model not being QOM that can be added or removed on demand so if we set it up in the beginning then we're stuck with legacy mode as we can't really disable the legacy io ports any more to switch to native mode without hacking into ISA emulation. (A similar problem was also found with other superio devices in the VIA south bridge, such as serial, parallel, FDC, that also have configurable io ports but we can't emulate that as ISA superio devices can only be created with port addresses but these addresses cannot be set later. Fortunately guests set it up once at startup and usually don't change the default so if we put it there it works.) > - QEMU handles the IO memory accesses fine, since in these cases > isa_bus_new() is given the IO space by pci_address_space_io(dev) so IO space > access generally "just works" > > - Currently it is the responsibility of these older PCI devices to determine > how they have been configured and either use e.g. pci_set_irq() or > qemu_raise_irq() on the ISA IRQ for interrupts This is probably OK considering that these IDE device can be in different modes and probably the only part that knows which mode it's in is the device itself so it has to determine what IRQ to use. But as in the via-ide case the modes (and thus IRQs used) can be different based on which south bridge or machine it's used in so maybe it should be the higher level object (south bridge or machine) which instantiates via-ide that decides which irq to use. So I wonder if it would be possible to remove the decision of using pci_set_irq or using an ISA irq from via-ide and only pass it a qemu_irq that it can raise without caring where it's connected and the south bridge or machine that creates via-ide could then pass it an appropriate irq (PCI or ISA based on how it's configured). This seems the simplest way but due to the current entanglement of IRQ handling in the different models it's not clear to me how to implement this or if it's possible at all. > - Generally ISA IRQs are fixed as per the old AT-style PCs so IDE would be > 14/15 > > My thoughts above were about how to allow a PCIDevice to locate its ISABus if > it is connected to a bus with a PCI-ISA bridge to potentially allow access to > ISA IRQs and DMA if configured in PCI legacy mode. In my opinion a PCI device should have no knowledge about ISA at all, it's probably the south bridge that uses this PCI IDE device that should connect it to ISA as that's the one that knows about ISA bus or ISA IRQs. I'm a bit concerned about the performance of your proposed pci_device_get_isabus() function that walks the PCI bus to get an ISA bus to get an ISA IRQ. Do you really want to do that every time an IRQ is raised which can be quite frequent? I think it would be better to find the IRQ once when the PCI IDE device is set up in it's parent object that also already has a reference to ISA bus and just pass the IRQ to the IDE device removing the need for it to know about ISA. I think that's a better direction to go but don't know how to get there. As a side note, in my understanding the main problem with fully emulating these south bridges is that ISA emulation predates everything and it's not fully QOM-ified so a lot of it still uses global vars and legacy init functions that allow creating these ISA devices but not changing them afterwards in any way. This prevents cleanly modelling south bridges that can switch between legacy and PCI mode for IDE for example because we can create legacy IDE ports but then we cannot switch tnem off to use BARs instead when the mode change so without ovethauling the ISA emulation we can only emulate one mode or the other. I've stumbled upon this for via-ide and VT8231 serial and decided I don't want to try cleaning up ISA as it's a basic device class that could break a lot of things so I did not feel like wanting to attempt that. This patch set from Philippe tries to go a bit further in that direction but maybe not all the way for the same reason that it's a big task with a lot of potential breakage. So I'm OK with leaving it as it is now as it works well enough or make small clean ups as possible without breaking too many things. If this introduces an ISABus link in PCI IDE I can live with that knowing that it's to avoid more extensive changes or adding new subclasses but if there's a simpler way by passing IRQs directly and it could be done that seems to be a cleaner way to me. Regards, BALATON Zoltan
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h index d8384e1c422..e790722ed14 100644 --- a/include/hw/ide/pci.h +++ b/include/hw/ide/pci.h @@ -47,6 +47,7 @@ struct PCIIDEState { PCIDevice parent_obj; /*< public >*/ + ISABus *isa_bus; IDEBus bus[2]; BMDMAState bmdma[2]; uint32_t secondary; /* used only for cmd646 */ diff --git a/hw/ide/piix.c b/hw/ide/piix.c index b9860e35a5c..48da68da37f 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -30,8 +30,9 @@ #include "sysemu/block-backend.h" #include "sysemu/blockdev.h" #include "sysemu/dma.h" - +#include "qapi/error.h" #include "hw/ide/pci.h" +#include "hw/isa/isa.h" #include "trace.h" static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size) @@ -207,6 +208,12 @@ static void pci_piix_ide_exitfn(PCIDevice *dev) } } +static Property piix_ide_properties[] = { + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, + TYPE_ISA_BUS, ISABus *), + DEFINE_PROP_END_OF_LIST(), +}; + /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */ static void piix3_ide_class_init(ObjectClass *klass, void *data) { @@ -221,6 +228,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_STORAGE_IDE; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); dc->hotpluggable = false; + device_class_set_props(dc, piix_ide_properties); } static const TypeInfo piix3_ide_info = { @@ -249,6 +257,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_STORAGE_IDE; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); dc->hotpluggable = false; + device_class_set_props(dc, piix_ide_properties); } static const TypeInfo piix4_ide_info = { diff --git a/hw/ide/via.c b/hw/ide/via.c index be09912b334..65fdca6dcf4 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -28,8 +28,9 @@ #include "hw/pci/pci.h" #include "migration/vmstate.h" #include "qemu/module.h" +#include "qapi/error.h" #include "sysemu/dma.h" - +#include "hw/isa/isa.h" #include "hw/ide/pci.h" #include "trace.h" @@ -210,6 +211,12 @@ static void via_ide_exitfn(PCIDevice *dev) } } +static Property via_ide_properties[] = { + DEFINE_PROP_LINK("isa-bus", PCIIDEState, isa_bus, + TYPE_ISA_BUS, ISABus *), + DEFINE_PROP_END_OF_LIST(), +}; + static void via_ide_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -224,6 +231,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data) k->revision = 0x06; k->class_id = PCI_CLASS_STORAGE_IDE; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); + device_class_set_props(dc, via_ide_properties); } static const TypeInfo via_ide_info = {
IDE bus depends on ISA bus for IRQ/DMA. Add an ISABus reference in PCIIDEState, and add link properties to it in the PIIX and VIA objects (which inherit PCI_IDE). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/hw/ide/pci.h | 1 + hw/ide/piix.c | 11 ++++++++++- hw/ide/via.c | 10 +++++++++- 3 files changed, 20 insertions(+), 2 deletions(-)