Re: [Qemu-devel] [PATCH] exec: skip MMIO regions correctly in cpu_physical_memory_write_rom_internal

[Paolo Bonzini <pbonzini@redhat.com>]

On 06/07/2015 10:51, Alexander Graf wrote:
> On 07/04/15 01:00, Laurent Vivier wrote:
>>
>> On 04/07/2015 00:42, Paolo Bonzini wrote:
>>> Loading the BIOS in the mac99 machine is interesting, because there is a
>>> PROM in the middle of the BIOS region (from 16K to 32K).  Before memory
>>> region accesses were clamped, when QEMU was asked to load a BIOS from
>>> 0xfff00000 to 0xffffffff it would put even those 16K from the BIOS file
>>> into the region.  This is weird because those 16K were not actually
>>> visible between 0xfff04000 and 0xfff07fff.  However, it worked.
>>>
>>> After clamping was added, this also worked.  In this case, the
>>> cpu_physical_memory_write_rom_internal function split the write in
>>> three parts: the first 16K were copied, the PROM area (second 16K) were
>>> ignored, then the rest was copied.
>>>
>>> Problems then started with commit 965eb2f (exec: do not clamp accesses
>>> to MMIO regions, 2015-06-17).  Clamping accesses is not done for MMIO
>>> regions because they can overlap wildly, and MMIO registers can be
>>> expected to perform full-width accesses based only on their address
>>> (with no respect for adjacent registers that could decode to completely
>>> different MemoryRegions).  However, this lack of clamping also applied
>>> to the PROM area!  cpu_physical_memory_write_rom_internal thus failed
>>> to copy the third range above, i.e. only copied the first 16K of the
>>> BIOS.
>>>
>>> In effect, address_space_translate is expecting _something else_ to do
>>> the clamping for MMIO regions if the incoming length is large.  This
>>> "something else" is memory_access_size in the case of address_space_rw,
>>> so use the same logic in cpu_physical_memory_write_rom_internal.
>>>
>>> The fix is just one line, but also add a comment explaining why there
>>> is no clamping for MMIO regions, and what it means for the callers.
>>>
>>> Reported-by: Alexander Graf <agraf@redhat.com>
>>> Fixes: 965eb2f
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>> Tested-by: Laurent Vivier <lvivier@redhat.com>
>>
> 
> Thanks, I've applied this to ppc-next to fix up the mac99 target. But
> I'd be happy to see it in the tree before my next pull request ;)

Sending pull request later today.

Paolo