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

[Jan Kiszka]

On 2013-05-30 23:03, Paolo Bonzini 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);
> +#endif
> +    return 0;

This changed the value read from unassigned memory from -1 to 0. Any
particular reason or an unintentional change?

Note that this also breaks unassigned portio accesses, specifically the
case Stefan reported around IPMI access of the Linux kernel.

Jan

> +}
> +
> +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);
> 
-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux