diff mbox series

[08/13] hw/ide: Rename PCIIDEState::*_bar attributes

Message ID 20230422150728.176512-9-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series Clean up PCI IDE device models | expand

Commit Message

Bernhard Beschow April 22, 2023, 3:07 p.m. UTC 
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(-)

Comments

BALATON Zoltan April 22, 2023, 5:53 p.m. UTC | #1 
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++) {
>
Mark Cave-Ayland April 26, 2023, 11:21 a.m. UTC | #2 
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.
Bernhard Beschow April 26, 2023, 6:29 p.m. UTC | #3 
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.
Mark Cave-Ayland April 27, 2023, 11:07 a.m. UTC | #4 
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 mbox series

Patch

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++) {