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 <pbonzini@redhat.com>
> ---
> 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 <thuth@redhat.com>