* [Qemu-devel] [PATCH v4 0/2] integratorcp: fix and simplify flash remap code @ 2012-01-06 18:58 Peter Maydell 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit Peter Maydell 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code Peter Maydell 0 siblings, 2 replies; 9+ messages in thread From: Peter Maydell @ 2012-01-06 18:58 UTC (permalink / raw) To: qemu-devel; +Cc: Benoît Canet, Avi Kivity, patches Simplify the flash remap code now we have an API for simple enable/disable of memory regions. Changes: v1->v2: split 'fix sense of REMAP bit' into its own patch v2->v3: minor change suggested by Avi in patch 2 (pass !(sm->cm_ctrl & 4) to set_enabled() rather than using if) v3->v4: rebase, no functional change Peter Maydell (2): hw/integratorcp: Fix sense of REMAP bit hw/integratorcp: Simplify flash remap code hw/integratorcp.c | 26 +++++++------------------- 1 files changed, 7 insertions(+), 19 deletions(-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit 2012-01-06 18:58 [Qemu-devel] [PATCH v4 0/2] integratorcp: fix and simplify flash remap code Peter Maydell @ 2012-01-06 18:58 ` Peter Maydell 2012-01-10 17:35 ` andrzej zaborowski 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code Peter Maydell 1 sibling, 1 reply; 9+ messages in thread From: Peter Maydell @ 2012-01-06 18:58 UTC (permalink / raw) To: qemu-devel; +Cc: Benoît Canet, Avi Kivity, patches Fix the sense of the REMAP bit: 0 should mean "map flash", 1 should mean "map RAM". Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/integratorcp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/hw/integratorcp.c b/hw/integratorcp.c index c8f3955..d9d8da3 100644 --- a/hw/integratorcp.c +++ b/hw/integratorcp.c @@ -112,7 +112,7 @@ static uint64_t integratorcm_read(void *opaque, target_phys_addr_t offset, static void integratorcm_do_remap(integratorcm_state *s, int flash) { - if (flash) { + if (!flash) { if (s->flash_mapped) { sysbus_del_memory(&s->busdev, &s->flash); s->flash_mapped = false; -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit Peter Maydell @ 2012-01-10 17:35 ` andrzej zaborowski 0 siblings, 0 replies; 9+ messages in thread From: andrzej zaborowski @ 2012-01-10 17:35 UTC (permalink / raw) To: Peter Maydell; +Cc: Benoît Canet, qemu-devel, patches, Avi Kivity On 6 January 2012 19:58, Peter Maydell <peter.maydell@linaro.org> wrote: > Fix the sense of the REMAP bit: 0 should mean "map flash", > 1 should mean "map RAM". Thanks, applied both patches. Cheers ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code 2012-01-06 18:58 [Qemu-devel] [PATCH v4 0/2] integratorcp: fix and simplify flash remap code Peter Maydell 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit Peter Maydell @ 2012-01-06 18:58 ` Peter Maydell 2012-01-07 4:40 ` Andreas Färber 1 sibling, 1 reply; 9+ messages in thread From: Peter Maydell @ 2012-01-06 18:58 UTC (permalink / raw) To: qemu-devel; +Cc: Benoît Canet, Avi Kivity, patches 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(-) 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; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code Peter Maydell @ 2012-01-07 4:40 ` Andreas Färber 2012-01-07 16:09 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Andreas Färber @ 2012-01-07 4:40 UTC (permalink / raw) To: Peter Maydell; +Cc: Benoît Canet, qemu-devel, patches, Avi Kivity 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; > } -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code 2012-01-07 4:40 ` Andreas Färber @ 2012-01-07 16:09 ` Peter Maydell 2012-01-08 11:16 ` Avi Kivity 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2012-01-07 16:09 UTC (permalink / raw) To: Andreas Färber; +Cc: Benoît Canet, qemu-devel, patches, Avi Kivity 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code 2012-01-07 16:09 ` Peter Maydell @ 2012-01-08 11:16 ` Avi Kivity 2012-01-08 11:19 ` Andreas Färber 0 siblings, 1 reply; 9+ messages in thread From: Avi Kivity @ 2012-01-08 11:16 UTC (permalink / raw) To: Peter Maydell; +Cc: Benoît Canet, Andreas Färber, patches, qemu-devel 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. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code 2012-01-08 11:16 ` Avi Kivity @ 2012-01-08 11:19 ` Andreas Färber 2012-01-08 11:22 ` Avi Kivity 0 siblings, 1 reply; 9+ messages in thread From: Andreas Färber @ 2012-01-08 11:19 UTC (permalink / raw) To: Avi Kivity; +Cc: Peter Maydell, Benoît Canet, qemu-devel, patches 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code 2012-01-08 11:19 ` Andreas Färber @ 2012-01-08 11:22 ` Avi Kivity 0 siblings, 0 replies; 9+ messages in thread From: Avi Kivity @ 2012-01-08 11:22 UTC (permalink / raw) To: Andreas Färber; +Cc: Peter Maydell, Benoît Canet, qemu-devel, patches 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. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-01-10 17:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-06 18:58 [Qemu-devel] [PATCH v4 0/2] integratorcp: fix and simplify flash remap code Peter Maydell 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit Peter Maydell 2012-01-10 17:35 ` andrzej zaborowski 2012-01-06 18:58 ` [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code Peter Maydell 2012-01-07 4:40 ` Andreas Färber 2012-01-07 16:09 ` Peter Maydell 2012-01-08 11:16 ` Avi Kivity 2012-01-08 11:19 ` Andreas Färber 2012-01-08 11:22 ` Avi Kivity
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.