| Message ID | 26cea9d1d7137ffbf0d133e0e553b6b79c9d1e6d.1345549695.git.julien.grall@citrix.com |
|---|---|
| State | New |
| Headers | show |
On 2012-08-22 14:27, Julien Grall wrote: > This patch replaces all register_ioport* with portio_*. It permits to > use the new Memory stuff like listener. > > Signed-off-by: Julien Grall <julien.grall@citrix.com> > --- > hw/cirrus_vga.c | 42 ++++++++++++++++++++++++------------------ > 1 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c > index e8dcc6b..adfc855 100644 > --- a/hw/cirrus_vga.c > +++ b/hw/cirrus_vga.c > @@ -200,6 +200,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s, > typedef struct CirrusVGAState { > VGACommonState vga; > > + MemoryRegion cirrus_vga_io; > MemoryRegion cirrus_linear_io; > MemoryRegion cirrus_linear_bitblt_io; > MemoryRegion cirrus_mmio_io; > @@ -2528,12 +2529,15 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr) > return val; > } > > -static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) > +static void cirrus_vga_ioport_write(void *opaque, target_phys_addr_t addr, > + uint64_t val, unsigned size) > { > CirrusVGAState *c = opaque; > VGACommonState *s = &c->vga; > int index; > > + addr += 0x3b0; > + > /* check port range access depending on color/monochrome mode */ > if (vga_ioport_invalid(s, addr)) { > return; > @@ -2657,7 +2661,7 @@ static void cirrus_mmio_write(void *opaque, target_phys_addr_t addr, > if (addr >= 0x100) { > cirrus_mmio_blt_write(s, addr - 0x100, val); > } else { > - cirrus_vga_ioport_write(s, addr + 0x3c0, val); > + cirrus_vga_ioport_write(s, addr + 0x10, val, size); > } > } > > @@ -2783,8 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = { > }, > }; > > +static const MemoryRegionOps cirrus_vga_io_ops = { > + .write = cirrus_vga_ioport_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .impl = { > + .min_access_size = 1, > + .max_access_size = 1, > + }, > +}; > + > static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, > - MemoryRegion *system_memory) > + MemoryRegion *system_memory, > + MemoryRegion *system_io) > { > int i; > static int inited; > @@ -2816,19 +2830,10 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, > s->bustype = CIRRUS_BUSTYPE_ISA; > } > > - register_ioport_write(0x3c0, 16, 1, cirrus_vga_ioport_write, s); > - > - register_ioport_write(0x3b4, 2, 1, cirrus_vga_ioport_write, s); > - register_ioport_write(0x3d4, 2, 1, cirrus_vga_ioport_write, s); > - register_ioport_write(0x3ba, 1, 1, cirrus_vga_ioport_write, s); > - register_ioport_write(0x3da, 1, 1, cirrus_vga_ioport_write, s); > - > - register_ioport_read(0x3c0, 16, 1, cirrus_vga_ioport_read, s); > - > - register_ioport_read(0x3b4, 2, 1, cirrus_vga_ioport_read, s); > - register_ioport_read(0x3d4, 2, 1, cirrus_vga_ioport_read, s); > - register_ioport_read(0x3ba, 1, 1, cirrus_vga_ioport_read, s); > - register_ioport_read(0x3da, 1, 1, cirrus_vga_ioport_read, s); > + /* Register ioport 0x3b0 - 0x3df */ > + memory_region_init_io(&s->cirrus_vga_io, &cirrus_vga_io_ops, s, > + "cirrus-io", 0x30); > + memory_region_add_subregion(system_io, 0x3b0, &s->cirrus_vga_io); Can't imagine that this reflects the original ranges and constraints correctly. Or were they all wrong? Jan
On 08/24/2012 02:44 PM, Jan Kiszka wrote: > On 2012-08-22 14:27, Julien Grall wrote: > >> This patch replaces all register_ioport* with portio_*. It permits to >> use the new Memory stuff like listener. >> >> Signed-off-by: Julien Grall<julien.grall@citrix.com> >> --- >> hw/cirrus_vga.c | 42 ++++++++++++++++++++++++------------------ >> 1 files changed, 24 insertions(+), 18 deletions(-) >> >> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c >> index e8dcc6b..adfc855 100644 >> --- a/hw/cirrus_vga.c >> +++ b/hw/cirrus_vga.c >> @@ -200,6 +200,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s, >> typedef struct CirrusVGAState { >> VGACommonState vga; >> >> + MemoryRegion cirrus_vga_io; >> MemoryRegion cirrus_linear_io; >> MemoryRegion cirrus_linear_bitblt_io; >> MemoryRegion cirrus_mmio_io; >> @@ -2528,12 +2529,15 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr) >> return val; >> } >> >> -static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) >> +static void cirrus_vga_ioport_write(void *opaque, target_phys_addr_t addr, >> + uint64_t val, unsigned size) >> { >> CirrusVGAState *c = opaque; >> VGACommonState *s =&c->vga; >> int index; >> >> + addr += 0x3b0; >> + >> /* check port range access depending on color/monochrome mode */ >> if (vga_ioport_invalid(s, addr)) { >> return; >> @@ -2657,7 +2661,7 @@ static void cirrus_mmio_write(void *opaque, target_phys_addr_t addr, >> if (addr>= 0x100) { >> cirrus_mmio_blt_write(s, addr - 0x100, val); >> } else { >> - cirrus_vga_ioport_write(s, addr + 0x3c0, val); >> + cirrus_vga_ioport_write(s, addr + 0x10, val, size); >> } >> } >> >> @@ -2783,8 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = { >> }, >> }; >> >> +static const MemoryRegionOps cirrus_vga_io_ops = { >> + .write = cirrus_vga_ioport_write, >> + .endianness = DEVICE_LITTLE_ENDIAN, >> + .impl = { >> + .min_access_size = 1, >> + .max_access_size = 1, >> + }, >> +}; >> + >> static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, >> - MemoryRegion *system_memory) >> + MemoryRegion *system_memory, >> + MemoryRegion *system_io) >> { >> int i; >> static int inited; >> @@ -2816,19 +2830,10 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, >> s->bustype = CIRRUS_BUSTYPE_ISA; >> } >> >> - register_ioport_write(0x3c0, 16, 1, cirrus_vga_ioport_write, s); >> - >> - register_ioport_write(0x3b4, 2, 1, cirrus_vga_ioport_write, s); >> - register_ioport_write(0x3d4, 2, 1, cirrus_vga_ioport_write, s); >> - register_ioport_write(0x3ba, 1, 1, cirrus_vga_ioport_write, s); >> - register_ioport_write(0x3da, 1, 1, cirrus_vga_ioport_write, s); >> - >> - register_ioport_read(0x3c0, 16, 1, cirrus_vga_ioport_read, s); >> - >> - register_ioport_read(0x3b4, 2, 1, cirrus_vga_ioport_read, s); >> - register_ioport_read(0x3d4, 2, 1, cirrus_vga_ioport_read, s); >> - register_ioport_read(0x3ba, 1, 1, cirrus_vga_ioport_read, s); >> - register_ioport_read(0x3da, 1, 1, cirrus_vga_ioport_read, s); >> + /* Register ioport 0x3b0 - 0x3df */ >> + memory_region_init_io(&s->cirrus_vga_io,&cirrus_vga_io_ops, s, >> + "cirrus-io", 0x30); >> + memory_region_add_subregion(system_io, 0x3b0,&s->cirrus_vga_io); >> > Can't imagine that this reflects the original ranges and constraints > correctly. Or were they all wrong? > > I made a version (V4) with the same mapping, but Anthony has proposed to register 0x3b0 - 0x3df (https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03329.html) I don't see a problem, and it works on my computer. By the way, I will resend this patch because I forget read access in MemoryRegionOps. Sorry. > Jan > >
On 2012-08-24 16:49, Julien Grall wrote: > On 08/24/2012 02:44 PM, Jan Kiszka wrote: >> On 2012-08-22 14:27, Julien Grall wrote: >> >>> This patch replaces all register_ioport* with portio_*. It permits to >>> use the new Memory stuff like listener. >>> >>> Signed-off-by: Julien Grall<julien.grall@citrix.com> >>> --- >>> hw/cirrus_vga.c | 42 ++++++++++++++++++++++++------------------ >>> 1 files changed, 24 insertions(+), 18 deletions(-) >>> >>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c >>> index e8dcc6b..adfc855 100644 >>> --- a/hw/cirrus_vga.c >>> +++ b/hw/cirrus_vga.c >>> @@ -200,6 +200,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s, >>> typedef struct CirrusVGAState { >>> VGACommonState vga; >>> >>> + MemoryRegion cirrus_vga_io; >>> MemoryRegion cirrus_linear_io; >>> MemoryRegion cirrus_linear_bitblt_io; >>> MemoryRegion cirrus_mmio_io; >>> @@ -2528,12 +2529,15 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr) >>> return val; >>> } >>> >>> -static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) >>> +static void cirrus_vga_ioport_write(void *opaque, target_phys_addr_t addr, >>> + uint64_t val, unsigned size) >>> { >>> CirrusVGAState *c = opaque; >>> VGACommonState *s =&c->vga; >>> int index; >>> >>> + addr += 0x3b0; >>> + >>> /* check port range access depending on color/monochrome mode */ >>> if (vga_ioport_invalid(s, addr)) { >>> return; >>> @@ -2657,7 +2661,7 @@ static void cirrus_mmio_write(void *opaque, target_phys_addr_t addr, >>> if (addr>= 0x100) { >>> cirrus_mmio_blt_write(s, addr - 0x100, val); >>> } else { >>> - cirrus_vga_ioport_write(s, addr + 0x3c0, val); >>> + cirrus_vga_ioport_write(s, addr + 0x10, val, size); >>> } >>> } >>> >>> @@ -2783,8 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = { >>> }, >>> }; >>> >>> +static const MemoryRegionOps cirrus_vga_io_ops = { >>> + .write = cirrus_vga_ioport_write, >>> + .endianness = DEVICE_LITTLE_ENDIAN, >>> + .impl = { >>> + .min_access_size = 1, >>> + .max_access_size = 1, >>> + }, >>> +}; >>> + >>> static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, >>> - MemoryRegion *system_memory) >>> + MemoryRegion *system_memory, >>> + MemoryRegion *system_io) >>> { >>> int i; >>> static int inited; >>> @@ -2816,19 +2830,10 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, >>> s->bustype = CIRRUS_BUSTYPE_ISA; >>> } >>> >>> - register_ioport_write(0x3c0, 16, 1, cirrus_vga_ioport_write, s); >>> - >>> - register_ioport_write(0x3b4, 2, 1, cirrus_vga_ioport_write, s); >>> - register_ioport_write(0x3d4, 2, 1, cirrus_vga_ioport_write, s); >>> - register_ioport_write(0x3ba, 1, 1, cirrus_vga_ioport_write, s); >>> - register_ioport_write(0x3da, 1, 1, cirrus_vga_ioport_write, s); >>> - >>> - register_ioport_read(0x3c0, 16, 1, cirrus_vga_ioport_read, s); >>> - >>> - register_ioport_read(0x3b4, 2, 1, cirrus_vga_ioport_read, s); >>> - register_ioport_read(0x3d4, 2, 1, cirrus_vga_ioport_read, s); >>> - register_ioport_read(0x3ba, 1, 1, cirrus_vga_ioport_read, s); >>> - register_ioport_read(0x3da, 1, 1, cirrus_vga_ioport_read, s); >>> + /* Register ioport 0x3b0 - 0x3df */ >>> + memory_region_init_io(&s->cirrus_vga_io,&cirrus_vga_io_ops, s, >>> + "cirrus-io", 0x30); >>> + memory_region_add_subregion(system_io, 0x3b0,&s->cirrus_vga_io); >>> >> Can't imagine that this reflects the original ranges and constraints >> correctly. Or were they all wrong? >> >> > > I made a version (V4) with the same mapping, but Anthony has > proposed to register 0x3b0 - 0x3df > (https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg03329.html) Yes, likely no problem, the handlers seem to catch invalid accesses. > > I don't see a problem, and it works on my computer. > > By the way, I will resend this patch because I forget read access in > MemoryRegionOps. Sorry. Well, the fix for patch 2 is also still required. ;) I'm currently working on removing the remaining register_ioport users as I'd like to tweak the portio interface. I will follow up on your series. Jan
On 2012-08-22 14:27, Julien Grall wrote: > This patch replaces all register_ioport* with portio_*. It permits to > use the new Memory stuff like listener. > > Signed-off-by: Julien Grall <julien.grall@citrix.com> > --- > hw/cirrus_vga.c | 42 ++++++++++++++++++++++++------------------ > 1 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c > index e8dcc6b..adfc855 100644 > --- a/hw/cirrus_vga.c > +++ b/hw/cirrus_vga.c > @@ -200,6 +200,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s, > typedef struct CirrusVGAState { > VGACommonState vga; > > + MemoryRegion cirrus_vga_io; > MemoryRegion cirrus_linear_io; > MemoryRegion cirrus_linear_bitblt_io; > MemoryRegion cirrus_mmio_io; > @@ -2528,12 +2529,15 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr) > return val; > } > > -static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) > +static void cirrus_vga_ioport_write(void *opaque, target_phys_addr_t addr, > + uint64_t val, unsigned size) > { > CirrusVGAState *c = opaque; > VGACommonState *s = &c->vga; > int index; > > + addr += 0x3b0; > + > /* check port range access depending on color/monochrome mode */ > if (vga_ioport_invalid(s, addr)) { > return; > @@ -2657,7 +2661,7 @@ static void cirrus_mmio_write(void *opaque, target_phys_addr_t addr, > if (addr >= 0x100) { > cirrus_mmio_blt_write(s, addr - 0x100, val); > } else { > - cirrus_vga_ioport_write(s, addr + 0x3c0, val); > + cirrus_vga_ioport_write(s, addr + 0x10, val, size); > } > } > > @@ -2783,8 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = { > }, > }; > > +static const MemoryRegionOps cirrus_vga_io_ops = { > + .write = cirrus_vga_ioport_write, Missing .read. Crashes immediately when you test it. Jan
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c index e8dcc6b..adfc855 100644 --- a/hw/cirrus_vga.c +++ b/hw/cirrus_vga.c @@ -200,6 +200,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s, typedef struct CirrusVGAState { VGACommonState vga; + MemoryRegion cirrus_vga_io; MemoryRegion cirrus_linear_io; MemoryRegion cirrus_linear_bitblt_io; MemoryRegion cirrus_mmio_io; @@ -2528,12 +2529,15 @@ static uint32_t cirrus_vga_ioport_read(void *opaque, uint32_t addr) return val; } -static void cirrus_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) +static void cirrus_vga_ioport_write(void *opaque, target_phys_addr_t addr, + uint64_t val, unsigned size) { CirrusVGAState *c = opaque; VGACommonState *s = &c->vga; int index; + addr += 0x3b0; + /* check port range access depending on color/monochrome mode */ if (vga_ioport_invalid(s, addr)) { return; @@ -2657,7 +2661,7 @@ static void cirrus_mmio_write(void *opaque, target_phys_addr_t addr, if (addr >= 0x100) { cirrus_mmio_blt_write(s, addr - 0x100, val); } else { - cirrus_vga_ioport_write(s, addr + 0x3c0, val); + cirrus_vga_ioport_write(s, addr + 0x10, val, size); } } @@ -2783,8 +2787,18 @@ static const MemoryRegionOps cirrus_linear_io_ops = { }, }; +static const MemoryRegionOps cirrus_vga_io_ops = { + .write = cirrus_vga_ioport_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .impl = { + .min_access_size = 1, + .max_access_size = 1, + }, +}; + static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, - MemoryRegion *system_memory) + MemoryRegion *system_memory, + MemoryRegion *system_io) { int i; static int inited; @@ -2816,19 +2830,10 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, s->bustype = CIRRUS_BUSTYPE_ISA; } - register_ioport_write(0x3c0, 16, 1, cirrus_vga_ioport_write, s); - - register_ioport_write(0x3b4, 2, 1, cirrus_vga_ioport_write, s); - register_ioport_write(0x3d4, 2, 1, cirrus_vga_ioport_write, s); - register_ioport_write(0x3ba, 1, 1, cirrus_vga_ioport_write, s); - register_ioport_write(0x3da, 1, 1, cirrus_vga_ioport_write, s); - - register_ioport_read(0x3c0, 16, 1, cirrus_vga_ioport_read, s); - - register_ioport_read(0x3b4, 2, 1, cirrus_vga_ioport_read, s); - register_ioport_read(0x3d4, 2, 1, cirrus_vga_ioport_read, s); - register_ioport_read(0x3ba, 1, 1, cirrus_vga_ioport_read, s); - register_ioport_read(0x3da, 1, 1, cirrus_vga_ioport_read, s); + /* Register ioport 0x3b0 - 0x3df */ + memory_region_init_io(&s->cirrus_vga_io, &cirrus_vga_io_ops, s, + "cirrus-io", 0x30); + memory_region_add_subregion(system_io, 0x3b0, &s->cirrus_vga_io); memory_region_init(&s->low_mem_container, "cirrus-lowmem-container", @@ -2896,7 +2901,7 @@ static int vga_initfn(ISADevice *dev) s->vram_size_mb = VGA_RAM_SIZE >> 20; vga_common_init(s); cirrus_init_common(&d->cirrus_vga, CIRRUS_ID_CLGD5430, 0, - isa_address_space(dev)); + isa_address_space(dev), isa_address_space_io(dev)); s->ds = graphic_console_init(s->update, s->invalidate, s->screen_dump, s->text_update, s); @@ -2938,7 +2943,8 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev) /* setup VGA */ s->vga.vram_size_mb = VGA_RAM_SIZE >> 20; vga_common_init(&s->vga); - cirrus_init_common(s, device_id, 1, pci_address_space(dev)); + cirrus_init_common(s, device_id, 1, pci_address_space(dev), + pci_address_space_io(dev)); s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate, s->vga.screen_dump, s->vga.text_update, &s->vga);
This patch replaces all register_ioport* with portio_*. It permits to use the new Memory stuff like listener. Signed-off-by: Julien Grall <julien.grall@citrix.com> --- hw/cirrus_vga.c | 42 ++++++++++++++++++++++++------------------ 1 files changed, 24 insertions(+), 18 deletions(-)