[PATCH 0/1] hw/ide: share bmdma read and write functions

[PATCH 0/1] hw/ide: share bmdma read and write functions

[Liav Albani]

This is a preparation before I send v3 of ich6-ide controller emulation patch.
I figured that it's more trivial to split the changes this way, by extracting
the bmdma functions from via.c and piix.c and sharing them together. Then,
I could easily put these into use when I send v3 of the ich6-ide patch by just
using the already separated functions. This was suggested by BALATON Zoltan when
he submitted a code review on my ich6-ide controller emulation patch.

Liav Albani (1):
  hw/ide: share bmdma read and write functions between piix.c and via.c

 hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
 hw/ide/piix.c        | 50 ++-----------------------------------------
 hw/ide/via.c         | 51 ++------------------------------------------
 include/hw/ide/pci.h |  4 ++++
 4 files changed, 55 insertions(+), 97 deletions(-)

-- 
2.35.1

[PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[Liav Albani]

Instead of letting each implementation to duplicate this code, we can
share these functions between IDE PIIX3/4 and VIA implementations.

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
 hw/ide/piix.c        | 50 ++-----------------------------------------
 hw/ide/via.c         | 51 ++------------------------------------------
 include/hw/ide/pci.h |  4 ++++
 4 files changed, 55 insertions(+), 97 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 84ba733548..c8b867659a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
     .reset = bmdma_reset,
 };
 
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+                           unsigned size)
+{
+    BMDMAState *bm = opaque;
+    uint32_t val;
+
+    if (size != 1) {
+        return ((uint64_t)1 << (size * 8)) - 1;
+    }
+
+    switch (addr & 3) {
+    case 0:
+        val = bm->cmd;
+        break;
+    case 2:
+        val = bm->status;
+        break;
+    default:
+        val = 0xff;
+        break;
+    }
+
+    trace_bmdma_read_via(addr, val);
+    return val;
+}
+
+void bmdma_default_write(void *opaque, hwaddr addr,
+                        uint64_t val, unsigned size)
+{
+    BMDMAState *bm = opaque;
+
+    if (size != 1) {
+        return;
+    }
+
+    trace_bmdma_write_via(addr, val);
+    switch (addr & 3) {
+    case 0:
+        bmdma_cmd_writeb(bm, val);
+        break;
+    case 2:
+        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
+        break;
+    default:;
+    }
+}
+
 void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
 {
     if (bus->dma == &bm->dma) {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..fdf3a04cb1 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -35,55 +35,9 @@
 #include "hw/ide/pci.h"
 #include "trace.h"
 
-static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
-{
-    BMDMAState *bm = opaque;
-    uint32_t val;
-
-    if (size != 1) {
-        return ((uint64_t)1 << (size * 8)) - 1;
-    }
-
-    switch(addr & 3) {
-    case 0:
-        val = bm->cmd;
-        break;
-    case 2:
-        val = bm->status;
-        break;
-    default:
-        val = 0xff;
-        break;
-    }
-
-    trace_bmdma_read(addr, val);
-    return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
-                        uint64_t val, unsigned size)
-{
-    BMDMAState *bm = opaque;
-
-    if (size != 1) {
-        return;
-    }
-
-    trace_bmdma_write(addr, val);
-
-    switch(addr & 3) {
-    case 0:
-        bmdma_cmd_writeb(bm, val);
-        break;
-    case 2:
-        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
-        break;
-    }
-}
-
 static const MemoryRegionOps piix_bmdma_ops = {
-    .read = bmdma_read,
-    .write = bmdma_write,
+    .read = bmdma_default_read,
+    .write = bmdma_default_write,
 };
 
 static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 82def819c4..13f27c9514 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -33,56 +33,9 @@
 #include "hw/ide/pci.h"
 #include "trace.h"
 
-static uint64_t bmdma_read(void *opaque, hwaddr addr,
-                           unsigned size)
-{
-    BMDMAState *bm = opaque;
-    uint32_t val;
-
-    if (size != 1) {
-        return ((uint64_t)1 << (size * 8)) - 1;
-    }
-
-    switch (addr & 3) {
-    case 0:
-        val = bm->cmd;
-        break;
-    case 2:
-        val = bm->status;
-        break;
-    default:
-        val = 0xff;
-        break;
-    }
-
-    trace_bmdma_read_via(addr, val);
-    return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
-                        uint64_t val, unsigned size)
-{
-    BMDMAState *bm = opaque;
-
-    if (size != 1) {
-        return;
-    }
-
-    trace_bmdma_write_via(addr, val);
-    switch (addr & 3) {
-    case 0:
-        bmdma_cmd_writeb(bm, val);
-        break;
-    case 2:
-        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
-        break;
-    default:;
-    }
-}
-
 static const MemoryRegionOps via_bmdma_ops = {
-    .read = bmdma_read,
-    .write = bmdma_write,
+    .read = bmdma_default_read,
+    .write = bmdma_default_write,
 };
 
 static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..159136f055 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,10 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
 }
 
 void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+                           unsigned size);
+void bmdma_default_write(void *opaque, hwaddr addr,
+                        uint64_t val, unsigned size);
 void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
 extern MemoryRegionOps bmdma_addr_ioport_ops;
 void pci_ide_create_devs(PCIDevice *dev);
-- 
2.35.1

Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[BALATON Zoltan]

On Sat, 19 Feb 2022, Liav Albani wrote:
> Instead of letting each implementation to duplicate this code, we can
> share these functions between IDE PIIX3/4 and VIA implementations.

OK but there's a way to take this even further as cmd646 also uses similar 
functions just with more cases so you could remove the cases handled by 
these functions and only leave the difference and call the default 
function from the default case. E.g. (untested, just to show the idea):

hw/ide/cmd646.c:
static uint64_t bmdma_read(void *opaque, hwaddr addr,
                            unsigned size)
{
     BMDMAState *bm = opaque;
     PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
     uint32_t val;

     if (size != 1) {
         return ((uint64_t)1 << (size * 8)) - 1;
     }

     switch(addr & 3) {
     case 1:
         val = pci_dev->config[MRDMODE];
         break;
     case 3:
         if (bm == &bm->pci_dev->bmdma[0]) {
             val = pci_dev->config[UDIDETCR0];
         } else {
             val = pci_dev->config[UDIDETCR1];
         }
         break;
     default:
         val = bmdma_default_read(opaque, addr, size);
         break;
     }

     trace_bmdma_read_cmd646(addr, val);
     return val;
}

> Signed-off-by: Liav Albani <liavalb@gmail.com>
> ---
> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
> hw/ide/piix.c        | 50 ++-----------------------------------------
> hw/ide/via.c         | 51 ++------------------------------------------
> include/hw/ide/pci.h |  4 ++++
> 4 files changed, 55 insertions(+), 97 deletions(-)
>
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 84ba733548..c8b867659a 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
>     .reset = bmdma_reset,
> };
>
> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
> +                           unsigned size)

Indentation off? Also everywhere else, usually we indent not with the 
parenthesis but with the list within. (Auto indent in most editors does 
that probably.)

Regards,
BALATON Zoltan

> +{
> +    BMDMAState *bm = opaque;
> +    uint32_t val;
> +
> +    if (size != 1) {
> +        return ((uint64_t)1 << (size * 8)) - 1;
> +    }
> +
> +    switch (addr & 3) {
> +    case 0:
> +        val = bm->cmd;
> +        break;
> +    case 2:
> +        val = bm->status;
> +        break;
> +    default:
> +        val = 0xff;
> +        break;
> +    }
> +
> +    trace_bmdma_read_via(addr, val);
> +    return val;
> +}
> +
> +void bmdma_default_write(void *opaque, hwaddr addr,
> +                        uint64_t val, unsigned size)
> +{
> +    BMDMAState *bm = opaque;
> +
> +    if (size != 1) {
> +        return;
> +    }
> +
> +    trace_bmdma_write_via(addr, val);
> +    switch (addr & 3) {
> +    case 0:
> +        bmdma_cmd_writeb(bm, val);
> +        break;
> +    case 2:
> +        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
> +        break;
> +    default:;
> +    }
> +}
> +
> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
> {
>     if (bus->dma == &bm->dma) {
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index ce89fd0aa3..fdf3a04cb1 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -35,55 +35,9 @@
> #include "hw/ide/pci.h"
> #include "trace.h"
>
> -static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
> -{
> -    BMDMAState *bm = opaque;
> -    uint32_t val;
> -
> -    if (size != 1) {
> -        return ((uint64_t)1 << (size * 8)) - 1;
> -    }
> -
> -    switch(addr & 3) {
> -    case 0:
> -        val = bm->cmd;
> -        break;
> -    case 2:
> -        val = bm->status;
> -        break;
> -    default:
> -        val = 0xff;
> -        break;
> -    }
> -
> -    trace_bmdma_read(addr, val);
> -    return val;
> -}
> -
> -static void bmdma_write(void *opaque, hwaddr addr,
> -                        uint64_t val, unsigned size)
> -{
> -    BMDMAState *bm = opaque;
> -
> -    if (size != 1) {
> -        return;
> -    }
> -
> -    trace_bmdma_write(addr, val);
> -
> -    switch(addr & 3) {
> -    case 0:
> -        bmdma_cmd_writeb(bm, val);
> -        break;
> -    case 2:
> -        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
> -        break;
> -    }
> -}
> -
> static const MemoryRegionOps piix_bmdma_ops = {
> -    .read = bmdma_read,
> -    .write = bmdma_write,
> +    .read = bmdma_default_read,
> +    .write = bmdma_default_write,
> };
>
> static void bmdma_setup_bar(PCIIDEState *d)
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 82def819c4..13f27c9514 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -33,56 +33,9 @@
> #include "hw/ide/pci.h"
> #include "trace.h"
>
> -static uint64_t bmdma_read(void *opaque, hwaddr addr,
> -                           unsigned size)
> -{
> -    BMDMAState *bm = opaque;
> -    uint32_t val;
> -
> -    if (size != 1) {
> -        return ((uint64_t)1 << (size * 8)) - 1;
> -    }
> -
> -    switch (addr & 3) {
> -    case 0:
> -        val = bm->cmd;
> -        break;
> -    case 2:
> -        val = bm->status;
> -        break;
> -    default:
> -        val = 0xff;
> -        break;
> -    }
> -
> -    trace_bmdma_read_via(addr, val);
> -    return val;
> -}
> -
> -static void bmdma_write(void *opaque, hwaddr addr,
> -                        uint64_t val, unsigned size)
> -{
> -    BMDMAState *bm = opaque;
> -
> -    if (size != 1) {
> -        return;
> -    }
> -
> -    trace_bmdma_write_via(addr, val);
> -    switch (addr & 3) {
> -    case 0:
> -        bmdma_cmd_writeb(bm, val);
> -        break;
> -    case 2:
> -        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
> -        break;
> -    default:;
> -    }
> -}
> -
> static const MemoryRegionOps via_bmdma_ops = {
> -    .read = bmdma_read,
> -    .write = bmdma_write,
> +    .read = bmdma_default_read,
> +    .write = bmdma_default_write,
> };
>
> static void bmdma_setup_bar(PCIIDEState *d)
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..159136f055 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -62,6 +62,10 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
> }
>
> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
> +                           unsigned size);
> +void bmdma_default_write(void *opaque, hwaddr addr,
> +                        uint64_t val, unsigned size);
> void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
> extern MemoryRegionOps bmdma_addr_ioport_ops;
> void pci_ide_create_devs(PCIDevice *dev);
>

Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[Liav Albani]

On 2/19/22 13:19, BALATON Zoltan wrote:
> On Sat, 19 Feb 2022, Liav Albani wrote:
>> Instead of letting each implementation to duplicate this code, we can
>> share these functions between IDE PIIX3/4 and VIA implementations.
>
> OK but there's a way to take this even further as cmd646 also uses 
> similar functions just with more cases so you could remove the cases 
> handled by these functions and only leave the difference and call the 
> default function from the default case. E.g. (untested, just to show 
> the idea):
>
> hw/ide/cmd646.c:
> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                            unsigned size)
> {
>     BMDMAState *bm = opaque;
>     PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>     uint32_t val;
>
>     if (size != 1) {
>         return ((uint64_t)1 << (size * 8)) - 1;
>     }
>
>     switch(addr & 3) {
>     case 1:
>         val = pci_dev->config[MRDMODE];
>         break;
>     case 3:
>         if (bm == &bm->pci_dev->bmdma[0]) {
>             val = pci_dev->config[UDIDETCR0];
>         } else {
>             val = pci_dev->config[UDIDETCR1];
>         }
>         break;
>     default:
>         val = bmdma_default_read(opaque, addr, size);
>         break;
>     }
>
>     trace_bmdma_read_cmd646(addr, val);
>     return val;
> }
>
Yeah, I see how I can do that for both bmdma write and read of cmd646. 
I'll send a v2 right away with a fix.
>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>> ---
>> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
>> hw/ide/piix.c        | 50 ++-----------------------------------------
>> hw/ide/via.c         | 51 ++------------------------------------------
>> include/hw/ide/pci.h |  4 ++++
>> 4 files changed, 55 insertions(+), 97 deletions(-)
>>
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index 84ba733548..c8b867659a 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
>>     .reset = bmdma_reset,
>> };
>>
>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>> +                           unsigned size)
>
> Indentation off? Also everywhere else, usually we indent not with the 
> parenthesis but with the list within. (Auto indent in most editors 
> does that probably.)
>
I guess you mean that it should be:

+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+                                                unsigned size)

Like this?

I'm using Visual Studio Code, so I might not have the correct settings 
for this editor with QEMU.
The checkpatch script doesn't complain on style issues, so what can I do 
to make this correct?

Best regards,
Liav

> Regards,
> BALATON Zoltan

Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]

On Sat, 19 Feb 2022, Liav Albani wrote:
> On 2/19/22 13:19, BALATON Zoltan wrote:
>> On Sat, 19 Feb 2022, Liav Albani wrote:
>>> Instead of letting each implementation to duplicate this code, we can
>>> share these functions between IDE PIIX3/4 and VIA implementations.
>> 
>> OK but there's a way to take this even further as cmd646 also uses similar 
>> functions just with more cases so you could remove the cases handled by 
>> these functions and only leave the difference and call the default function 
>> from the default case. E.g. (untested, just to show the idea):
>> 
>> hw/ide/cmd646.c:
>> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>>                            unsigned size)
>> {
>>     BMDMAState *bm = opaque;
>>     PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>>     uint32_t val;
>> 
>>     if (size != 1) {
>>         return ((uint64_t)1 << (size * 8)) - 1;
>>     }
>> 
>>     switch(addr & 3) {
>>     case 1:
>>         val = pci_dev->config[MRDMODE];
>>         break;
>>     case 3:
>>         if (bm == &bm->pci_dev->bmdma[0]) {
>>             val = pci_dev->config[UDIDETCR0];
>>         } else {
>>             val = pci_dev->config[UDIDETCR1];
>>         }
>>         break;
>>     default:
>>         val = bmdma_default_read(opaque, addr, size);
>>         break;
>>     }
>> 
>>     trace_bmdma_read_cmd646(addr, val);
>>     return val;
>> }
>> 
> Yeah, I see how I can do that for both bmdma write and read of cmd646. I'll 
> send a v2 right away with a fix.

Maybe even the first if that's already contained in the default function 
could be avoided with some reorganisation like

if (size == 1) {
     remaining switch cases to set val
} else {
     val = bmdma_default_read();
}

but I wasn't sure that won't change anything so may need a bit more 
thought.

>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>> ---
>>> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
>>> hw/ide/piix.c        | 50 ++-----------------------------------------
>>> hw/ide/via.c         | 51 ++------------------------------------------
>>> include/hw/ide/pci.h |  4 ++++
>>> 4 files changed, 55 insertions(+), 97 deletions(-)
>>> 
>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>> index 84ba733548..c8b867659a 100644
>>> --- a/hw/ide/pci.c
>>> +++ b/hw/ide/pci.c
>>> @@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
>>>     .reset = bmdma_reset,
>>> };
>>> 
>>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>> +                           unsigned size)
>> 
>> Indentation off? Also everywhere else, usually we indent not with the 
>> parenthesis but with the list within. (Auto indent in most editors does 
>> that probably.)
>> 
> I guess you mean that it should be:
>
> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
> +                                                unsigned size)
>
> Like this?

No, like the code you've moved had it. The split line should start after 
the ( not on the same column. So:

uint64_t bmdma_default_read(void *opaque, hwaddr addr,
                            unsigned size)

but this line does not need to be split at all as it fits within 80 chars 
so better to keep it one line and only split where needed.

> I'm using Visual Studio Code, so I might not have the correct settings for 
> this editor with QEMU.
> The checkpatch script doesn't complain on style issues, so what can I do to 
> make this correct?

If checkpatch is happy then probably not a problem but just look at how 
code is indented on other places and follow the same. The coding style doc 
may have some words on it too. I don't know what setting Visual Studio 
might need.

Regards,
BALATON Zoltan

Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4707 bytes --]

On Sat, 19 Feb 2022, BALATON Zoltan wrote:
> On Sat, 19 Feb 2022, Liav Albani wrote:
>> On 2/19/22 13:19, BALATON Zoltan wrote:
>>> On Sat, 19 Feb 2022, Liav Albani wrote:
>>>> Instead of letting each implementation to duplicate this code, we can
>>>> share these functions between IDE PIIX3/4 and VIA implementations.
>>> 
>>> OK but there's a way to take this even further as cmd646 also uses similar 
>>> functions just with more cases so you could remove the cases handled by 
>>> these functions and only leave the difference and call the default 
>>> function from the default case. E.g. (untested, just to show the idea):
>>> 
>>> hw/ide/cmd646.c:
>>> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>>>                            unsigned size)
>>> {
>>>     BMDMAState *bm = opaque;
>>>     PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>>>     uint32_t val;
>>> 
>>>     if (size != 1) {
>>>         return ((uint64_t)1 << (size * 8)) - 1;
>>>     }
>>> 
>>>     switch(addr & 3) {
>>>     case 1:
>>>         val = pci_dev->config[MRDMODE];
>>>         break;
>>>     case 3:
>>>         if (bm == &bm->pci_dev->bmdma[0]) {
>>>             val = pci_dev->config[UDIDETCR0];
>>>         } else {
>>>             val = pci_dev->config[UDIDETCR1];
>>>         }
>>>         break;
>>>     default:
>>>         val = bmdma_default_read(opaque, addr, size);
>>>         break;
>>>     }
>>> 
>>>     trace_bmdma_read_cmd646(addr, val);
>>>     return val;
>>> }
>>> 
>> Yeah, I see how I can do that for both bmdma write and read of cmd646. I'll 
>> send a v2 right away with a fix.
>
> Maybe even the first if that's already contained in the default function 
> could be avoided with some reorganisation like
>
> if (size == 1) {
>    remaining switch cases to set val
> } else {
>    val = bmdma_default_read();
> }

On second thought this misses the cases where size == 1 and addr is those 
handeled in bmdma_default_read so one would still need the default case in 
the if part and then it's not much better than duplicating the if. Maybe 
calling the default first, then handling the remaining cases, like

val = bmdma_default_read();
if (size == 1) {
     remaining switch cases to set val
}
return val;

is the simplest and avoids the duplicated if. (Now we only have two trace 
messages instead of one but that's probably not a problem as it's only a 
debugging aid.

Regards.
BALATON Zoltan

> but I wasn't sure that won't change anything so may need a bit more thought.
>
>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>>> ---
>>>> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
>>>> hw/ide/piix.c        | 50 ++-----------------------------------------
>>>> hw/ide/via.c         | 51 ++------------------------------------------
>>>> include/hw/ide/pci.h |  4 ++++
>>>> 4 files changed, 55 insertions(+), 97 deletions(-)
>>>> 
>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>>> index 84ba733548..c8b867659a 100644
>>>> --- a/hw/ide/pci.c
>>>> +++ b/hw/ide/pci.c
>>>> @@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
>>>>     .reset = bmdma_reset,
>>>> };
>>>> 
>>>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>>> +                           unsigned size)
>>> 
>>> Indentation off? Also everywhere else, usually we indent not with the 
>>> parenthesis but with the list within. (Auto indent in most editors does 
>>> that probably.)
>>> 
>> I guess you mean that it should be:
>> 
>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>> +                                                unsigned size)
>> 
>> Like this?
>
> No, like the code you've moved had it. The split line should start after the 
> ( not on the same column. So:
>
> uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>                             unsigned size)
>
> but this line does not need to be split at all as it fits within 80 chars so 
> better to keep it one line and only split where needed.
>
>> I'm using Visual Studio Code, so I might not have the correct settings for 
>> this editor with QEMU.
>> The checkpatch script doesn't complain on style issues, so what can I do to 
>> make this correct?
>
> If checkpatch is happy then probably not a problem but just look at how code 
> is indented on other places and follow the same. The coding style doc may 
> have some words on it too. I don't know what setting Visual Studio might 
> need.
>
> Regards,
> BALATON Zoltan

Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[Liav Albani]

On 2/19/22 17:57, BALATON Zoltan wrote:
> On Sat, 19 Feb 2022, BALATON Zoltan wrote:
>> Maybe even the first if that's already contained in the default 
>> function could be avoided with some reorganisation like
>>
>> if (size == 1) {
>>    remaining switch cases to set val
>> } else {
>>    val = bmdma_default_read();
>> }
>
> On second thought this misses the cases where size == 1 and addr is 
> those handeled in bmdma_default_read so one would still need the 
> default case in the if part and then it's not much better than 
> duplicating the if. Maybe calling the default first, then handling the 
> remaining cases, like
>
> val = bmdma_default_read();
> if (size == 1) {
>     remaining switch cases to set val
> }
> return val;
>
> is the simplest and avoids the duplicated if. (Now we only have two 
> trace messages instead of one but that's probably not a problem as 
> it's only a debugging aid.
>
Hmm, is it OK though to have two trace messages? I'm not against it, but 
if someone tries to use the debug messages to see what's going on, it's 
better to not have two of the same message as it will confuse people. We 
definitely don't want that to happen.

So, let's keep it simple (and remove code duplications) but also let's 
do that correctly, to ensure that in the view of the developer that uses 
the debug messages, it all seem clear and neat :)

>
>> but I wasn't sure that won't change anything so may need a bit more 
>> thought.
>>
>>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>>>> ---
>>>>> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
>>>>> hw/ide/piix.c        | 50 ++-----------------------------------------
>>>>> hw/ide/via.c         | 51 
>>>>> ++------------------------------------------
>>>>> include/hw/ide/pci.h |  4 ++++
>>>>> 4 files changed, 55 insertions(+), 97 deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>>>> index 84ba733548..c8b867659a 100644
>>>>> --- a/hw/ide/pci.c
>>>>> +++ b/hw/ide/pci.c
>>>>> @@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
>>>>>     .reset = bmdma_reset,
>>>>> };
>>>>>
>>>>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>>>> +                           unsigned size)
>>>>
>>>> Indentation off? Also everywhere else, usually we indent not with 
>>>> the parenthesis but with the list within. (Auto indent in most 
>>>> editors does that probably.)
>>>>
>>> I guess you mean that it should be:
>>>
>>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>> +                                                unsigned size)
>>>
>>> Like this?
>>
>> No, like the code you've moved had it. The split line should start 
>> after the ( not on the same column. So:
>>
>> uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>                             unsigned size)
>>
>> but this line does not need to be split at all as it fits within 80 
>> chars so better to keep it one line and only split where needed.
>>
>>> I'm using Visual Studio Code, so I might not have the correct 
>>> settings for this editor with QEMU.
>>> The checkpatch script doesn't complain on style issues, so what can 
>>> I do to make this correct?
>>
>> If checkpatch is happy then probably not a problem but just look at 
>> how code is indented on other places and follow the same. The coding 
>> style doc may have some words on it too. I don't know what setting 
>> Visual Studio might need.

OK. I'll align it to the character after the start of the parenthesis. 
I'll take a look into other code snippets in QEMU, but at least in the 
IDE code, there are lots of code style issues (the checkpatch script 
says so) so I'll probably look into other parts of QEMU to see how it goes.

I'll take this a bit slow, as I wanted to send v2 today. This is 
probably not a good idea as we should let people to see this and maybe 
to let the maintainer (John) to look into this and put his comments on 
this too. I'll give this a couple of days - no rush here, although I'd 
be very happy to see things going forward as soon as possible, so we 
merge this patch and then going back to the ich6-ide patch, and then 
hopefully more patches to the IDE code to add more features and fixes in 
this part of QEMU codebase.

Thanks again for the suggestions, these are awesome and I really 
appreciate the effort you put into these!

Best regards,
Liav

Re: [PATCH 1/1] hw/ide: share bmdma read and write functions between piix.c and via.c

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 5836 bytes --]

On Sat, 19 Feb 2022, Liav Albani wrote:
> On 2/19/22 17:57, BALATON Zoltan wrote:
>> On Sat, 19 Feb 2022, BALATON Zoltan wrote:
>>> Maybe even the first if that's already contained in the default function 
>>> could be avoided with some reorganisation like
>>> 
>>> if (size == 1) {
>>>    remaining switch cases to set val
>>> } else {
>>>    val = bmdma_default_read();
>>> }
>> 
>> On second thought this misses the cases where size == 1 and addr is those 
>> handeled in bmdma_default_read so one would still need the default case in 
>> the if part and then it's not much better than duplicating the if. Maybe 
>> calling the default first, then handling the remaining cases, like
>> 
>> val = bmdma_default_read();
>> if (size == 1) {
>>     remaining switch cases to set val
>> }
>> return val;
>> 
>> is the simplest and avoids the duplicated if. (Now we only have two trace 
>> messages instead of one but that's probably not a problem as it's only a 
>> debugging aid.
>> 
> Hmm, is it OK though to have two trace messages? I'm not against it, but if 
> someone tries to use the debug messages to see what's going on, it's better 
> to not have two of the same message as it will confuse people. We definitely 
> don't want that to happen.

I don't think this is a problem. One message is from the bmdma default 
function and one is from the cmd646 specific function overriding it so 
both may have some usefulness and one can enable/disable them separately 
so for cmd646 one can only enable the specific one to avoid duplicated 
messages.

> So, let's keep it simple (and remove code duplications) but also let's do 
> that correctly, to ensure that in the view of the developer that uses the 
> debug messages, it all seem clear and neat :)

I don't see an easy way to avoid that otherwise because we can't move the 
trace out from the default functions because some controllers only use 
that so the simplest is to not care about this.

>> 
>>> but I wasn't sure that won't change anything so may need a bit more 
>>> thought.
>>> 
>>>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>>>>> ---
>>>>>> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
>>>>>> hw/ide/piix.c        | 50 ++-----------------------------------------
>>>>>> hw/ide/via.c         | 51 ++------------------------------------------
>>>>>> include/hw/ide/pci.h |  4 ++++
>>>>>> 4 files changed, 55 insertions(+), 97 deletions(-)
>>>>>> 
>>>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>>>>>> index 84ba733548..c8b867659a 100644
>>>>>> --- a/hw/ide/pci.c
>>>>>> +++ b/hw/ide/pci.c
>>>>>> @@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
>>>>>>     .reset = bmdma_reset,
>>>>>> };
>>>>>> 
>>>>>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>>>>> +                           unsigned size)
>>>>> 
>>>>> Indentation off? Also everywhere else, usually we indent not with the 
>>>>> parenthesis but with the list within. (Auto indent in most editors does 
>>>>> that probably.)
>>>>> 
>>>> I guess you mean that it should be:
>>>> 
>>>> +uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>>> +                                                unsigned size)
>>>> 
>>>> Like this?
>>> 
>>> No, like the code you've moved had it. The split line should start after 
>>> the ( not on the same column. So:
>>> 
>>> uint64_t bmdma_default_read(void *opaque, hwaddr addr,
>>>                             unsigned size)
>>> 
>>> but this line does not need to be split at all as it fits within 80 chars 
>>> so better to keep it one line and only split where needed.
>>> 
>>>> I'm using Visual Studio Code, so I might not have the correct settings 
>>>> for this editor with QEMU.
>>>> The checkpatch script doesn't complain on style issues, so what can I do 
>>>> to make this correct?
>>> 
>>> If checkpatch is happy then probably not a problem but just look at how 
>>> code is indented on other places and follow the same. The coding style doc 
>>> may have some words on it too. I don't know what setting Visual Studio 
>>> might need.
>
> OK. I'll align it to the character after the start of the parenthesis. I'll 
> take a look into other code snippets in QEMU, but at least in the IDE code, 
> there are lots of code style issues (the checkpatch script says so) so I'll 
> probably look into other parts of QEMU to see how it goes.

Some parts of QEMU has some old code not yet following the latest coding 
style. Checkpatch usually tells you and if it's a few lines it's OK to 
correct it in the part you touch to silence checkpatch. Otherwise a 
separate code style fix patch before the modifications could also be 
included to avoid such warnings during later changes. I think this is 
explained in the patch submission guidelines.

> I'll take this a bit slow, as I wanted to send v2 today. This is probably not 
> a good idea as we should let people to see this and maybe to let the 
> maintainer (John) to look into this and put his comments on this too. I'll 
> give this a couple of days - no rush here, although I'd be very happy to see 
> things going forward as soon as possible, so we merge this patch and then 
> going back to the ich6-ide patch, and then hopefully more patches to the IDE 
> code to add more features and fixes in this part of QEMU codebase.

The only constraint is if you want this in the next release or it's ok to 
postpone it further. See the planning page about the schedule: 
https://wiki.qemu.org/Planning/7.0 which means if it's not in a pull 
request until March 8 then it will miss 7.0 and you'll have plenty of time 
to get the remaining changes polished until development opens again in 
second half of April.

Regards,
BALATON Zoltan

Re: [PATCH 0/1] hw/ide: share bmdma read and write functions

[Bernhard Beschow]

Am 19. Februar 2022 08:08:17 UTC schrieb Liav Albani <liavalb@gmail.com>:
>This is a preparation before I send v3 of ich6-ide controller emulation patch.
>I figured that it's more trivial to split the changes this way, by extracting
>the bmdma functions from via.c and piix.c and sharing them together. Then,
>I could easily put these into use when I send v3 of the ich6-ide patch by just
>using the already separated functions. This was suggested by BALATON Zoltan when
>he submitted a code review on my ich6-ide controller emulation patch.

Ping. Any news?

>Liav Albani (1):
>  hw/ide: share bmdma read and write functions between piix.c and via.c
>
> hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
> hw/ide/piix.c        | 50 ++-----------------------------------------
> hw/ide/via.c         | 51 ++------------------------------------------
> include/hw/ide/pci.h |  4 ++++
> 4 files changed, 55 insertions(+), 97 deletions(-)
>

Re: [PATCH 0/1] hw/ide: share bmdma read and write functions

[John Snow]

On Tue, Sep 6, 2022 at 10:27 AM Bernhard Beschow <shentey@gmail.com> wrote:
>
> Am 19. Februar 2022 08:08:17 UTC schrieb Liav Albani <liavalb@gmail.com>:
> >This is a preparation before I send v3 of ich6-ide controller emulation patch.
> >I figured that it's more trivial to split the changes this way, by extracting
> >the bmdma functions from via.c and piix.c and sharing them together. Then,
> >I could easily put these into use when I send v3 of the ich6-ide patch by just
> >using the already separated functions. This was suggested by BALATON Zoltan when
> >he submitted a code review on my ich6-ide controller emulation patch.
>
> Ping. Any news?

*cough*.

Has this been folded into subsequent series, or does this still need attention?

>
> >Liav Albani (1):
> >  hw/ide: share bmdma read and write functions between piix.c and via.c
> >
> > hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
> > hw/ide/piix.c        | 50 ++-----------------------------------------
> > hw/ide/via.c         | 51 ++------------------------------------------
> > include/hw/ide/pci.h |  4 ++++
> > 4 files changed, 55 insertions(+), 97 deletions(-)
> >
>

Re: [PATCH 0/1] hw/ide: share bmdma read and write functions

[Bernhard Beschow]

Am 9. Januar 2023 19:24:16 UTC schrieb John Snow <jsnow@redhat.com>:
>On Tue, Sep 6, 2022 at 10:27 AM Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> Am 19. Februar 2022 08:08:17 UTC schrieb Liav Albani <liavalb@gmail.com>:
>> >This is a preparation before I send v3 of ich6-ide controller emulation patch.
>> >I figured that it's more trivial to split the changes this way, by extracting
>> >the bmdma functions from via.c and piix.c and sharing them together. Then,
>> >I could easily put these into use when I send v3 of the ich6-ide patch by just
>> >using the already separated functions. This was suggested by BALATON Zoltan when
>> >he submitted a code review on my ich6-ide controller emulation patch.
>>
>> Ping. Any news?
>
>*cough*.
>
>Has this been folded into subsequent series, or does this still need attention?

Both piix and via still have their own bmdma implementations. This patch might be worth having.

Best regards,
Bernhard

>
>>
>> >Liav Albani (1):
>> >  hw/ide: share bmdma read and write functions between piix.c and via.c
>> >
>> > hw/ide/pci.c         | 47 ++++++++++++++++++++++++++++++++++++++++
>> > hw/ide/piix.c        | 50 ++-----------------------------------------
>> > hw/ide/via.c         | 51 ++------------------------------------------
>> > include/hw/ide/pci.h |  4 ++++
>> > 4 files changed, 55 insertions(+), 97 deletions(-)
>> >
>>
>

Re: [PATCH 0/1] hw/ide: share bmdma read and write functions

[John Snow]

On Fri, Jan 13, 2023 at 9:10 AM Liav Albani <liavalb@gmail.com> wrote:
>
>
> On 1/11/23 01:07, Bernhard Beschow wrote:
>
> Am 9. Januar 2023 19:24:16 UTC schrieb John Snow <jsnow@redhat.com>:
>
> On Tue, Sep 6, 2022 at 10:27 AM Bernhard Beschow <shentey@gmail.com> wrote:
>
> Am 19. Februar 2022 08:08:17 UTC schrieb Liav Albani <liavalb@gmail.com>:
>
> This is a preparation before I send v3 of ich6-ide controller emulation patch.
> I figured that it's more trivial to split the changes this way, by extracting
> the bmdma functions from via.c and piix.c and sharing them together. Then,
> I could easily put these into use when I send v3 of the ich6-ide patch by just
> using the already separated functions. This was suggested by BALATON Zoltan when
> he submitted a code review on my ich6-ide controller emulation patch.
>
> Ping. Any news?
>
> *cough*.
>
> Has this been folded into subsequent series, or does this still need attention?
>
> Both piix and via still have their own bmdma implementations. This patch might be worth having.
>
> Best regards,
> Bernhard
>
> I see. Since you are still interested, I will try to see what was the outcome of that patch as I really don't remember if it passed the CI tests, etc. If applicable, I will send this as v2, or if it's already approved, then I guess we could just let it be merged to the tree?
>

I was just going to run some smoke tests on it and as long as it
didn't hurt anything, I'd wave it in. If you want it alongside other
patches that I also should stage, you can bundle them if you'd like.
Just let me know what you plan on doing.

--js