1. Patchew
  2. QEMU
  3. [PATCH 00/13] Clean up PCI IDE device models
  4. View patch

[PATCH 07/13] hw/ide: Extract pci_ide_{cmd, data}_le_ops initialization into base class constructor

Bernhard Beschow posted 13 patches 2 years, 9 months ago
Maintainers: John Snow <jsnow@redhat.com>, BALATON Zoltan <balaton@eik.bme.hu>, Huacai Chen <chenhuacai@kernel.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>
[PATCH 07/13] hw/ide: Extract pci_ide_{cmd, data}_le_ops initialization into base class constructor
Posted by Bernhard Beschow 2 years, 9 months ago 
There is redundant code in cmd646 and via which can be extracted into the base
class. In case of piix and sii3112 this is currently unneccessary but shouldn't
interfere since the memory regions aren't mapped by those devices. In few
commits later this will be changed, i.e. those device models will also make use
of these memory regions.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ide/cmd646.c | 11 -----------
 hw/ide/pci.c    | 10 ++++++++++
 hw/ide/via.c    | 11 -----------
 3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 6fd09fe74e..85716aaf17 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -251,20 +251,9 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     dev->wmask[MRDMODE] = 0x0;
     dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
 
-    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
-                          &d->bus[0], "cmd646-data0", 8);
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
-
-    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
-                          &d->bus[0], "cmd646-cmd0", 4);
     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
-
-    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
-                          &d->bus[1], "cmd646-data1", 8);
     pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
-
-    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
-                          &d->bus[1], "cmd646-cmd1", 4);
     pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
 
     bmdma_init_ops(d, &cmd646_bmdma_ops);
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 65ed6f7f72..a9194313bd 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -543,6 +543,16 @@ 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,
+                          &d->bus[0], "pci-ide0-data-ops", 8);
+    memory_region_init_io(&d->cmd_bar[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,
+                          &d->bus[1], "pci-ide1-data-ops", 8);
+    memory_region_init_io(&d->cmd_bar[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));
 }
 
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 40704e2857..704a8024cb 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -154,20 +154,9 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     dev->wmask[PCI_INTERRUPT_LINE] = 0;
     dev->wmask[PCI_CLASS_PROG] = 5;
 
-    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
-                          &d->bus[0], "via-ide0-data", 8);
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
-
-    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
-                          &d->bus[0], "via-ide0-cmd", 4);
     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
-
-    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
-                          &d->bus[1], "via-ide1-data", 8);
     pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
-
-    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
-                          &d->bus[1], "via-ide1-cmd", 4);
     pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
 
     bmdma_init_ops(d, &via_bmdma_ops);
-- 
2.40.0
Re: [PATCH 07/13] hw/ide: Extract pci_ide_{cmd, data}_le_ops initialization into base class constructor
Posted by Mark Cave-Ayland 2 years, 9 months ago 
On 22/04/2023 16:07, Bernhard Beschow wrote:

> There is redundant code in cmd646 and via which can be extracted into the base
> class. In case of piix and sii3112 this is currently unneccessary but shouldn't
> interfere since the memory regions aren't mapped by those devices. In few
> commits later this will be changed, i.e. those device models will also make use
> of these memory regions.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ide/cmd646.c | 11 -----------
>   hw/ide/pci.c    | 10 ++++++++++
>   hw/ide/via.c    | 11 -----------
>   3 files changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index 6fd09fe74e..85716aaf17 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -251,20 +251,9 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>       dev->wmask[MRDMODE] = 0x0;
>       dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
>   
> -    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
> -                          &d->bus[0], "cmd646-data0", 8);
>       pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
> -
> -    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
> -                          &d->bus[0], "cmd646-cmd0", 4);
>       pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
> -
> -    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
> -                          &d->bus[1], "cmd646-data1", 8);
>       pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
> -
> -    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
> -                          &d->bus[1], "cmd646-cmd1", 4);
>       pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
>   
>       bmdma_init_ops(d, &cmd646_bmdma_ops);
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 65ed6f7f72..a9194313bd 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -543,6 +543,16 @@ 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,
> +                          &d->bus[0], "pci-ide0-data-ops", 8);
> +    memory_region_init_io(&d->cmd_bar[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,
> +                          &d->bus[1], "pci-ide1-data-ops", 8);
> +    memory_region_init_io(&d->cmd_bar[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));
>   }
>   
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 40704e2857..704a8024cb 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -154,20 +154,9 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>       dev->wmask[PCI_INTERRUPT_LINE] = 0;
>       dev->wmask[PCI_CLASS_PROG] = 5;
>   
> -    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
> -                          &d->bus[0], "via-ide0-data", 8);
>       pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
> -
> -    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
> -                          &d->bus[0], "via-ide0-cmd", 4);
>       pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
> -
> -    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
> -                          &d->bus[1], "via-ide1-data", 8);
>       pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
> -
> -    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
> -                          &d->bus[1], "via-ide1-cmd", 4);
>       pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
>   
>       bmdma_init_ops(d, &via_bmdma_ops);

I'd also be inclined to agree with Phil/Zoltan re: dropping the trailing "-ops" in 
the name, otherwise:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.
Re: [PATCH 07/13] hw/ide: Extract pci_ide_{cmd,data}_le_ops initialization into base class constructor
Posted by Philippe Mathieu-Daudé 2 years, 9 months ago 
On 22/4/23 17:07, Bernhard Beschow wrote:
> There is redundant code in cmd646 and via which can be extracted into the base
> class. In case of piix and sii3112 this is currently unneccessary but shouldn't
> interfere since the memory regions aren't mapped by those devices. In few
> commits later this will be changed, i.e. those device models will also make use
> of these memory regions.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ide/cmd646.c | 11 -----------
>   hw/ide/pci.c    | 10 ++++++++++
>   hw/ide/via.c    | 11 -----------
>   3 files changed, 10 insertions(+), 22 deletions(-)


> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 65ed6f7f72..a9194313bd 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -543,6 +543,16 @@ 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,
> +                          &d->bus[0], "pci-ide0-data-ops", 8);
> +    memory_region_init_io(&d->cmd_bar[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,
> +                          &d->bus[1], "pci-ide1-data-ops", 8);
> +    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
> +                          &d->bus[1], "pci-ide1-cmd-ops", 4);

The trailing "-ops" just adds noise IMO.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Re: [PATCH 07/13] hw/ide: Extract pci_ide_{cmd,data}_le_ops initialization into base class constructor
Posted by Bernhard Beschow 2 years, 9 months ago 

Am 23. April 2023 17:46:18 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 22/4/23 17:07, Bernhard Beschow wrote:
>> There is redundant code in cmd646 and via which can be extracted into the base
>> class. In case of piix and sii3112 this is currently unneccessary but shouldn't
>> interfere since the memory regions aren't mapped by those devices. In few
>> commits later this will be changed, i.e. those device models will also make use
>> of these memory regions.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ide/cmd646.c | 11 -----------
>>   hw/ide/pci.c    | 10 ++++++++++
>>   hw/ide/via.c    | 11 -----------
>>   3 files changed, 10 insertions(+), 22 deletions(-)
>
>
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 65ed6f7f72..a9194313bd 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -543,6 +543,16 @@ 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,
>> +                          &d->bus[0], "pci-ide0-data-ops", 8);
>> +    memory_region_init_io(&d->cmd_bar[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,
>> +                          &d->bus[1], "pci-ide1-data-ops", 8);
>> +    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
>> +                          &d->bus[1], "pci-ide1-cmd-ops", 4);
>
>The trailing "-ops" just adds noise IMO.

I'll remove them in the next iteration.

Best regards,
Bernhard

>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.