| Message ID | 1312823229-12822-10-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 8 August 2011 18:06, Avi Kivity <avi@redhat.com> wrote: > Somewhat clumsy since it needs a variable sized region. > @@ -119,7 +120,7 @@ void omap_sdrc_reset(struct omap_sdrc_s *s); > struct omap_gpmc_s; > struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq); > void omap_gpmc_reset(struct omap_gpmc_s *s); > -void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype, > +void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem, > void (*base_upd)(void *opaque, target_phys_addr_t new), > void (*unmap)(void *opaque), void *opaque); I have a pile of omap_gpmc patches almost ready to submit which are mostly waiting on my figuring out how to get them to work with the new memory API. In particular I think this is wrong -- omap_gpmc_attach should just take a MemoryRegion*, and there should be standard APIs for "resize this memory region" and "unmap this memory region", [we have the latter but not the former currently], at which point there's no need to pass in base_upd or unmap function pointers. -- PMM
On 08/08/2011 08:43 PM, Peter Maydell wrote: > On 8 August 2011 18:06, Avi Kivity<avi@redhat.com> wrote: > > Somewhat clumsy since it needs a variable sized region. > > > @@ -119,7 +120,7 @@ void omap_sdrc_reset(struct omap_sdrc_s *s); > > struct omap_gpmc_s; > > struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq); > > void omap_gpmc_reset(struct omap_gpmc_s *s); > > -void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype, > > +void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem, > > void (*base_upd)(void *opaque, target_phys_addr_t new), > > void (*unmap)(void *opaque), void *opaque); > > I have a pile of omap_gpmc patches almost ready to submit which > are mostly waiting on my figuring out how to get them to work > with the new memory API. > > In particular I think this is wrong -- omap_gpmc_attach should > just take a MemoryRegion*, and there should be standard APIs > for "resize this memory region" and "unmap this memory region", > [we have the latter but not the former currently], at which point > there's no need to pass in base_upd or unmap function pointers. > I agree we need better support for this in the core. But I'm limiting core updates so I can continue to convert users, without which the entire effort is useless, and so I can collect additional requirements. Also, my patchset focuses on mechanical transformations. It is already risky enough in terms of regressions, I'm not going to rewrite/improve all of qemu; if you want those callbacks removed, you will have to remove them yourself.
On 9 August 2011 07:34, Avi Kivity <avi@redhat.com> wrote: > Also, my patchset focuses on mechanical transformations. It is already > risky enough in terms of regressions, I'm not going to rewrite/improve all > of qemu; if you want those callbacks removed, you will have to remove them > yourself. Sure. Can I ask you to drop this one and the onenand patch, and I'll have a go over the next week or three at incorporating a conversion into the patchset I have for those? Thanks -- PMM
On 08/09/2011 10:37 AM, Peter Maydell wrote: > On 9 August 2011 07:34, Avi Kivity<avi@redhat.com> wrote: > > Also, my patchset focuses on mechanical transformations. It is already > > risky enough in terms of regressions, I'm not going to rewrite/improve all > > of qemu; if you want those callbacks removed, you will have to remove them > > yourself. > > Sure. Can I ask you to drop this one and the onenand patch, and I'll > have a go over the next week or three at incorporating a conversion into > the patchset I have for those? > Umm... next week or three? I'd like to move fast on this. Can't you base your patches on this instead? After all, this is just a mechanical transformation, there should be no functional change.
On 9 August 2011 08:41, Avi Kivity <avi@redhat.com> wrote: > On 08/09/2011 10:37 AM, Peter Maydell wrote: >> >> On 9 August 2011 07:34, Avi Kivity<avi@redhat.com> wrote: >> > Also, my patchset focuses on mechanical transformations. It is already >> > risky enough in terms of regressions, I'm not going to rewrite/improve >> > all >> > of qemu; if you want those callbacks removed, you will have to remove >> > them >> > yourself. >> >> Sure. Can I ask you to drop this one and the onenand patch, and I'll >> have a go over the next week or three at incorporating a conversion into >> the patchset I have for those? > Umm... next week or three? I'd like to move fast on this. I'm busy at least half of this week, and next week is the KVM Forum, so I didn't want to promise anything faster. > Can't you base your patches on this instead? After all, this is just a > mechanical transformation, there should be no functional change. Well, maybe, but it seems a bit silly to commit something which is going to be half-reverted and rewritten. Also I just noticed this change: -static CPUReadMemoryFunc * const omap_gpmc_readfn[] = { - omap_badwidth_read32, /* TODO */ - omap_badwidth_read32, /* TODO */ - omap_gpmc_read, -}; - -static CPUWriteMemoryFunc * const omap_gpmc_writefn[] = { - omap_badwidth_write32, /* TODO */ - omap_badwidth_write32, /* TODO */ - omap_gpmc_write, +static const MemoryRegionOps omap_gpmc_ops = { + /* TODO: specialize <4 byte writes? */ + .read = omap_gpmc_read, + .write = omap_gpmc_write, + .endianness = DEVICE_NATIVE_ENDIAN, }; ...isn't this just throwing away the warnings on bad-width accesses? (it's not clear to me from the docs what the behaviour of the new API on bad-width accesses is going to be.) -- PMM
On 08/09/2011 11:07 AM, Peter Maydell wrote: > On 9 August 2011 08:41, Avi Kivity<avi@redhat.com> wrote: > > On 08/09/2011 10:37 AM, Peter Maydell wrote: > >> > >> On 9 August 2011 07:34, Avi Kivity<avi@redhat.com> wrote: > >> > Also, my patchset focuses on mechanical transformations. It is already > >> > risky enough in terms of regressions, I'm not going to rewrite/improve > >> > all > >> > of qemu; if you want those callbacks removed, you will have to remove > >> > them > >> > yourself. > >> > >> Sure. Can I ask you to drop this one and the onenand patch, and I'll > >> have a go over the next week or three at incorporating a conversion into > >> the patchset I have for those? > > > Umm... next week or three? I'd like to move fast on this. > > I'm busy at least half of this week, and next week is the KVM > Forum, so I didn't want to promise anything faster. > > > Can't you base your patches on this instead? After all, this is just a > > mechanical transformation, there should be no functional change. > > Well, maybe, but it seems a bit silly to commit something which is > going to be half-reverted and rewritten. It certainly won't be reverted, since the ram_addr_t based API will be removed. So it will just be modified to suit however you want it to be. If you want it otherwise, post a patch, either on top of this or instead of it, and I will incorporate it into this patchset. Again, this is not about improving individual devices (that is left for device maintainers); it's just about converting to the new API with minimal changes. > Also I just noticed this change: > > -static CPUReadMemoryFunc * const omap_gpmc_readfn[] = { > - omap_badwidth_read32, /* TODO */ > - omap_badwidth_read32, /* TODO */ > - omap_gpmc_read, > -}; > - > -static CPUWriteMemoryFunc * const omap_gpmc_writefn[] = { > - omap_badwidth_write32, /* TODO */ > - omap_badwidth_write32, /* TODO */ > - omap_gpmc_write, > +static const MemoryRegionOps omap_gpmc_ops = { > + /* TODO: specialize<4 byte writes? */ > + .read = omap_gpmc_read, > + .write = omap_gpmc_write, > + .endianness = DEVICE_NATIVE_ENDIAN, > }; > > ...isn't this just throwing away the warnings on bad-width accesses? It is; will fix. > (it's not clear to me from the docs what the behaviour of the new > API on bad-width accesses is going to be.) It's one of the things I'm collecting requirements on. So far I think we'll have an inheritable, configurable policy that allows you to ignore writes/read ones, raise a machine check exception, or warn (in developer mode only).
On 08/09/2011 11:44 AM, Avi Kivity wrote: >> ...isn't this just throwing away the warnings on bad-width accesses? > > > It is; will fix. Reading the original code, it seems broken: uint32_t omap_badwidth_read32(void *opaque, target_phys_addr_t addr) { uint32_t ret; OMAP_32B_REG(addr); cpu_physical_memory_read(addr, (void *) &ret, 4); return ret; } The code issues a read from addr, but that is not the original address used by the guest, since addresses are relative to the start of the region (in both the old and new APIs), not to physical address start. So I'll just set access size validity in the new code.
diff --git a/hw/omap.h b/hw/omap.h index a064353..c2fe54c 100644 --- a/hw/omap.h +++ b/hw/omap.h @@ -17,6 +17,7 @@ * with this program; if not, see <http://www.gnu.org/licenses/>. */ #ifndef hw_omap_h +#include "memory.h" # define hw_omap_h "omap.h" # define OMAP_EMIFS_BASE 0x00000000 @@ -119,7 +120,7 @@ void omap_sdrc_reset(struct omap_sdrc_s *s); struct omap_gpmc_s; struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq); void omap_gpmc_reset(struct omap_gpmc_s *s); -void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype, +void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem, void (*base_upd)(void *opaque, target_phys_addr_t new), void (*unmap)(void *opaque), void *opaque); diff --git a/hw/omap_gpmc.c b/hw/omap_gpmc.c index 8bf3343..4901aba 100644 --- a/hw/omap_gpmc.c +++ b/hw/omap_gpmc.c @@ -21,10 +21,13 @@ #include "hw.h" #include "flash.h" #include "omap.h" +#include "memory.h" +#include "exec-memory.h" /* General-Purpose Memory Controller */ struct omap_gpmc_s { qemu_irq irq; + MemoryRegion iomem; uint8_t sysconfig; uint16_t irqst; @@ -39,7 +42,8 @@ struct omap_gpmc_s { uint32_t config[7]; target_phys_addr_t base; size_t size; - int iomemtype; + MemoryRegion *iomem; + MemoryRegion container; void (*base_update)(void *opaque, target_phys_addr_t new); void (*unmap)(void *opaque); void *opaque; @@ -75,8 +79,12 @@ static void omap_gpmc_cs_map(struct omap_gpmc_cs_file_s *f, int base, int mask) * constant), the mask should cause wrapping of the address space, so * that the same memory becomes accessible at every <i>size</i> bytes * starting from <i>base</i>. */ - if (f->iomemtype) - cpu_register_physical_memory(f->base, f->size, f->iomemtype); + if (f->iomem) { + memory_region_init(&f->container, "omap-gpmc-file", f->size); + memory_region_add_subregion(&f->container, 0, f->iomem); + memory_region_add_subregion(get_system_memory(), f->base, + &f->container); + } if (f->base_update) f->base_update(f->opaque, f->base); @@ -87,8 +95,11 @@ static void omap_gpmc_cs_unmap(struct omap_gpmc_cs_file_s *f) if (f->size) { if (f->unmap) f->unmap(f->opaque); - if (f->iomemtype) - cpu_register_physical_memory(f->base, f->size, IO_MEM_UNASSIGNED); + if (f->iomem) { + memory_region_del_subregion(get_system_memory(), &f->container); + memory_region_del_subregion(&f->container, f->iomem); + memory_region_destroy(&f->container); + } f->base = 0; f->size = 0; } @@ -132,7 +143,8 @@ void omap_gpmc_reset(struct omap_gpmc_s *s) ecc_reset(&s->ecc[i]); } -static uint32_t omap_gpmc_read(void *opaque, target_phys_addr_t addr) +static uint64_t omap_gpmc_read(void *opaque, target_phys_addr_t addr, + unsigned size) { struct omap_gpmc_s *s = (struct omap_gpmc_s *) opaque; int cs; @@ -230,7 +242,7 @@ static uint32_t omap_gpmc_read(void *opaque, target_phys_addr_t addr) } static void omap_gpmc_write(void *opaque, target_phys_addr_t addr, - uint32_t value) + uint64_t value, unsigned size) { struct omap_gpmc_s *s = (struct omap_gpmc_s *) opaque; int cs; @@ -249,7 +261,7 @@ static void omap_gpmc_write(void *opaque, target_phys_addr_t addr, case 0x010: /* GPMC_SYSCONFIG */ if ((value >> 3) == 0x3) - fprintf(stderr, "%s: bad SDRAM idle mode %i\n", + fprintf(stderr, "%s: bad SDRAM idle mode %"PRIi64"\n", __FUNCTION__, value >> 3); if (value & 2) omap_gpmc_reset(s); @@ -369,34 +381,27 @@ static void omap_gpmc_write(void *opaque, target_phys_addr_t addr, } } -static CPUReadMemoryFunc * const omap_gpmc_readfn[] = { - omap_badwidth_read32, /* TODO */ - omap_badwidth_read32, /* TODO */ - omap_gpmc_read, -}; - -static CPUWriteMemoryFunc * const omap_gpmc_writefn[] = { - omap_badwidth_write32, /* TODO */ - omap_badwidth_write32, /* TODO */ - omap_gpmc_write, +static const MemoryRegionOps omap_gpmc_ops = { + /* TODO: specialize <4 byte writes? */ + .read = omap_gpmc_read, + .write = omap_gpmc_write, + .endianness = DEVICE_NATIVE_ENDIAN, }; struct omap_gpmc_s *omap_gpmc_init(target_phys_addr_t base, qemu_irq irq) { - int iomemtype; struct omap_gpmc_s *s = (struct omap_gpmc_s *) qemu_mallocz(sizeof(struct omap_gpmc_s)); omap_gpmc_reset(s); - iomemtype = cpu_register_io_memory(omap_gpmc_readfn, - omap_gpmc_writefn, s, DEVICE_NATIVE_ENDIAN); - cpu_register_physical_memory(base, 0x1000, iomemtype); + memory_region_init_io(&s->iomem, &omap_gpmc_ops, s, "omap-gpmc", 0x1000); + memory_region_add_subregion(get_system_memory(), base, &s->iomem); return s; } -void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype, +void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, MemoryRegion *iomem, void (*base_upd)(void *opaque, target_phys_addr_t new), void (*unmap)(void *opaque), void *opaque) { @@ -408,7 +413,7 @@ void omap_gpmc_attach(struct omap_gpmc_s *s, int cs, int iomemtype, } f = &s->cs_file[cs]; - f->iomemtype = iomemtype; + f->iomem = iomem; f->base_update = base_upd; f->unmap = unmap; f->opaque = opaque; diff --git a/hw/tusb6010.c b/hw/tusb6010.c index add748c..28ff52c 100644 --- a/hw/tusb6010.c +++ b/hw/tusb6010.c @@ -26,7 +26,7 @@ #include "tusb6010.h" struct TUSBState { - int iomemtype[2]; + MemoryRegion iomem[2]; qemu_irq irq; MUSBState *musb; QEMUTimer *otg_timer; @@ -234,14 +234,14 @@ struct TUSBState { #define TUSB_EP_CONFIG_XFR_SIZE(v) ((v) & 0x7fffffff) #define TUSB_PROD_TEST_RESET_VAL 0xa596 -int tusb6010_sync_io(TUSBState *s) +MemoryRegion *tusb6010_sync_io(TUSBState *s) { - return s->iomemtype[0]; + return &s->iomem[0]; } -int tusb6010_async_io(TUSBState *s) +MemoryRegion *tusb6010_async_io(TUSBState *s) { - return s->iomemtype[1]; + return &s->iomem[1]; } static void tusb_intr_update(TUSBState *s) @@ -647,16 +647,12 @@ static void tusb_async_writew(void *opaque, target_phys_addr_t addr, } } -static CPUReadMemoryFunc * const tusb_async_readfn[] = { - tusb_async_readb, - tusb_async_readh, - tusb_async_readw, -}; - -static CPUWriteMemoryFunc * const tusb_async_writefn[] = { - tusb_async_writeb, - tusb_async_writeh, - tusb_async_writew, +static const MemoryRegionOps tusb_async_ops = { + .old_mmio = { + .read = { tusb_async_readb, tusb_async_readh, tusb_async_readw, }, + .write = { tusb_async_writeb, tusb_async_writeh, tusb_async_writew, }, + }, + .endianness = DEVICE_NATIVE_ENDIAN, }; static void tusb_otg_tick(void *opaque) @@ -739,8 +735,8 @@ TUSBState *tusb6010_init(qemu_irq intr) s->mask = 0xffffffff; s->intr = 0x00000000; s->otg_timer_val = 0; - s->iomemtype[1] = cpu_register_io_memory(tusb_async_readfn, - tusb_async_writefn, s, DEVICE_NATIVE_ENDIAN); + memory_region_init_io(&s->iomem[1], &tusb_async_ops, s, "tusb-async", + UINT32_MAX); s->irq = intr; s->otg_timer = qemu_new_timer_ns(vm_clock, tusb_otg_tick, s); s->pwr_timer = qemu_new_timer_ns(vm_clock, tusb_power_tick, s); diff --git a/hw/tusb6010.h b/hw/tusb6010.h index 6faa94d..9d53b26 100644 --- a/hw/tusb6010.h +++ b/hw/tusb6010.h @@ -1,10 +1,13 @@ #ifndef TUSB6010_H #define TUSB6010_H +#include "targphys.h" +#include "memory.h" + typedef struct TUSBState TUSBState; TUSBState *tusb6010_init(qemu_irq intr); -int tusb6010_sync_io(TUSBState *s); -int tusb6010_async_io(TUSBState *s); +MemoryRegion *tusb6010_sync_io(TUSBState *s); +MemoryRegion *tusb6010_async_io(TUSBState *s); void tusb6010_power(TUSBState *s, int on); #endif
Somewhat clumsy since it needs a variable sized region. Signed-off-by: Avi Kivity <avi@redhat.com> --- hw/omap.h | 3 ++- hw/omap_gpmc.c | 53 +++++++++++++++++++++++++++++------------------------ hw/tusb6010.c | 30 +++++++++++++----------------- hw/tusb6010.h | 7 +++++-- 4 files changed, 49 insertions(+), 44 deletions(-)