| Message ID | 1325876308-28097-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 563c2bf35cf4a20b504dd371c395d987f67e16d4 |
| Headers | show |
Am 06.01.2012 19:58, schrieb Peter Maydell: > Use the new memory mutator API to simplify the flash remap code; > this allows us to drop the flash_mapped flag. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Andreas Färber <afaerber@suse.de> I noticed you're dropping a comment about tlb_flush(). Is that because it was dead code, or does the Memory API take care of it for us? (I assume the former because memory_region_set_enabled() does not take a CPUState* to operate on.) Andreas > --- > hw/integratorcp.c | 26 +++++++------------------- > 1 files changed, 7 insertions(+), 19 deletions(-) > > diff --git a/hw/integratorcp.c b/hw/integratorcp.c > index d9d8da3..e5712fc 100644 > --- a/hw/integratorcp.c > +++ b/hw/integratorcp.c > @@ -21,7 +21,6 @@ typedef struct { > MemoryRegion iomem; > uint32_t memsz; > MemoryRegion flash; > - bool flash_mapped; > uint32_t cm_osc; > uint32_t cm_ctrl; > uint32_t cm_lock; > @@ -110,20 +109,12 @@ static uint64_t integratorcm_read(void *opaque, target_phys_addr_t offset, > } > } > > -static void integratorcm_do_remap(integratorcm_state *s, int flash) > +static void integratorcm_do_remap(integratorcm_state *s) > { > - if (!flash) { > - if (s->flash_mapped) { > - sysbus_del_memory(&s->busdev, &s->flash); > - s->flash_mapped = false; > - } > - } else { > - if (!s->flash_mapped) { > - sysbus_add_memory_overlap(&s->busdev, 0, &s->flash, 1); > - s->flash_mapped = true; > - } > - } > - //??? tlb_flush (cpu_single_env, 1); > + /* Sync memory region state with CM_CTRL REMAP bit: > + * bit 0 => flash at address 0; bit 1 => RAM > + */ > + memory_region_set_enabled(&s->flash, !(s->cm_ctrl & 4)); > } > > static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value) > @@ -131,9 +122,6 @@ static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value) > if (value & 8) { > qemu_system_reset_request(); > } > - if ((s->cm_ctrl ^ value) & 4) { > - integratorcm_do_remap(s, (value & 4) == 0); > - } > if ((s->cm_ctrl ^ value) & 1) { > /* (value & 1) != 0 means the green "MISC LED" is lit. > * We don't have any nice place to display LEDs. printf is a bad > @@ -143,6 +131,7 @@ static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value) > } > /* Note that the RESET bit [3] always reads as zero */ > s->cm_ctrl = (s->cm_ctrl & ~5) | (value & 5); > + integratorcm_do_remap(s); > } > > static void integratorcm_update(integratorcm_state *s) > @@ -263,13 +252,12 @@ static int integratorcm_init(SysBusDevice *dev) > s->cm_init = 0x00000112; > memory_region_init_ram(&s->flash, "integrator.flash", 0x100000); > vmstate_register_ram_global(&s->flash); > - s->flash_mapped = false; > > memory_region_init_io(&s->iomem, &integratorcm_ops, s, > "integratorcm", 0x00800000); > sysbus_init_mmio(dev, &s->iomem); > > - integratorcm_do_remap(s, 1); > + integratorcm_do_remap(s); > /* ??? Save/restore. */ > return 0; > }
On 7 January 2012 04:40, Andreas Färber <afaerber@suse.de> wrote: > I noticed you're dropping a comment about tlb_flush(). Is that because > it was dead code, or does the Memory API take care of it for us? > (I assume the former because memory_region_set_enabled() does not take a > CPUState* to operate on.) Arranging for a tlb_flush if needed has to be the Memory API's problem, because everything that uses the memory API to rearrange the view of the address space has exactly the same requirement that anything else in QEMU has to be told it may need to redo any caching data structure it has set up. In particular, this doesn't apply just for a single CPUState* -- in a multicore system every CPU has to be made aware that the world has changed, not just the one which happened to do the store to the "remap the flash" register. Also the memory API is the only thing that knows whether the region was already enabled (ie this is a no-op), whether some other region is overlapping so in fact it makes no difference whether we enable or disable this one, etc etc). So basically I dropped that line because it was a bit of commented out code with a ??? marker appended, and the device layer is the wrong place to be trying to solve the issue it was gesturing at. -- PMM
On 01/07/2012 06:09 PM, Peter Maydell wrote: > On 7 January 2012 04:40, Andreas Färber <afaerber@suse.de> wrote: > > I noticed you're dropping a comment about tlb_flush(). Is that because > > it was dead code, or does the Memory API take care of it for us? > > (I assume the former because memory_region_set_enabled() does not take a > > CPUState* to operate on.) > > Arranging for a tlb_flush if needed has to be the Memory API's > problem, because everything that uses the memory API to rearrange > the view of the address space has exactly the same requirement that > anything else in QEMU has to be told it may need to redo any caching > data structure it has set up. In particular, this doesn't apply just > for a single CPUState* -- in a multicore system every CPU has to be > made aware that the world has changed, not just the one which > happened to do the store to the "remap the flash" register. > Also the memory API is the only thing that knows whether the region > was already enabled (ie this is a no-op), whether some other region > is overlapping so in fact it makes no difference whether we enable > or disable this one, etc etc). > > So basically I dropped that line because it was a bit of commented > out code with a ??? marker appended, and the device layer is the > wrong place to be trying to solve the issue it was gesturing at. > Correct, and the memory core will flush the tlb, so all is well.
Am 08.01.2012 12:16, schrieb Avi Kivity: > On 01/07/2012 06:09 PM, Peter Maydell wrote: >> On 7 January 2012 04:40, Andreas Färber <afaerber@suse.de> wrote: >>> I noticed you're dropping a comment about tlb_flush(). Is that because >>> it was dead code, or does the Memory API take care of it for us? >>> (I assume the former because memory_region_set_enabled() does not take a >>> CPUState* to operate on.) >> >> Arranging for a tlb_flush if needed has to be the Memory API's >> problem, because everything that uses the memory API to rearrange >> the view of the address space has exactly the same requirement that >> anything else in QEMU has to be told it may need to redo any caching >> data structure it has set up. In particular, this doesn't apply just >> for a single CPUState* -- in a multicore system every CPU has to be >> made aware that the world has changed, not just the one which >> happened to do the store to the "remap the flash" register. >> Also the memory API is the only thing that knows whether the region >> was already enabled (ie this is a no-op), whether some other region >> is overlapping so in fact it makes no difference whether we enable >> or disable this one, etc etc). >> >> So basically I dropped that line because it was a bit of commented >> out code with a ??? marker appended, and the device layer is the >> wrong place to be trying to solve the issue it was gesturing at. >> > > Correct, and the memory core will flush the tlb, so all is well. Hm, I had grep'ed for tlb_flush in memory.* without a hit... I would've assumed to find tlb_flush_all() somewhere. Andreas
On 01/08/2012 01:19 PM, Andreas Färber wrote: > > > > Correct, and the memory core will flush the tlb, so all is well. > > Hm, I had grep'ed for tlb_flush in memory.* without a hit... > I would've assumed to find tlb_flush_all() somewhere. > It's at then end of cpu_register_physical_memory_log(), which the memory core still uses as part of the implementation.
diff --git a/hw/integratorcp.c b/hw/integratorcp.c index d9d8da3..e5712fc 100644 --- a/hw/integratorcp.c +++ b/hw/integratorcp.c @@ -21,7 +21,6 @@ typedef struct { MemoryRegion iomem; uint32_t memsz; MemoryRegion flash; - bool flash_mapped; uint32_t cm_osc; uint32_t cm_ctrl; uint32_t cm_lock; @@ -110,20 +109,12 @@ static uint64_t integratorcm_read(void *opaque, target_phys_addr_t offset, } } -static void integratorcm_do_remap(integratorcm_state *s, int flash) +static void integratorcm_do_remap(integratorcm_state *s) { - if (!flash) { - if (s->flash_mapped) { - sysbus_del_memory(&s->busdev, &s->flash); - s->flash_mapped = false; - } - } else { - if (!s->flash_mapped) { - sysbus_add_memory_overlap(&s->busdev, 0, &s->flash, 1); - s->flash_mapped = true; - } - } - //??? tlb_flush (cpu_single_env, 1); + /* Sync memory region state with CM_CTRL REMAP bit: + * bit 0 => flash at address 0; bit 1 => RAM + */ + memory_region_set_enabled(&s->flash, !(s->cm_ctrl & 4)); } static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value) @@ -131,9 +122,6 @@ static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value) if (value & 8) { qemu_system_reset_request(); } - if ((s->cm_ctrl ^ value) & 4) { - integratorcm_do_remap(s, (value & 4) == 0); - } if ((s->cm_ctrl ^ value) & 1) { /* (value & 1) != 0 means the green "MISC LED" is lit. * We don't have any nice place to display LEDs. printf is a bad @@ -143,6 +131,7 @@ static void integratorcm_set_ctrl(integratorcm_state *s, uint32_t value) } /* Note that the RESET bit [3] always reads as zero */ s->cm_ctrl = (s->cm_ctrl & ~5) | (value & 5); + integratorcm_do_remap(s); } static void integratorcm_update(integratorcm_state *s) @@ -263,13 +252,12 @@ static int integratorcm_init(SysBusDevice *dev) s->cm_init = 0x00000112; memory_region_init_ram(&s->flash, "integrator.flash", 0x100000); vmstate_register_ram_global(&s->flash); - s->flash_mapped = false; memory_region_init_io(&s->iomem, &integratorcm_ops, s, "integratorcm", 0x00800000); sysbus_init_mmio(dev, &s->iomem); - integratorcm_do_remap(s, 1); + integratorcm_do_remap(s); /* ??? Save/restore. */ return 0; }
Use the new memory mutator API to simplify the flash remap code; this allows us to drop the flash_mapped flag. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/integratorcp.c | 26 +++++++------------------- 1 files changed, 7 insertions(+), 19 deletions(-)