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
