Message ID | 20230422150728.176512-9-shentey@gmail.com |
---|---|
State | New |
Headers | show |
Series | Clean up PCI IDE device models | expand |
On Sat, 22 Apr 2023, Bernhard Beschow wrote: > The attributes represent memory regions containing operations which are mapped > by the device models into PCI BARs. Reflect this by changing the suffic into > "_ops". > > Note that in a few commits piix will also use the {cmd,data}_ops but won't map > them into BARs. This further suggests that the "_bar" suffix doesn't match > very well. I'm not sure about this. Ops is typically used for read/write functions of an io MemeoryRegion while these are typically regions of a PCI IDE contoller that are mapped via BARs so calling them bar looks OK to me and this patch seems to be just code churn so I'd just drop this if there's no good reason to keep it. Regards, BALATON Zoltan > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 6 +++--- > hw/ide/cmd646.c | 10 +++++----- > hw/ide/pci.c | 18 +++++++++--------- > hw/ide/piix.c | 2 +- > hw/ide/via.c | 10 +++++----- > 5 files changed, 23 insertions(+), 23 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 597c77c7ad..5025df5b82 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -51,9 +51,9 @@ struct PCIIDEState { > BMDMAState bmdma[2]; > qemu_irq isa_irq[2]; > uint32_t secondary; /* used only for cmd646 */ > - MemoryRegion bmdma_bar; > - MemoryRegion cmd_bar[2]; > - MemoryRegion data_bar[2]; > + MemoryRegion bmdma_ops; > + MemoryRegion cmd_ops[2]; > + MemoryRegion data_ops[2]; > }; > > void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d); > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index 85716aaf17..b9d005a357 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) > dev->wmask[MRDMODE] = 0x0; > dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1; > > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); > - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); > - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); > - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); > + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); > + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); > + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); > > bmdma_init_ops(d, &cmd646_bmdma_ops); > - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); > > /* TODO: RST# value should be 0 */ > pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1 > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index a9194313bd..b2fcc00a64 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops) > { > size_t i; > > - memory_region_init(&d->bmdma_bar, OBJECT(d), "bmdma-container", 16); > + memory_region_init(&d->bmdma_ops, OBJECT(d), "bmdma-container", 16); > for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { > BMDMAState *bm = &d->bmdma[i]; > > memory_region_init_io(&bm->extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); > + memory_region_add_subregion(&d->bmdma_ops, i * 8, &bm->extra_io); > memory_region_init_io(&bm->addr_ioport, OBJECT(d), &bmdma_addr_ioport_ops, bm, > "bmdma-ioport-ops", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); > + memory_region_add_subregion(&d->bmdma_ops, i * 8 + 4, &bm->addr_ioport); > } > } > > @@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj) > { > PCIIDEState *d = PCI_IDE(obj); > > - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, > + memory_region_init_io(&d->data_ops[0], OBJECT(d), &pci_ide_data_le_ops, > &d->bus[0], "pci-ide0-data-ops", 8); > - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, > + memory_region_init_io(&d->cmd_ops[0], OBJECT(d), &pci_ide_cmd_le_ops, > &d->bus[0], "pci-ide0-cmd-ops", 4); > > - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, > + memory_region_init_io(&d->data_ops[1], OBJECT(d), &pci_ide_data_le_ops, > &d->bus[1], "pci-ide1-data-ops", 8); > - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, > + memory_region_init_io(&d->cmd_ops[1], OBJECT(d), &pci_ide_cmd_le_ops, > &d->bus[1], "pci-ide1-cmd-ops", 4); > > qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq)); > @@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev) > unsigned i; > > for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) { > - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io); > - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport); > + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].extra_io); > + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].addr_ioport); > } > } > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 5611473d37..6942b484f9 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -140,7 +140,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp) > pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode > > bmdma_init_ops(d, &piix_bmdma_ops); > - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); > > for (unsigned i = 0; i < 2; i++) { > if (!pci_piix_init_bus(d, i, errp)) { > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 704a8024cb..35dd97e49b 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -154,13 +154,13 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) > dev->wmask[PCI_INTERRUPT_LINE] = 0; > dev->wmask[PCI_CLASS_PROG] = 5; > > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); > - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); > - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); > - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); > + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); > + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); > + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); > > bmdma_init_ops(d, &via_bmdma_ops); > - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); > > qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus)); > for (i = 0; i < ARRAY_SIZE(d->bus); i++) { >
On 22/04/2023 16:07, Bernhard Beschow wrote: > The attributes represent memory regions containing operations which are mapped > by the device models into PCI BARs. Reflect this by changing the suffic into > "_ops". > > Note that in a few commits piix will also use the {cmd,data}_ops but won't map > them into BARs. This further suggests that the "_bar" suffix doesn't match > very well. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 6 +++--- > hw/ide/cmd646.c | 10 +++++----- > hw/ide/pci.c | 18 +++++++++--------- > hw/ide/piix.c | 2 +- > hw/ide/via.c | 10 +++++----- > 5 files changed, 23 insertions(+), 23 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 597c77c7ad..5025df5b82 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -51,9 +51,9 @@ struct PCIIDEState { > BMDMAState bmdma[2]; > qemu_irq isa_irq[2]; > uint32_t secondary; /* used only for cmd646 */ > - MemoryRegion bmdma_bar; > - MemoryRegion cmd_bar[2]; > - MemoryRegion data_bar[2]; > + MemoryRegion bmdma_ops; > + MemoryRegion cmd_ops[2]; > + MemoryRegion data_ops[2]; > }; > > void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d); > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index 85716aaf17..b9d005a357 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) > dev->wmask[MRDMODE] = 0x0; > dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1; > > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); > - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); > - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); > - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); > + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); > + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); > + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); > > bmdma_init_ops(d, &cmd646_bmdma_ops); > - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); > > /* TODO: RST# value should be 0 */ > pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1 > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index a9194313bd..b2fcc00a64 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops) > { > size_t i; > > - memory_region_init(&d->bmdma_bar, OBJECT(d), "bmdma-container", 16); > + memory_region_init(&d->bmdma_ops, OBJECT(d), "bmdma-container", 16); > for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { > BMDMAState *bm = &d->bmdma[i]; > > memory_region_init_io(&bm->extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); > + memory_region_add_subregion(&d->bmdma_ops, i * 8, &bm->extra_io); > memory_region_init_io(&bm->addr_ioport, OBJECT(d), &bmdma_addr_ioport_ops, bm, > "bmdma-ioport-ops", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); > + memory_region_add_subregion(&d->bmdma_ops, i * 8 + 4, &bm->addr_ioport); > } > } > > @@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj) > { > PCIIDEState *d = PCI_IDE(obj); > > - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, > + memory_region_init_io(&d->data_ops[0], OBJECT(d), &pci_ide_data_le_ops, > &d->bus[0], "pci-ide0-data-ops", 8); > - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, > + memory_region_init_io(&d->cmd_ops[0], OBJECT(d), &pci_ide_cmd_le_ops, > &d->bus[0], "pci-ide0-cmd-ops", 4); > > - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, > + memory_region_init_io(&d->data_ops[1], OBJECT(d), &pci_ide_data_le_ops, > &d->bus[1], "pci-ide1-data-ops", 8); > - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, > + memory_region_init_io(&d->cmd_ops[1], OBJECT(d), &pci_ide_cmd_le_ops, > &d->bus[1], "pci-ide1-cmd-ops", 4); > > qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq)); > @@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev) > unsigned i; > > for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) { > - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io); > - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport); > + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].extra_io); > + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].addr_ioport); > } > } > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 5611473d37..6942b484f9 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -140,7 +140,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp) > pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode > > bmdma_init_ops(d, &piix_bmdma_ops); > - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); > > for (unsigned i = 0; i < 2; i++) { > if (!pci_piix_init_bus(d, i, errp)) { > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 704a8024cb..35dd97e49b 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -154,13 +154,13 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) > dev->wmask[PCI_INTERRUPT_LINE] = 0; > dev->wmask[PCI_CLASS_PROG] = 5; > > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); > - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); > - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); > - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); > + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); > + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); > + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); > > bmdma_init_ops(d, &via_bmdma_ops); > - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); > > qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus)); > for (i = 0; i < ARRAY_SIZE(d->bus); i++) { I don't really feel strongly either way on this one, so I'm happy to go along with the silent majority here - I see that Zoltan has expressed a preference for it to stay as-is. ATB, Mark.
Am 26. April 2023 11:21:28 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >On 22/04/2023 16:07, Bernhard Beschow wrote: > >> The attributes represent memory regions containing operations which are mapped >> by the device models into PCI BARs. Reflect this by changing the suffic into >> "_ops". >> >> Note that in a few commits piix will also use the {cmd,data}_ops but won't map >> them into BARs. This further suggests that the "_bar" suffix doesn't match >> very well. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/ide/pci.h | 6 +++--- >> hw/ide/cmd646.c | 10 +++++----- >> hw/ide/pci.c | 18 +++++++++--------- >> hw/ide/piix.c | 2 +- >> hw/ide/via.c | 10 +++++----- >> 5 files changed, 23 insertions(+), 23 deletions(-) >> >> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >> index 597c77c7ad..5025df5b82 100644 >> --- a/include/hw/ide/pci.h >> +++ b/include/hw/ide/pci.h >> @@ -51,9 +51,9 @@ struct PCIIDEState { >> BMDMAState bmdma[2]; >> qemu_irq isa_irq[2]; >> uint32_t secondary; /* used only for cmd646 */ >> - MemoryRegion bmdma_bar; >> - MemoryRegion cmd_bar[2]; >> - MemoryRegion data_bar[2]; >> + MemoryRegion bmdma_ops; >> + MemoryRegion cmd_ops[2]; >> + MemoryRegion data_ops[2]; >> }; >> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d); >> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >> index 85716aaf17..b9d005a357 100644 >> --- a/hw/ide/cmd646.c >> +++ b/hw/ide/cmd646.c >> @@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) >> dev->wmask[MRDMODE] = 0x0; >> dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1; >> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); >> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); >> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); >> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); >> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); >> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); >> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); >> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); >> bmdma_init_ops(d, &cmd646_bmdma_ops); >> - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); >> + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); >> /* TODO: RST# value should be 0 */ >> pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1 >> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >> index a9194313bd..b2fcc00a64 100644 >> --- a/hw/ide/pci.c >> +++ b/hw/ide/pci.c >> @@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops) >> { >> size_t i; >> - memory_region_init(&d->bmdma_bar, OBJECT(d), "bmdma-container", 16); >> + memory_region_init(&d->bmdma_ops, OBJECT(d), "bmdma-container", 16); >> for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { >> BMDMAState *bm = &d->bmdma[i]; >> memory_region_init_io(&bm->extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4); >> - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); >> + memory_region_add_subregion(&d->bmdma_ops, i * 8, &bm->extra_io); >> memory_region_init_io(&bm->addr_ioport, OBJECT(d), &bmdma_addr_ioport_ops, bm, >> "bmdma-ioport-ops", 4); >> - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); >> + memory_region_add_subregion(&d->bmdma_ops, i * 8 + 4, &bm->addr_ioport); >> } >> } >> @@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj) >> { >> PCIIDEState *d = PCI_IDE(obj); >> - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, >> + memory_region_init_io(&d->data_ops[0], OBJECT(d), &pci_ide_data_le_ops, >> &d->bus[0], "pci-ide0-data-ops", 8); >> - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, >> + memory_region_init_io(&d->cmd_ops[0], OBJECT(d), &pci_ide_cmd_le_ops, >> &d->bus[0], "pci-ide0-cmd-ops", 4); >> - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, >> + memory_region_init_io(&d->data_ops[1], OBJECT(d), &pci_ide_data_le_ops, >> &d->bus[1], "pci-ide1-data-ops", 8); >> - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, >> + memory_region_init_io(&d->cmd_ops[1], OBJECT(d), &pci_ide_cmd_le_ops, >> &d->bus[1], "pci-ide1-cmd-ops", 4); >> qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq)); >> @@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev) >> unsigned i; >> for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) { >> - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io); >> - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport); >> + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].extra_io); >> + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].addr_ioport); >> } >> } >> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >> index 5611473d37..6942b484f9 100644 >> --- a/hw/ide/piix.c >> +++ b/hw/ide/piix.c >> @@ -140,7 +140,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp) >> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode >> bmdma_init_ops(d, &piix_bmdma_ops); >> - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); >> + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); >> for (unsigned i = 0; i < 2; i++) { >> if (!pci_piix_init_bus(d, i, errp)) { >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index 704a8024cb..35dd97e49b 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -154,13 +154,13 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) >> dev->wmask[PCI_INTERRUPT_LINE] = 0; >> dev->wmask[PCI_CLASS_PROG] = 5; >> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); >> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); >> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); >> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); >> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); >> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); >> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); >> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); >> bmdma_init_ops(d, &via_bmdma_ops); >> - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); >> + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); >> qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus)); >> for (i = 0; i < ARRAY_SIZE(d->bus); i++) { > >I don't really feel strongly either way on this one, so I'm happy to go along with the silent majority here - I see that Zoltan has expressed a preference for it to stay as-is. Doesn't it look off in PIIX-IDE where we don't map these regions via BARs? Don't these memory regions only become "BARs" by mapping them as such? > > >ATB, > >Mark.
On 26/04/2023 19:29, Bernhard Beschow wrote: > Am 26. April 2023 11:21:28 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >> On 22/04/2023 16:07, Bernhard Beschow wrote: >> >>> The attributes represent memory regions containing operations which are mapped >>> by the device models into PCI BARs. Reflect this by changing the suffic into >>> "_ops". >>> >>> Note that in a few commits piix will also use the {cmd,data}_ops but won't map >>> them into BARs. This further suggests that the "_bar" suffix doesn't match >>> very well. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> include/hw/ide/pci.h | 6 +++--- >>> hw/ide/cmd646.c | 10 +++++----- >>> hw/ide/pci.c | 18 +++++++++--------- >>> hw/ide/piix.c | 2 +- >>> hw/ide/via.c | 10 +++++----- >>> 5 files changed, 23 insertions(+), 23 deletions(-) >>> >>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>> index 597c77c7ad..5025df5b82 100644 >>> --- a/include/hw/ide/pci.h >>> +++ b/include/hw/ide/pci.h >>> @@ -51,9 +51,9 @@ struct PCIIDEState { >>> BMDMAState bmdma[2]; >>> qemu_irq isa_irq[2]; >>> uint32_t secondary; /* used only for cmd646 */ >>> - MemoryRegion bmdma_bar; >>> - MemoryRegion cmd_bar[2]; >>> - MemoryRegion data_bar[2]; >>> + MemoryRegion bmdma_ops; >>> + MemoryRegion cmd_ops[2]; >>> + MemoryRegion data_ops[2]; >>> }; >>> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d); >>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >>> index 85716aaf17..b9d005a357 100644 >>> --- a/hw/ide/cmd646.c >>> +++ b/hw/ide/cmd646.c >>> @@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) >>> dev->wmask[MRDMODE] = 0x0; >>> dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1; >>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); >>> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); >>> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); >>> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); >>> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); >>> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); >>> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); >>> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); >>> bmdma_init_ops(d, &cmd646_bmdma_ops); >>> - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); >>> + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); >>> /* TODO: RST# value should be 0 */ >>> pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1 >>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>> index a9194313bd..b2fcc00a64 100644 >>> --- a/hw/ide/pci.c >>> +++ b/hw/ide/pci.c >>> @@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops) >>> { >>> size_t i; >>> - memory_region_init(&d->bmdma_bar, OBJECT(d), "bmdma-container", 16); >>> + memory_region_init(&d->bmdma_ops, OBJECT(d), "bmdma-container", 16); >>> for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { >>> BMDMAState *bm = &d->bmdma[i]; >>> memory_region_init_io(&bm->extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4); >>> - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); >>> + memory_region_add_subregion(&d->bmdma_ops, i * 8, &bm->extra_io); >>> memory_region_init_io(&bm->addr_ioport, OBJECT(d), &bmdma_addr_ioport_ops, bm, >>> "bmdma-ioport-ops", 4); >>> - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); >>> + memory_region_add_subregion(&d->bmdma_ops, i * 8 + 4, &bm->addr_ioport); >>> } >>> } >>> @@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj) >>> { >>> PCIIDEState *d = PCI_IDE(obj); >>> - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, >>> + memory_region_init_io(&d->data_ops[0], OBJECT(d), &pci_ide_data_le_ops, >>> &d->bus[0], "pci-ide0-data-ops", 8); >>> - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, >>> + memory_region_init_io(&d->cmd_ops[0], OBJECT(d), &pci_ide_cmd_le_ops, >>> &d->bus[0], "pci-ide0-cmd-ops", 4); >>> - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, >>> + memory_region_init_io(&d->data_ops[1], OBJECT(d), &pci_ide_data_le_ops, >>> &d->bus[1], "pci-ide1-data-ops", 8); >>> - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, >>> + memory_region_init_io(&d->cmd_ops[1], OBJECT(d), &pci_ide_cmd_le_ops, >>> &d->bus[1], "pci-ide1-cmd-ops", 4); >>> qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq)); >>> @@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev) >>> unsigned i; >>> for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) { >>> - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io); >>> - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport); >>> + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].extra_io); >>> + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].addr_ioport); >>> } >>> } >>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >>> index 5611473d37..6942b484f9 100644 >>> --- a/hw/ide/piix.c >>> +++ b/hw/ide/piix.c >>> @@ -140,7 +140,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp) >>> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode >>> bmdma_init_ops(d, &piix_bmdma_ops); >>> - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); >>> + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); >>> for (unsigned i = 0; i < 2; i++) { >>> if (!pci_piix_init_bus(d, i, errp)) { >>> diff --git a/hw/ide/via.c b/hw/ide/via.c >>> index 704a8024cb..35dd97e49b 100644 >>> --- a/hw/ide/via.c >>> +++ b/hw/ide/via.c >>> @@ -154,13 +154,13 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) >>> dev->wmask[PCI_INTERRUPT_LINE] = 0; >>> dev->wmask[PCI_CLASS_PROG] = 5; >>> - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); >>> - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); >>> - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); >>> - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); >>> + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); >>> + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); >>> + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); >>> + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); >>> bmdma_init_ops(d, &via_bmdma_ops); >>> - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); >>> + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); >>> qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus)); >>> for (i = 0; i < ARRAY_SIZE(d->bus); i++) { >> >> I don't really feel strongly either way on this one, so I'm happy to go along with the silent majority here - I see that Zoltan has expressed a preference for it to stay as-is. > > Doesn't it look off in PIIX-IDE where we don't map these regions via BARs? Don't these memory regions only become "BARs" by mapping them as such? Following on from my previous reply, I'm fairly sure we sholdn't be doing this at all for PCI IDE controllers in legacy mode such as PIIX so it shouldn't be an issue. ATB, Mark.
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h index 597c77c7ad..5025df5b82 100644 --- a/include/hw/ide/pci.h +++ b/include/hw/ide/pci.h @@ -51,9 +51,9 @@ struct PCIIDEState { BMDMAState bmdma[2]; qemu_irq isa_irq[2]; uint32_t secondary; /* used only for cmd646 */ - MemoryRegion bmdma_bar; - MemoryRegion cmd_bar[2]; - MemoryRegion data_bar[2]; + MemoryRegion bmdma_ops; + MemoryRegion cmd_ops[2]; + MemoryRegion data_ops[2]; }; void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d); diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 85716aaf17..b9d005a357 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -251,13 +251,13 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) dev->wmask[MRDMODE] = 0x0; dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1; - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); bmdma_init_ops(d, &cmd646_bmdma_ops); - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); /* TODO: RST# value should be 0 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1 diff --git a/hw/ide/pci.c b/hw/ide/pci.c index a9194313bd..b2fcc00a64 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -527,15 +527,15 @@ void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops) { size_t i; - memory_region_init(&d->bmdma_bar, OBJECT(d), "bmdma-container", 16); + memory_region_init(&d->bmdma_ops, OBJECT(d), "bmdma-container", 16); for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { BMDMAState *bm = &d->bmdma[i]; memory_region_init_io(&bm->extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); + memory_region_add_subregion(&d->bmdma_ops, i * 8, &bm->extra_io); memory_region_init_io(&bm->addr_ioport, OBJECT(d), &bmdma_addr_ioport_ops, bm, "bmdma-ioport-ops", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); + memory_region_add_subregion(&d->bmdma_ops, i * 8 + 4, &bm->addr_ioport); } } @@ -543,14 +543,14 @@ static void pci_ide_init(Object *obj) { PCIIDEState *d = PCI_IDE(obj); - memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops, + memory_region_init_io(&d->data_ops[0], OBJECT(d), &pci_ide_data_le_ops, &d->bus[0], "pci-ide0-data-ops", 8); - memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops, + memory_region_init_io(&d->cmd_ops[0], OBJECT(d), &pci_ide_cmd_le_ops, &d->bus[0], "pci-ide0-cmd-ops", 4); - memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops, + memory_region_init_io(&d->data_ops[1], OBJECT(d), &pci_ide_data_le_ops, &d->bus[1], "pci-ide1-data-ops", 8); - memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops, + memory_region_init_io(&d->cmd_ops[1], OBJECT(d), &pci_ide_cmd_le_ops, &d->bus[1], "pci-ide1-cmd-ops", 4); qdev_init_gpio_out(DEVICE(d), d->isa_irq, ARRAY_SIZE(d->isa_irq)); @@ -562,8 +562,8 @@ static void pci_ide_exitfn(PCIDevice *dev) unsigned i; for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) { - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io); - memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport); + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].extra_io); + memory_region_del_subregion(&d->bmdma_ops, &d->bmdma[i].addr_ioport); } } diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 5611473d37..6942b484f9 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -140,7 +140,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp) pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode bmdma_init_ops(d, &piix_bmdma_ops); - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); for (unsigned i = 0; i < 2; i++) { if (!pci_piix_init_bus(d, i, errp)) { diff --git a/hw/ide/via.c b/hw/ide/via.c index 704a8024cb..35dd97e49b 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -154,13 +154,13 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) dev->wmask[PCI_INTERRUPT_LINE] = 0; dev->wmask[PCI_CLASS_PROG] = 5; - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]); - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]); - pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]); - pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); + pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[0]); + pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[0]); + pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_ops[1]); + pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_ops[1]); bmdma_init_ops(d, &via_bmdma_ops); - pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); + pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_ops); qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus)); for (i = 0; i < ARRAY_SIZE(d->bus); i++) {
The attributes represent memory regions containing operations which are mapped by the device models into PCI BARs. Reflect this by changing the suffic into "_ops". Note that in a few commits piix will also use the {cmd,data}_ops but won't map them into BARs. This further suggests that the "_bar" suffix doesn't match very well. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- include/hw/ide/pci.h | 6 +++--- hw/ide/cmd646.c | 10 +++++----- hw/ide/pci.c | 18 +++++++++--------- hw/ide/piix.c | 2 +- hw/ide/via.c | 10 +++++----- 5 files changed, 23 insertions(+), 23 deletions(-)