Re: [Qemu-devel] [PATCH 06/22] memory: dispatch unassigned accesses based on .valid.accepts

[Blue Swirl]

On Thu, May 30, 2013 at 9:03 PM, Paolo Bonzini <address@hidden> wrote:
> This provides the basics for detecting accesses to unassigned memory
> as soon as they happen, and also for a simple implementation of
> address_space_access_valid.
>
> Reviewed-by: Richard Henderson <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  exec.c   |   36 ++++++++++++------------------------
>  memory.c |   28 ++++++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 785eeeb..c5100d6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1383,32 +1383,14 @@ ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>      return ram_addr;
>  }
>
> -static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
> -                                    unsigned size)
> +static bool unassigned_mem_accepts(void *opaque, hwaddr addr,
> +                                   unsigned size, bool is_write)
>  {
> -#ifdef DEBUG_UNASSIGNED
> -    printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
> -#endif
> -#if defined(TARGET_ALPHA) || defined(TARGET_SPARC) || 
> defined(TARGET_MICROBLAZE)
> -    cpu_unassigned_access(cpu_single_env, addr, 0, 0, 0, size);
> -#endif
> -    return 0;
> -}
> -
> -static void unassigned_mem_write(void *opaque, hwaddr addr,
> -                                 uint64_t val, unsigned size)
> -{
> -#ifdef DEBUG_UNASSIGNED
> -    printf("Unassigned mem write " TARGET_FMT_plx " = 0x%"PRIx64"\n", addr, 
> val);
> -#endif
> -#if defined(TARGET_ALPHA) || defined(TARGET_SPARC) || 
> defined(TARGET_MICROBLAZE)
> -    cpu_unassigned_access(cpu_single_env, addr, 1, 0, 0, size);
> -#endif
> +    return false;
>  }
>
> -static const MemoryRegionOps unassigned_mem_ops = {
> -    .read = unassigned_mem_read,
> -    .write = unassigned_mem_write,
> +const MemoryRegionOps unassigned_mem_ops = {
> +    .valid.accepts = unassigned_mem_accepts,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> @@ -1442,9 +1424,15 @@ static void notdirty_mem_write(void *opaque, hwaddr 
> ram_addr,
>          tlb_set_dirty(cpu_single_env, cpu_single_env->mem_io_vaddr);
>  }
>
> +static bool notdirty_mem_accepts(void *opaque, hwaddr addr,
> +                                 unsigned size, bool is_write)
> +{
> +    return is_write;
> +}
> +
>  static const MemoryRegionOps notdirty_mem_ops = {
> -    .read = unassigned_mem_read,
>      .write = notdirty_mem_write,
> +    .valid.accepts = notdirty_mem_accepts,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> diff --git a/memory.c b/memory.c
> index 99f046d..15da877 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -814,6 +814,29 @@ void memory_region_init(MemoryRegion *mr,
>      mr->flush_coalesced_mmio = false;
>  }
>
> +static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
> +                                    unsigned size)
> +{
> +#ifdef DEBUG_UNASSIGNED
> +    printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
> +#endif
> +#if defined(TARGET_ALPHA) || defined(TARGET_SPARC) || 
> defined(TARGET_MICROBLAZE)
> +    cpu_unassigned_access(cpu_single_env, addr, 0, 0, 0, size);

This means that memory.c is getting knowledge about CPU types and it
becomes more specific to current target. I think memory.c should be
generic and target agnostic (maybe one day compiled just once) with
exec.c implementing the target specific functionality.

> +#endif
> +    return 0;
> +}
> +
> +static void unassigned_mem_write(void *opaque, hwaddr addr,
> +                                 uint64_t val, unsigned size)
> +{
> +#ifdef DEBUG_UNASSIGNED
> +    printf("Unassigned mem write " TARGET_FMT_plx " = 0x%"PRIx64"\n", addr, 
> val);
> +#endif
> +#if defined(TARGET_ALPHA) || defined(TARGET_SPARC) || 
> defined(TARGET_MICROBLAZE)
> +    cpu_unassigned_access(cpu_single_env, addr, 1, 0, 0, size);
> +#endif
> +}
> +
>  static bool memory_region_access_valid(MemoryRegion *mr,
>                                         hwaddr addr,
>                                         unsigned size,
> @@ -847,7 +870,7 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion 
> *mr,
>      uint64_t data = 0;
>
>      if (!memory_region_access_valid(mr, addr, size, false)) {
> -        return -1U; /* FIXME: better signalling */
> +        return unassigned_mem_read(mr, addr, size);
>      }
>
>      if (!mr->ops->read) {
> @@ -898,7 +921,8 @@ static void memory_region_dispatch_write(MemoryRegion *mr,
>                                           unsigned size)
>  {
>      if (!memory_region_access_valid(mr, addr, size, true)) {
> -        return; /* FIXME: better signalling */
> +        unassigned_mem_write(mr, addr, data, size);
> +        return;
>      }
>
>      adjust_endianness(mr, &data, size);
> --
> 1.7.4.1
>
>
>