From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tu6eX-00029B-Eg for qemu-devel@nongnu.org; Sat, 12 Jan 2013 14:22:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tu6eU-0004QC-0e for qemu-devel@nongnu.org; Sat, 12 Jan 2013 14:22:01 -0500 Received: from cantor2.suse.de ([195.135.220.15]:53737 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tu6eT-0004Q0-JI for qemu-devel@nongnu.org; Sat, 12 Jan 2013 14:21:57 -0500 Message-ID: <50F1B7CD.1020100@suse.de> Date: Sat, 12 Jan 2013 20:21:49 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1357334986-13941-1-git-send-email-hpoussin@reactos.org> <1357334986-13941-9-git-send-email-hpoussin@reactos.org> In-Reply-To: <1357334986-13941-9-git-send-email-hpoussin@reactos.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 08/10] isa: use memory regions instead of portio_list_* functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?SGVydsOpIFBvdXNzaW5lYXU=?= Cc: Kevin Wolf , Blue Swirl , Stefan Hajnoczi , qemu-devel@nongnu.org, Markus Armbruster , Julien Grall , malc , Gerd Hoffmann , Anthony Liguori Am 04.01.2013 22:29, schrieb Herv=C3=A9 Poussineau: > Signed-off-by: Herv=C3=A9 Poussineau > --- > hw/isa-bus.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++++= ++++++-- > hw/isa.h | 2 +- > 2 files changed, 125 insertions(+), 4 deletions(-) >=20 > diff --git a/hw/isa-bus.c b/hw/isa-bus.c > index 86b0bbd..bcf7cd4 100644 > --- a/hw/isa-bus.c > +++ b/hw/isa-bus.c > @@ -104,19 +104,140 @@ void isa_register_ioport(ISADevice *dev, MemoryR= egion *io, uint16_t start) > isa_init_ioport(dev, start); > } > =20 > +typedef struct PortioState { > + const char *name; /* debug purposes */ > + uint16_t start; > + uint16_t offset; > + const MemoryRegionPortio *pio_start; > + void *opaque; > +} PortioState; > + > +static const MemoryRegionPortio *portio_find(const MemoryRegionPortio = *mrp, > + uint64_t offset, > + unsigned int width, bool = write, > + bool smaller) > +{ > + for (; mrp->size; ++mrp) { > + if (offset >=3D mrp->offset && offset < mrp->offset + mrp->len > + && (width =3D=3D mrp->size || (smaller && width < mrp->siz= e)) > + && (write ? (bool)mrp->write : (bool)mrp->read)) { > + return mrp; > + } > + } > + return NULL; > +} > + > +static uint64_t portio_read(void *opaque, hwaddr addr, unsigned int si= ze) > +{ > + const PortioState *s =3D opaque; > + const MemoryRegionPortio *mrp; > + > + addr +=3D s->offset; > + mrp =3D portio_find(s->pio_start, addr, size, false, false); > + if (mrp) { > + return mrp->read(s->opaque, s->start + addr); > + } else if (size =3D=3D 2) { > + uint64_t data; > + mrp =3D portio_find(s->pio_start, addr, 1, false, false); > + assert(mrp); > + data =3D mrp->read(s->opaque, s->start + addr) | > + (mrp->read(s->opaque, s->start + addr + 1) << 8); > + return data; > + } > + qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid read from 0x%x size=3D= %d", > + s->name, s->start + (int)addr, size); I note that patch 10/10 shows that memory.c does it similarly, assuming Little Endian for size =3D=3D 2 and ignoring assembled size =3D=3D 4. > + return -1U; > +} > + > +static void portio_write(void *opaque, hwaddr addr, uint64_t data, > + unsigned int size) > +{ > + const PortioState *s =3D opaque; > + const MemoryRegionPortio *mrp; > + > + addr +=3D s->offset; > + mrp =3D portio_find(s->pio_start, addr, size, true, false); > + if (mrp) { > + mrp->write(s->opaque, s->start + addr, data); > + return; > + } else if (size =3D=3D 2) { > + mrp =3D portio_find(s->pio_start, addr, 1, true, false); > + assert(mrp); > + mrp->write(s->opaque, s->start + addr, data & 0xff); > + mrp->write(s->opaque, s->start + addr + 1, data >> 8); > + return; > + } > + qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid write to 0x%x size=3D%= d", > + s->name, s->start + (int)addr, size); Ditto. > +} > + > +static bool portio_accepts(void *opaque, hwaddr addr, unsigned int siz= e, > + bool is_write) > +{ > + const PortioState *s =3D opaque; > + const MemoryRegionPortio *mrp; > + > + addr +=3D s->offset; > + mrp =3D portio_find(s->pio_start, addr, size, is_write, true); > + return (mrp !=3D NULL); > +} > + > +const MemoryRegionOps portio_ops =3D { static const? > + .read =3D portio_read, > + .write =3D portio_write, > + .valid.accepts =3D portio_accepts, > + .endianness =3D DEVICE_LITTLE_ENDIAN, > +}; > + > +static void isa_register_portio_list_1(ISADevice *dev, uint16_t start, > + uint16_t offset, uint16_t end, > + const MemoryRegionPortio *pio_s= tart, > + void *opaque, const char *name) > +{ > + MemoryRegion *mr =3D g_new(MemoryRegion, 1); > + PortioState *s =3D g_new(PortioState, 1); > + > + s->name =3D name; > + s->start =3D start; > + s->offset =3D offset; > + s->pio_start =3D pio_start; > + s->opaque =3D opaque; > + memory_region_init_io(mr, &portio_ops, s, name, end - offset); > + memory_region_add_subregion(isa_address_space_io(dev), > + start + offset, mr); > +} > + > void isa_register_portio_list(ISADevice *dev, uint16_t start, > const MemoryRegionPortio *pio_start, > void *opaque, const char *name) > { > - PortioList *piolist =3D g_new(PortioList, 1); > + const MemoryRegionPortio *pio, *first; > + uint16_t end; > =20 > /* START is how we should treat DEV, regardless of the actual > contents of the portio array. This is how the old code > actually handled e.g. the FDC device. */ > isa_init_ioport(dev, start); > =20 > - portio_list_init(piolist, pio_start, opaque, name); > - portio_list_add(piolist, isabus->address_space_io, start); > + assert(pio_start->size); > + > + first =3D pio_start; > + end =3D 0; > + for (pio =3D pio_start; pio->size; pio++) { > + assert(pio->offset >=3D first->offset); > + if (pio->offset > first->offset + first->len) { > + isa_register_portio_list_1(dev, start, first->offset, end, > + pio_start, opaque, name); > + first =3D pio; > + end =3D 0; > + } > + if (pio->offset + pio->len > end) { > + end =3D pio->offset + pio->len; > + } > + } > + > + isa_register_portio_list_1(dev, start, first->offset, end, > + pio_start, opaque, name); > } > =20 > static int isa_qdev_init(DeviceState *qdev) > diff --git a/hw/isa.h b/hw/isa.h > index 62e89d3..c3b01ea 100644 > --- a/hw/isa.h > +++ b/hw/isa.h > @@ -73,7 +73,7 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion= *io, uint16_t start); > * @dev: the ISADevice against which these are registered; may be NULL= . > * @start: the base I/O port against which the portio->offset is appli= ed. > * @portio: the ports, sorted by offset. > - * @opaque: passed into the old_portio callbacks. > + * @opaque: passed into the portio callbacks. > * @name: passed into memory_region_init_io. > */ > void isa_register_portio_list(ISADevice *dev, uint16_t start, Otherwise looks okay to me, but I'd appreciate another pair of eyes. AFAIU this moves the current implementation from ioport.c (09/10) and memory.c (10/10) into isa-bus.c, to remove MemoryRegionOps::old_portio in 10/10. But ISA keeps using MemoryRegionPortio unlike memory.c itself. I wonder if it were not a cleaner course of action to remove isa_register_portio_list() completely by converting callers to use isa_register_ioport() with a MemoryRegion owned by the caller? Remaining MemoryRegionPortio lists after this series below. Regards, Andreas dma.c: /* IOport from page_base */ static const MemoryRegionPortio page_portio_list[] =3D { { 0x01, 3, 1, .write =3D write_page, .read =3D read_page, }, { 0x07, 1, 1, .write =3D write_page, .read =3D read_page, }, PORTIO_END_OF_LIST(), }; /* IOport from pageh_base */ static const MemoryRegionPortio pageh_portio_list[] =3D { { 0x01, 3, 1, .write =3D write_pageh, .read =3D read_pageh, }, { 0x07, 3, 1, .write =3D write_pageh, .read =3D read_pageh, }, PORTIO_END_OF_LIST(), }; fdc.c: static const MemoryRegionPortio fdc_portio_list[] =3D { { 1, 5, 1, .read =3D fdctrl_read, .write =3D fdctrl_write }, { 7, 1, 1, .read =3D fdctrl_read, .write =3D fdctrl_write }, PORTIO_END_OF_LIST(), }; gus.c: static const MemoryRegionPortio gus_portio_list1[] =3D { {0x000, 1, 1, .write =3D gus_writeb }, {0x000, 1, 2, .write =3D gus_writew }, {0x006, 10, 1, .read =3D gus_readb, .write =3D gus_writeb }, {0x006, 10, 2, .read =3D gus_readw, .write =3D gus_writew }, {0x100, 8, 1, .read =3D gus_readb, .write =3D gus_writeb }, {0x100, 8, 2, .read =3D gus_readw, .write =3D gus_writew }, PORTIO_END_OF_LIST (), }; static const MemoryRegionPortio gus_portio_list2[] =3D { {0, 1, 1, .read =3D gus_readb }, {0, 1, 2, .read =3D gus_readw }, PORTIO_END_OF_LIST (), }; ide/core.c: static const MemoryRegionPortio ide_portio_list[] =3D { { 0, 8, 1, .read =3D ide_ioport_read, .write =3D ide_ioport_write }, { 0, 2, 2, .read =3D ide_data_readw, .write =3D ide_data_writew }, { 0, 4, 4, .read =3D ide_data_readl, .write =3D ide_data_writel }, PORTIO_END_OF_LIST(), }; static const MemoryRegionPortio ide_portio2_list[] =3D { { 0, 1, 1, .read =3D ide_status_read, .write =3D ide_cmd_write }, PORTIO_END_OF_LIST(), }; parallel.c: static const MemoryRegionPortio isa_parallel_portio_hw_list[] =3D { { 0, 8, 1, .read =3D parallel_ioport_read_hw, .write =3D parallel_ioport_write_hw }, { 4, 1, 2, .read =3D parallel_ioport_eppdata_read_hw2, .write =3D parallel_ioport_eppdata_write_hw2 }, { 4, 1, 4, .read =3D parallel_ioport_eppdata_read_hw4, .write =3D parallel_ioport_eppdata_write_hw4 }, { 0x400, 8, 1, .read =3D parallel_ioport_ecp_read, .write =3D parallel_ioport_ecp_write }, PORTIO_END_OF_LIST(), }; static const MemoryRegionPortio isa_parallel_portio_sw_list[] =3D { { 0, 8, 1, .read =3D parallel_ioport_read_sw, .write =3D parallel_ioport_write_sw }, PORTIO_END_OF_LIST(), }; qxl.c: static const MemoryRegionPortio qxl_vga_portio_list[] =3D { { 0x04, 2, 1, .read =3D vga_ioport_read, .write =3D qxl_vga_ioport_write }, /* 3b4 */ { 0x0a, 1, 1, .read =3D vga_ioport_read, .write =3D qxl_vga_ioport_write }, /* 3ba */ { 0x10, 16, 1, .read =3D vga_ioport_read, .write =3D qxl_vga_ioport_write }, /* 3c0 */ { 0x24, 2, 1, .read =3D vga_ioport_read, .write =3D qxl_vga_ioport_write }, /* 3d4 */ { 0x2a, 1, 1, .read =3D vga_ioport_read, .write =3D qxl_vga_ioport_write }, /* 3da */ PORTIO_END_OF_LIST(), }; sb16.c: static const MemoryRegionPortio sb16_ioport_list[] =3D { { 4, 1, 1, .write =3D mixer_write_indexb }, { 4, 1, 2, .write =3D mixer_write_indexw }, { 5, 1, 1, .read =3D mixer_read, .write =3D mixer_write_datab }, { 6, 1, 1, .read =3D dsp_read, .write =3D dsp_write }, { 10, 1, 1, .read =3D dsp_read }, { 12, 1, 1, .write =3D dsp_write }, { 12, 4, 1, .read =3D dsp_read }, PORTIO_END_OF_LIST (), }; vga.c: static const MemoryRegionPortio vga_portio_list[] =3D { { 0x04, 2, 1, .read =3D vga_ioport_read, .write =3D vga_ioport_write= }, /* 3b4 */ { 0x0a, 1, 1, .read =3D vga_ioport_read, .write =3D vga_ioport_write= }, /* 3ba */ { 0x10, 16, 1, .read =3D vga_ioport_read, .write =3D vga_ioport_write= }, /* 3c0 */ { 0x24, 2, 1, .read =3D vga_ioport_read, .write =3D vga_ioport_write= }, /* 3d4 */ { 0x2a, 1, 1, .read =3D vga_ioport_read, .write =3D vga_ioport_write= }, /* 3da */ PORTIO_END_OF_LIST(), }; static const MemoryRegionPortio vbe_portio_list[] =3D { { 0, 1, 2, .read =3D vbe_ioport_read_index, .write =3D vbe_ioport_write_index }, # ifdef TARGET_I386 { 1, 1, 2, .read =3D vbe_ioport_read_data, .write =3D vbe_ioport_write_data }, # endif { 2, 1, 2, .read =3D vbe_ioport_read_data, .write =3D vbe_ioport_write_data }, PORTIO_END_OF_LIST(), }; --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg