Re: [Qemu-devel] [PATCH 03/12] s390: Add mapping helper functions.

[Alexander Graf]

On 17.01.2013, at 15:23, Cornelia Huck wrote:

> Add s390_cpu_physical_memory_{map,unmap} with special handling
> for the lowcore.
> 
> Signed-off-by: Cornelia Huck <address@hidden>
> ---
> target-s390x/cpu.h    |  4 ++++
> target-s390x/helper.c | 26 ++++++++++++++++++++++++++
> 2 files changed, 30 insertions(+)
> 
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 6700fe9..cd729d3 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -299,6 +299,10 @@ int cpu_s390x_handle_mmu_fault (CPUS390XState *env, 
> target_ulong address, int rw
> 
> 
> #ifndef CONFIG_USER_ONLY
> +void *s390_cpu_physical_memory_map(CPUS390XState *env, hwaddr addr, hwaddr 
> len,
> +                                   int is_write);
> +void s390_cpu_physical_memory_unmap(CPUS390XState *env, void *addr, hwaddr 
> len,
> +                                    int is_write);
> void s390x_tod_timer(void *opaque);
> void s390x_cpu_timer(void *opaque);
> 
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index bf2b4d3..d350f28 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -490,6 +490,32 @@ static void cpu_unmap_lowcore(LowCore *lowcore, hwaddr 
> len)
>     cpu_physical_memory_unmap(lowcore, len, 1, len);
> }
> 
> +void *s390_cpu_physical_memory_map(CPUS390XState *env, hwaddr addr, hwaddr 
> len,

Please stick to the same semantics as cpu_physical_memory_map and pass hwaddr 
*len here. Otherwise the caller can't do sanity checks.

> +                                   int is_write)
> +{
> +    hwaddr start = addr;
> +
> +    /* Mind the prefix area. */
> +    if (addr < 8192) {
> +        start += env->psa;
> +    } else if ((env->psa <= addr) && (addr < env->psa + 8192)) {
> +        start -= env->psa;
> +    }
> +
> +    if ((addr + len <= env->psa) || (addr >= env->psa + 8192)) {
> +        return cpu_physical_memory_map(start, &len, is_write);
> +    }
> +
> +    DPRINTF("mapping across lowcore boundaries not yet supported\n");

I only understand half of what this function does :). You basically want

  if (addr < 8192) {
    start += env->psa;
    len = min(8192 - addr, len);
}

We can easily support mapping of the lowcode higher up in memory, right? So we 
only have to take the case into account where [addr..addr+len] is within the 
first 8192 bytes.

And if you pass len as pointer, we can also indicate to the caller that the 
mapping isn't for all of the memory he requested, but only part of it.


Alex

> +    return NULL;
> +}
> +
> +void s390_cpu_physical_memory_unmap(CPUS390XState *env, void *addr, hwaddr 
> len,
> +                                    int is_write)
> +{
> +    cpu_physical_memory_unmap(addr, len, is_write, len);
> +}
> +
> static void do_svc_interrupt(CPUS390XState *env)
> {
>     uint64_t mask, addr;
> -- 
> 1.7.12.4
>