Patchwork [02/14] isa: Add isa_register_old_portio_list().

login
register
mail settings
Submitter Richard Henderson
Date Aug. 16, 2011, 4:45 p.m.
Message ID <1313513145-5348-3-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/110194/
State New
Headers show

Comments

Richard Henderson - Aug. 16, 2011, 4:45 p.m.
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/isa-bus.c |   39 +++++++++++++++++++++++++++++++++++++++
 hw/isa.h     |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletions(-)
Avi Kivity - Aug. 17, 2011, 1:45 p.m.
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);
Avi Kivity - Aug. 17, 2011, 1:50 p.m.
On 08/17/2011 06:45 AM, Avi Kivity wrote:
>
>> +{
>> +    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.

Oh, after seeing a user I see how it works now.  But can't we derive 
this information instead of forcing the user to specify it?
Richard Henderson - Aug. 17, 2011, 2:06 p.m.
On 08/17/2011 06:50 AM, Avi Kivity wrote:
> Oh, after seeing a user I see how it works now. But can't we derive
> this information instead of forcing the user to specify it?

I thought about that, but then when I went to implement I found it
just as easy to have the user specify it.  With the later I get to
re-use the pieces of the read-only memory.


r~
Avi Kivity - Aug. 17, 2011, 2:09 p.m.
On 08/17/2011 07:06 AM, Richard Henderson wrote:
> On 08/17/2011 06:50 AM, Avi Kivity wrote:
> >  Oh, after seeing a user I see how it works now. But can't we derive
> >  this information instead of forcing the user to specify it?
>
> I thought about that, but then when I went to implement I found it
> just as easy to have the user specify it.  With the later I get to
> re-use the pieces of the read-only memory.
>

Yes, question is, will the user get the rules where you need to stick an 
P_E_O_L() (between any contiguous block, yes?).

I guess we can get away with just documenting it.  The list of users is 
unlikely to grow.
Blue Swirl - Aug. 17, 2011, 5:23 p.m.
On Wed, Aug 17, 2011 at 1:45 PM, Avi Kivity <avi@redhat.com> 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.
>
>
>
Avi Kivity - Aug. 17, 2011, 7:07 p.m.
On 08/17/2011 10:23 AM, Blue Swirl wrote:
> >
> >>  +        }
> >>  +
> >>  +        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.
>

Why?
Blue Swirl - Aug. 17, 2011, 7:32 p.m.
On Wed, Aug 17, 2011 at 7:07 PM, Avi Kivity <avi@redhat.com> wrote:
> On 08/17/2011 10:23 AM, Blue Swirl wrote:
>>
>> >
>> >>  +        }
>> >>  +
>> >>  +        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.
>>
>
> Why?

http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg02620.html
Avi Kivity - Aug. 17, 2011, 7:41 p.m.
On 08/17/2011 12:32 PM, Blue Swirl wrote:
> >>  Please avoid g_new/g_malloc until they are taught to use qemu_malloc
> >>  or was it the other way around.
> >>
> >
> >  Why?
>
> http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg02620.html

I don't understand.

Patch

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index e9c1712..d8e1880 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -16,6 +16,7 @@ 
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+#include <glib.h>
 #include "hw.h"
 #include "monitor.h"
 #include "sysbus.h"
@@ -103,6 +104,44 @@  void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
     }
 }
 
+void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
+                                  const MemoryRegionPortio *pio_start,
+                                  void *opaque, const char *name)
+{
+    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;
+            }
+        }
+
+        ops = g_new(MemoryRegionOps, 1);
+        ops->old_portio = pio_start;
+
+        region = g_new(MemoryRegion, 1);
+        memory_region_init_io(region, ops, opaque, name, off_high - off_low);
+        memory_region_set_offset(region, start + off_low);
+        memory_region_add_subregion(io_space, start + off_low, region);
+    }
+}
+
 static int isa_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     ISADevice *dev = DO_UPCAST(ISADevice, qdev, qdev);
diff --git a/hw/isa.h b/hw/isa.h
index c5c2618..117d8b0 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -28,7 +28,6 @@  ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space_io);
 void isa_bus_irqs(qemu_irq *irqs);
 qemu_irq isa_get_irq(int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
-void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
 void isa_init_ioport(ISADevice *dev, uint16_t ioport);
 void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length);
 void isa_qdev_register(ISADeviceInfo *info);
@@ -37,6 +36,38 @@  ISADevice *isa_create(const char *name);
 ISADevice *isa_try_create(const char *name);
 ISADevice *isa_create_simple(const char *name);
 
+/**
+ * isa_register_ioport: Install an I/O port region on the ISA bus.
+ *
+ * Register an I/O port region via memory_region_add_subregion
+ * inside the ISA I/O address space.
+ *
+ * @dev: the ISADevice against which these are registered; may be NULL.
+ * @io: the #MemoryRegion being registered.
+ * @start: the base I/O port.
+ */
+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().
+ * @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);