Re: [Qemu-devel] [PATCH 12/40] memory: add address_space_translate

[Paolo Bonzini <pbonzini@redhat.com>]

Il 07/05/2013 20:08, Peter Maydell ha scritto:
>> >
>> > -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
>> > +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
>> > +                                      false);
> I find it a little confusing reusing 'addr' for the returned
> offset from address_space_translate(); maybe using a different
> variable would be clearer? (don't feel very strongly about it)

True, on the other hand this matches usage before this patch.

-        addr = memory_region_section_addr(section, addr);

>> > +    if (l < 4) {
>> > +        return -1;
>> > +    }
>> >
>> >      if (!(memory_region_is_ram(section->mr) ||
>> >            memory_region_is_romd(section->mr))) {
>> >          /* I/O case */
>> > -        addr = memory_region_section_addr(section, addr);
>> >          val = io_mem_read(section->mr, addr, 4);
>> >  #if defined(TARGET_WORDS_BIGENDIAN)
>> >          if (endian == DEVICE_LITTLE_ENDIAN) {
>> > @@ -2249,7 +2240,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>> >          /* RAM case */
>> >          ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
>> >                                  & TARGET_PAGE_MASK)
>> > -                               + memory_region_section_addr(section, addr));
>> > +                               + addr);

>>
>> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
>> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
>> +                                      false);
>> +    if (l < 2) {
>> +        return -1;
>> +    }
> 
> This kind of "let's just return -1 if the read failed" kind
> of bothers me, because "memory access aborted" is really
> completely different from "memory access succeeded and
> returned -1". But there are a lot of APIs which we'd need
> to fix to properly communicate the problem back to the
> caller, so let it slide for now. (What used to happen
> in the old code in this case?)

It would call unassigned_mem_read and return 0.  I'll change this to
call unassigned_mem_read, which also requires fixing the "double use" of
addr that you pointed out above.

Paolo