[06/13] hw/ide: Extract bmdma_init_ops()

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(-)

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

On 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.