Re: [Qemu-devel] [PATCH V5 3/8] hw/cirrus_vga.c: replace register_ioport*

[Julien Grall]

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<address@hidden>
> > ---
> >   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
>
>