qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Mark Cave-Ayland
Subject: Re: [PATCH 08/13] hw/ide: Rename PCIIDEState::*_bar attributes
Date: Thu, 27 Apr 2023 12:07:32 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 
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.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]