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

[Blue Swirl]

On Wed, Aug 17, 2011 at 1:45 PM, Avi Kivity <address@hidden> wrote:
> 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

Please avoid g_new/g_malloc until they are taught to use qemu_malloc
or was it the other way around.

>> +        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.
>
>
>