There are three private copies of bmdma_setup_bar() with small adaptions.
Consolidate them into one public implementation.
While at it rename the function to bmdma_init_ops() to reflect that the memory
regions being initialized represent BMDMA operations. The actual mapping as a
PCI BAR is still performed separately in each device.
Note that the bmdma_bar attribute will be renamed in a separate commit.
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
include/hw/ide/pci.h | 1 +
hw/ide/cmd646.c | 20 +-------------------
hw/ide/pci.c | 16 ++++++++++++++++
hw/ide/piix.c | 19 +------------------
hw/ide/via.c | 19 +------------------
5 files changed, 20 insertions(+), 55 deletions(-)
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 7bc4e53d02..597c77c7ad 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -57,6 +57,7 @@ struct PCIIDEState {
};
void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
+void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops);
void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
extern MemoryRegionOps bmdma_addr_ioport_ops;
void pci_ide_create_devs(PCIDevice *dev);
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 9aabf80e52..6fd09fe74e 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -161,24 +161,6 @@ static const MemoryRegionOps cmd646_bmdma_ops = {
.write = bmdma_write,
};
-static void bmdma_setup_bar(PCIIDEState *d)
-{
- BMDMAState *bm;
- int i;
-
- memory_region_init(&d->bmdma_bar, OBJECT(d), "cmd646-bmdma", 16);
- for(i = 0;i < 2; i++) {
- bm = &d->bmdma[i];
- memory_region_init_io(&bm->extra_io, OBJECT(d), &cmd646_bmdma_ops, bm,
- "cmd646-bmdma-bus", 4);
- memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
- memory_region_init_io(&bm->addr_ioport, OBJECT(d),
- &bmdma_addr_ioport_ops, bm,
- "cmd646-bmdma-ioport", 4);
- memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
- }
-}
-
static void cmd646_update_irq(PCIDevice *pd)
{
int pci_level;
@@ -285,7 +267,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
&d->bus[1], "cmd646-cmd1", 4);
pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
- bmdma_setup_bar(d);
+ bmdma_init_ops(d, &cmd646_bmdma_ops);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
/* TODO: RST# value should be 0 */
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 8bea92e394..65ed6f7f72 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -523,6 +523,22 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
bm->pci_dev = d;
}
+void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops)
+{
+ size_t i;
+
+ memory_region_init(&d->bmdma_bar, 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_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);
+ }
+}
+
static void pci_ide_init(Object *obj)
{
PCIIDEState *d = PCI_IDE(obj);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 4e6ca99123..5611473d37 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -86,23 +86,6 @@ static const MemoryRegionOps piix_bmdma_ops = {
.write = bmdma_write,
};
-static void bmdma_setup_bar(PCIIDEState *d)
-{
- int i;
-
- memory_region_init(&d->bmdma_bar, OBJECT(d), "piix-bmdma-container", 16);
- for(i = 0;i < 2; i++) {
- BMDMAState *bm = &d->bmdma[i];
-
- memory_region_init_io(&bm->extra_io, OBJECT(d), &piix_bmdma_ops, bm,
- "piix-bmdma", 4);
- memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
- memory_region_init_io(&bm->addr_ioport, OBJECT(d),
- &bmdma_addr_ioport_ops, bm, "bmdma", 4);
- memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
- }
-}
-
static void piix_ide_reset(DeviceState *dev)
{
PCIIDEState *d = PCI_IDE(dev);
@@ -156,7 +139,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
- bmdma_setup_bar(d);
+ bmdma_init_ops(d, &piix_bmdma_ops);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
for (unsigned i = 0; i < 2; i++) {
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 287143a005..40704e2857 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -86,23 +86,6 @@ static const MemoryRegionOps via_bmdma_ops = {
.write = bmdma_write,
};
-static void bmdma_setup_bar(PCIIDEState *d)
-{
- int i;
-
- memory_region_init(&d->bmdma_bar, OBJECT(d), "via-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), &via_bmdma_ops, bm,
- "via-bmdma", 4);
- memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
- memory_region_init_io(&bm->addr_ioport, OBJECT(d),
- &bmdma_addr_ioport_ops, bm, "bmdma", 4);
- memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
- }
-}
-
static void via_ide_set_irq(void *opaque, int n, int level)
{
PCIIDEState *s = opaque;
@@ -187,7 +170,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
&d->bus[1], "via-ide1-cmd", 4);
pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
- bmdma_setup_bar(d);
+ bmdma_init_ops(d, &via_bmdma_ops);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));
--
2.40.0On 22/04/2023 16:07, Bernhard Beschow wrote:
> There are three private copies of bmdma_setup_bar() with small adaptions.
> Consolidate them into one public implementation.
>
> While at it rename the function to bmdma_init_ops() to reflect that the memory
> regions being initialized represent BMDMA operations. The actual mapping as a
> PCI BAR is still performed separately in each device.
>
> Note that the bmdma_bar attribute will be renamed in a separate commit.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/ide/pci.h | 1 +
> hw/ide/cmd646.c | 20 +-------------------
> hw/ide/pci.c | 16 ++++++++++++++++
> hw/ide/piix.c | 19 +------------------
> hw/ide/via.c | 19 +------------------
> 5 files changed, 20 insertions(+), 55 deletions(-)
>
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index 7bc4e53d02..597c77c7ad 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -57,6 +57,7 @@ struct PCIIDEState {
> };
>
> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
> +void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops);
> void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
> extern MemoryRegionOps bmdma_addr_ioport_ops;
> void pci_ide_create_devs(PCIDevice *dev);
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index 9aabf80e52..6fd09fe74e 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -161,24 +161,6 @@ static const MemoryRegionOps cmd646_bmdma_ops = {
> .write = bmdma_write,
> };
>
> -static void bmdma_setup_bar(PCIIDEState *d)
> -{
> - BMDMAState *bm;
> - int i;
> -
> - memory_region_init(&d->bmdma_bar, OBJECT(d), "cmd646-bmdma", 16);
> - for(i = 0;i < 2; i++) {
> - bm = &d->bmdma[i];
> - memory_region_init_io(&bm->extra_io, OBJECT(d), &cmd646_bmdma_ops, bm,
> - "cmd646-bmdma-bus", 4);
> - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
> - memory_region_init_io(&bm->addr_ioport, OBJECT(d),
> - &bmdma_addr_ioport_ops, bm,
> - "cmd646-bmdma-ioport", 4);
> - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
> - }
> -}
> -
> static void cmd646_update_irq(PCIDevice *pd)
> {
> int pci_level;
> @@ -285,7 +267,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
> &d->bus[1], "cmd646-cmd1", 4);
> pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
>
> - bmdma_setup_bar(d);
> + bmdma_init_ops(d, &cmd646_bmdma_ops);
> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>
> /* TODO: RST# value should be 0 */
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 8bea92e394..65ed6f7f72 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -523,6 +523,22 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
> bm->pci_dev = d;
> }
>
> +void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops)
> +{
> + size_t i;
> +
> + memory_region_init(&d->bmdma_bar, 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_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);
> + }
> +}
> +
> static void pci_ide_init(Object *obj)
> {
> PCIIDEState *d = PCI_IDE(obj);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 4e6ca99123..5611473d37 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -86,23 +86,6 @@ static const MemoryRegionOps piix_bmdma_ops = {
> .write = bmdma_write,
> };
>
> -static void bmdma_setup_bar(PCIIDEState *d)
> -{
> - int i;
> -
> - memory_region_init(&d->bmdma_bar, OBJECT(d), "piix-bmdma-container", 16);
> - for(i = 0;i < 2; i++) {
> - BMDMAState *bm = &d->bmdma[i];
> -
> - memory_region_init_io(&bm->extra_io, OBJECT(d), &piix_bmdma_ops, bm,
> - "piix-bmdma", 4);
> - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
> - memory_region_init_io(&bm->addr_ioport, OBJECT(d),
> - &bmdma_addr_ioport_ops, bm, "bmdma", 4);
> - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
> - }
> -}
> -
> static void piix_ide_reset(DeviceState *dev)
> {
> PCIIDEState *d = PCI_IDE(dev);
> @@ -156,7 +139,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
>
> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>
> - bmdma_setup_bar(d);
> + bmdma_init_ops(d, &piix_bmdma_ops);
> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>
> for (unsigned i = 0; i < 2; i++) {
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 287143a005..40704e2857 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -86,23 +86,6 @@ static const MemoryRegionOps via_bmdma_ops = {
> .write = bmdma_write,
> };
>
> -static void bmdma_setup_bar(PCIIDEState *d)
> -{
> - int i;
> -
> - memory_region_init(&d->bmdma_bar, OBJECT(d), "via-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), &via_bmdma_ops, bm,
> - "via-bmdma", 4);
> - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
> - memory_region_init_io(&bm->addr_ioport, OBJECT(d),
> - &bmdma_addr_ioport_ops, bm, "bmdma", 4);
> - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
> - }
> -}
> -
> static void via_ide_set_irq(void *opaque, int n, int level)
> {
> PCIIDEState *s = opaque;
> @@ -187,7 +170,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
> &d->bus[1], "via-ide1-cmd", 4);
> pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
>
> - bmdma_setup_bar(d);
> + bmdma_init_ops(d, &via_bmdma_ops);
> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>
> qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));
Nice! In actual fact, with some more refactoring of the CMD646 device I believe you
could remove the per-implementation ops and move everything into hw/ide/pci.c.
The main reason this is a bit difficult now is because of the "Device specific"
registers intertwined with the BMDMA registers, but there's no reason that CMD646
couldn't manually attach a fallback MemoryRegion to PCIIDEState::bmdma_bar and
implement its device-specific registers there.
Unfortunately this isn't just a cut/paste job because there is also some mirroring of
the BMDMA in PCI configuration space which will need some extra untangling: let's
leave this as-is for now since it makes it easier for a follow-up patch to improve
this later. On that basis:
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
On 22/4/23 17:07, Bernhard Beschow wrote: > There are three private copies of bmdma_setup_bar() with small adaptions. > Consolidate them into one public implementation. > > While at it rename the function to bmdma_init_ops() to reflect that the memory > regions being initialized represent BMDMA operations. The actual mapping as a > PCI BAR is still performed separately in each device. > > Note that the bmdma_bar attribute will be renamed in a separate commit. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 1 + > hw/ide/cmd646.c | 20 +------------------- > hw/ide/pci.c | 16 ++++++++++++++++ > hw/ide/piix.c | 19 +------------------ > hw/ide/via.c | 19 +------------------ > 5 files changed, 20 insertions(+), 55 deletions(-) Nice. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Am 23. April 2023 17:43:22 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 22/4/23 17:07, Bernhard Beschow wrote: >> There are three private copies of bmdma_setup_bar() with small adaptions. >> Consolidate them into one public implementation. >> >> While at it rename the function to bmdma_init_ops() to reflect that the memory >> regions being initialized represent BMDMA operations. The actual mapping as a >> PCI BAR is still performed separately in each device. >> >> Note that the bmdma_bar attribute will be renamed in a separate commit. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/ide/pci.h | 1 + >> hw/ide/cmd646.c | 20 +------------------- >> hw/ide/pci.c | 16 ++++++++++++++++ >> hw/ide/piix.c | 19 +------------------ >> hw/ide/via.c | 19 +------------------ >> 5 files changed, 20 insertions(+), 55 deletions(-) > >Nice. > >Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> I'd rework this patch in the next iteration. I think that most of the memory region initialization can be moved to pci_ide_init(). Unlike realize methods, the nice thing about these instance_init() methods is that the parent implementation is called implicitly rather than being overridden by the child implementation, similar to C++ constructors. This allows for code reuse without much gymnastics. Best regards, Bernhard
© 2016 - 2026 Red Hat, Inc.