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

[Paolo Bonzini]

Il 16/07/2013 14:28, Jan Kiszka ha scritto:
> 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?

Cut-and-paste (unassigned RAM used to return 0, invalid MMIO used to
return -1, unifying the paths dropped the difference).

I guess unassigned RAM can read as -1 just fine, so we can just change
unassigned_mem_read to return -1.

Paolo

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