Re: [Qemu-devel] [PATCH 02/52] ide: split ioport registration to a separate file

[Thomas Huth]

On 2019-01-25 11:06, Paolo Bonzini wrote:
> This is not needed on ARM, and brings in ISA bus code which is otherwise not
> necessary.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/ide/Makefile.objs |  6 ++---
>  hw/ide/core.c        | 25 --------------------
>  hw/ide/ioport.c      | 67 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 70 insertions(+), 28 deletions(-)
>  create mode 100644 hw/ide/ioport.c
> 
> diff --git a/hw/ide/Makefile.objs b/hw/ide/Makefile.objs
> index fc328ff..3f3edd10 100644
> --- a/hw/ide/Makefile.objs
> +++ b/hw/ide/Makefile.objs
> @@ -1,12 +1,12 @@
>  common-obj-$(CONFIG_IDE_CORE) += core.o atapi.o
>  common-obj-$(CONFIG_IDE_QDEV) += qdev.o
> -common-obj-$(CONFIG_IDE_PCI) += pci.o
> -common-obj-$(CONFIG_IDE_ISA) += isa.o
> +common-obj-$(CONFIG_IDE_PCI) += pci.o ioport.o
> +common-obj-$(CONFIG_IDE_ISA) += isa.o ioport.o
>  common-obj-$(CONFIG_IDE_PIIX) += piix.o
>  common-obj-$(CONFIG_IDE_CMD646) += cmd646.o
>  common-obj-$(CONFIG_IDE_MACIO) += macio.o
>  common-obj-$(CONFIG_IDE_MMIO) += mmio.o
> -common-obj-$(CONFIG_IDE_VIA) += via.o
> +common-obj-$(CONFIG_IDE_VIA) += via.o ioport.o
>  common-obj-$(CONFIG_MICRODRIVE) += microdrive.o
>  common-obj-$(CONFIG_AHCI) += ahci.o
>  common-obj-$(CONFIG_AHCI) += ich.o

Since the ioport code clearly depends on ISA, I think you could also
simply do:

common-obj-$(CONFIG_ISA_BUS) += ioport.o

instead of adding it to three different lines here?

Well, the linker should be smart enough to deal with multiple entries,
so it's likely just a matter of taste.

> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c3d779d..8483200 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2686,31 +2686,6 @@ void ide_exit(IDEState *s)
>      qemu_vfree(s->io_buffer);
>  }
>  
> -static const MemoryRegionPortio ide_portio_list[] = {
> -    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
> -    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
> -    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
> -    PORTIO_END_OF_LIST(),
> -};
> -
> -static const MemoryRegionPortio ide_portio2_list[] = {
> -    { 0, 1, 1, .read = ide_status_read, .write = ide_cmd_write },
> -    PORTIO_END_OF_LIST(),
> -};
> -
> -void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> -{
> -    /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
> -       bridge has been setup properly to always register with ISA.  */
> -    isa_register_portio_list(dev, &bus->portio_list,
> -                             iobase, ide_portio_list, bus, "ide");
> -
> -    if (iobase2) {
> -        isa_register_portio_list(dev, &bus->portio2_list,
> -                                 iobase2, ide_portio2_list, bus, "ide");
> -    }
> -}
> -
>  static bool is_identify_set(void *opaque, int version_id)
>  {
>      IDEState *s = opaque;
> diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
> new file mode 100644
> index 0000000..b8f1b3f
> --- /dev/null
> +++ b/hw/ide/ioport.c
> @@ -0,0 +1,67 @@
> +/*
> + * QEMU IDE disk and CD/DVD-ROM Emulator
> + *
> + * Copyright (c) 2003 Fabrice Bellard
> + * Copyright (c) 2006 Openedhand Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/hw.h"
> +#include "hw/isa/isa.h"
> +#include "qemu/error-report.h"
> +#include "qemu/timer.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/dma.h"
> +#include "hw/block/block.h"
> +#include "sysemu/block-backend.h"
> +#include "qapi/error.h"
> +#include "qemu/cutils.h"
> +#include "sysemu/replay.h"
> +
> +#include "hw/ide/internal.h"
> +#include "trace.h"

Maybe shorten the #include list a little bit?

> +static const MemoryRegionPortio ide_portio_list[] = {
> +    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
> +    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
> +    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
> +    PORTIO_END_OF_LIST(),
> +};
> +
> +static const MemoryRegionPortio ide_portio2_list[] = {
> +    { 0, 1, 1, .read = ide_status_read, .write = ide_cmd_write },
> +    PORTIO_END_OF_LIST(),
> +};
> +
> +void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
> +{
> +    /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
> +       bridge has been setup properly to always register with ISA.  */
> +    isa_register_portio_list(dev, &bus->portio_list,
> +                             iobase, ide_portio_list, bus, "ide");
> +
> +    if (iobase2) {
> +        isa_register_portio_list(dev, &bus->portio2_list,
> +                                 iobase2, ide_portio2_list, bus, "ide");
> +    }
> +}

Anyway:

Reviewed-by: Thomas Huth <address@hidden>