| Message ID | 1318865312-27483-4-git-send-email-benoit.canet@gmail.com |
|---|---|
| State | New |
| Headers | show |
2011/10/17 Benoît Canet <benoit.canet@gmail.com>: > -static void icp_control_init(uint32_t base) > +static void icp_control_init(target_phys_addr_t base) > { > - int iomemtype; > + MemoryRegion *io; > > - iomemtype = cpu_register_io_memory(icp_control_readfn, > - icp_control_writefn, NULL, > - DEVICE_NATIVE_ENDIAN); > - cpu_register_physical_memory(base, 0x00800000, iomemtype); > + io = (MemoryRegion *)g_malloc0(sizeof(MemoryRegion)); > + memory_region_init_io(io, &icp_control_ops, NULL, > + "control", 0x00800000); > + memory_region_add_subregion(get_system_memory(), base, io); > /* ??? Save/restore. */ > } I didn't spot this the first time round, but this is wrong. We shouldn't be g_malloc0()ing the MemoryRegion -- it should be an element in some suitable device struct. I think the right thing to do here is probably first to do the (fairly trivial) conversion of the icp_control code to be a sysbus device, then do the memory region conversion on top of that. -- PMM
On 10/18/2011 01:44 PM, Peter Maydell wrote: > 2011/10/17 Benoît Canet <benoit.canet@gmail.com>: > > -static void icp_control_init(uint32_t base) > > +static void icp_control_init(target_phys_addr_t base) > > { > > - int iomemtype; > > + MemoryRegion *io; > > > > - iomemtype = cpu_register_io_memory(icp_control_readfn, > > - icp_control_writefn, NULL, > > - DEVICE_NATIVE_ENDIAN); > > - cpu_register_physical_memory(base, 0x00800000, iomemtype); > > + io = (MemoryRegion *)g_malloc0(sizeof(MemoryRegion)); > > + memory_region_init_io(io, &icp_control_ops, NULL, > > + "control", 0x00800000); > > + memory_region_add_subregion(get_system_memory(), base, io); > > /* ??? Save/restore. */ > > } > > I didn't spot this the first time round, but this is wrong. > We shouldn't be g_malloc0()ing the MemoryRegion -- it should > be an element in some suitable device struct. > > I think the right thing to do here is probably first to do the > (fairly trivial) conversion of the icp_control code to be a > sysbus device, then do the memory region conversion on top of that. It's not any less wrong than the original code, which also leaked memory. I'll merge it and let any further conversion take place on top. (though g_malloc0() is better replaced by g_new()).
On 10/18/2011 04:06 PM, Avi Kivity wrote: > > I didn't spot this the first time round, but this is wrong. > > We shouldn't be g_malloc0()ing the MemoryRegion -- it should > > be an element in some suitable device struct. > > > > I think the right thing to do here is probably first to do the > > (fairly trivial) conversion of the icp_control code to be a > > sysbus device, then do the memory region conversion on top of that. > > It's not any less wrong than the original code, which also leaked > memory. I'll merge it and let any further conversion take place on top. I meant, it's not any more wrong than the original code, though my original statement is also correct.
On 18 October 2011 15:32, Avi Kivity <avi@redhat.com> wrote: > On 10/18/2011 04:06 PM, Avi Kivity wrote: >> > I didn't spot this the first time round, but this is wrong. >> > We shouldn't be g_malloc0()ing the MemoryRegion -- it should >> > be an element in some suitable device struct. >> > >> > I think the right thing to do here is probably first to do the >> > (fairly trivial) conversion of the icp_control code to be a >> > sysbus device, then do the memory region conversion on top of that. >> >> It's not any less wrong than the original code, which also leaked >> memory. I'll merge it and let any further conversion take place on top. > > I meant, it's not any more wrong than the original code, though my > original statement is also correct. Either way, if you're happy to take it into your tree that's fine with me :-) -- PMM
diff --git a/hw/integratorcp.c b/hw/integratorcp.c index c7d6596..7f79560 100644 --- a/hw/integratorcp.c +++ b/hw/integratorcp.c @@ -392,7 +392,9 @@ static int icp_pic_init(SysBusDevice *dev) } /* CP control registers. */ -static uint32_t icp_control_read(void *opaque, target_phys_addr_t offset) + +static uint64_t icp_control_read(void *opaque, target_phys_addr_t offset, + unsigned size) { switch (offset >> 2) { case 0: /* CP_IDFIELD */ @@ -410,7 +412,7 @@ static uint32_t icp_control_read(void *opaque, target_phys_addr_t offset) } static void icp_control_write(void *opaque, target_phys_addr_t offset, - uint32_t value) + uint64_t value, unsigned size) { switch (offset >> 2) { case 1: /* CP_FLASHPROG */ @@ -422,26 +424,21 @@ static void icp_control_write(void *opaque, target_phys_addr_t offset, hw_error("icp_control_write: Bad offset %x\n", (int)offset); } } -static CPUReadMemoryFunc * const icp_control_readfn[] = { - icp_control_read, - icp_control_read, - icp_control_read -}; -static CPUWriteMemoryFunc * const icp_control_writefn[] = { - icp_control_write, - icp_control_write, - icp_control_write +static const MemoryRegionOps icp_control_ops = { + .read = icp_control_read, + .write = icp_control_write, + .endianness = DEVICE_NATIVE_ENDIAN, }; -static void icp_control_init(uint32_t base) +static void icp_control_init(target_phys_addr_t base) { - int iomemtype; + MemoryRegion *io; - iomemtype = cpu_register_io_memory(icp_control_readfn, - icp_control_writefn, NULL, - DEVICE_NATIVE_ENDIAN); - cpu_register_physical_memory(base, 0x00800000, iomemtype); + io = (MemoryRegion *)g_malloc0(sizeof(MemoryRegion)); + memory_region_init_io(io, &icp_control_ops, NULL, + "control", 0x00800000); + memory_region_add_subregion(get_system_memory(), base, io); /* ??? Save/restore. */ }
Signed-off-by: Benoit Canet <benoit.canet@gmail.com> --- hw/integratorcp.c | 31 ++++++++++++++----------------- 1 files changed, 14 insertions(+), 17 deletions(-)