| Message ID | 1313513145-5348-3-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
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);
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?
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~
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.
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. > > >
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?
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
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.
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);
Signed-off-by: Richard Henderson <rth@twiddle.net> --- hw/isa-bus.c | 39 +++++++++++++++++++++++++++++++++++++++ hw/isa.h | 33 ++++++++++++++++++++++++++++++++- 2 files changed, 71 insertions(+), 1 deletions(-)