Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Avi Kivity <avi@redhat.com>]

On 08/16/2011 09:45 AM, Richard Henderson wrote:
>
> +void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
> +                                  const MemoryRegionPortio *pio_start,
> +                                  void *opaque, const char *name)

_old_ implies it's deprecated, please drop.  It's only old if it's in a 
user specified MemoryRegionOps.

> +{
> +    MemoryRegion *io_space = isabus->address_space_io;
> +    const MemoryRegionPortio *pio_iter;
> +
> +    /* 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.  */
> +    if (dev) {
> +        isa_init_ioport(dev, start);
> +    }
> +
> +    for (; pio_start->size != 0; pio_start = pio_iter + 1) {
> +        unsigned int off_low = UINT_MAX, off_high = 0;
> +        MemoryRegionOps *ops;
> +        MemoryRegion *region;
> +
> +        for (pio_iter = pio_start; pio_iter->size; ++pio_iter) {
> +            if (pio_iter->offset<  off_low) {
> +                off_low = pio_iter->offset;
> +            }
> +            if (pio_iter->offset + pio_iter->len>  off_high) {
> +                off_high = pio_iter->offset + pio_iter->len;
> +            }

This is supposed to pick up MRPs that are for the same port address?  If 
so that should be in the loop termination condition.

> +        }
> +
> +        ops = g_new(MemoryRegionOps, 1);


g_new0(), we rely on initialized memory here

> +        ops->old_portio = pio_start;
> +
> +        region = g_new(MemoryRegion, 1);

(but not here)

> +        memory_region_init_io(region, ops, opaque, name, off_high - off_low);
> +        memory_region_set_offset(region, start + off_low);

I think the memory core ignores set_offset() for portio.

> +        memory_region_add_subregion(io_space, start + off_low, region);
> +    }
> +}

> +void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
> +
> +/**
> + * isa_register_old_portio_list: Initialize a set of ISA io ports
> + *
> + * Several ISA devices have many dis-joint I/O ports.  Worse, these I/O
> + * ports can be interleaved with I/O ports from other devices.  This
> + * function makes it easy to create multiple MemoryRegions for a single
> + * device and use the legacy portio routines.
> + *
> + * @dev: the ISADevice against which these are registered; may be NULL.
> + * @start: the base I/O port against which the portio->offset is applied.
> + * @old_portio: A concatenation of several #MemoryRegionOps old_portio
> + *   parameters.  The entire list should be terminated by a double
> + *   PORTIO_END_OF_LIST().

double seems harsh.

> + * @opaque: passed into the old_portio callbacks.
> + * @name: passed into memory_region_init_io.
> + */
> +void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
> +                                  const MemoryRegionPortio *old_portio,
> +                                  void *opaque, const char *name);
> +
>   extern target_phys_addr_t isa_mem_base;
>
>   void isa_mmio_setup(MemoryRegion *mr, target_phys_addr_t size);


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.