[Qemu-devel] [PATCH v4 0/2] integratorcp: fix and simplify flash remap code

[Qemu-devel] [PATCH v4 0/2] integratorcp: fix and simplify flash remap code

[Peter Maydell]

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(-)

[Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit

[Peter Maydell]

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

[Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code

[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>
---
 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

Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code

[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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH v4 2/2] hw/integratorcp: Simplify flash remap code

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH v4 1/2] hw/integratorcp: Fix sense of REMAP bit

[andrzej zaborowski]

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