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