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

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

[Paolo Bonzini]

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>
---
 exec.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 3457f7e..251dc79 100644
--- a/exec.c
+++ b/exec.c
@@ -353,6 +353,18 @@ address_space_translate_internal(AddressSpaceDispatch *d, hwaddr addr, hwaddr *x
     *xlat = addr + section->offset_within_region;
 
     mr = section->mr;
+
+    /* MMIO registers can be expected to perform full-width accesses based only
+     * on their address, without considering adjacent registers that could
+     * decode to completely different MemoryRegions.  When such registers
+     * exist (e.g. I/O ports 0xcf8 and 0xcf9 on most PC chipsets), MMIO
+     * regions overlap wildly.  For this reason we cannot clamp the accesses
+     * here.
+     *
+     * If the length is small (as is the case for address_space_ldl/stl),
+     * everything works fine.  If the incoming length is large, however,
+     * the caller really has to do the clamping through memory_access_size.
+     */
     if (memory_region_is_ram(mr)) {
         diff = int128_sub(section->size, int128_make64(addr));
         *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
@@ -2491,7 +2503,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
 
         if (!(memory_region_is_ram(mr) ||
               memory_region_is_romd(mr))) {
-            /* do nothing */
+            l = memory_access_size(mr, l, addr1);
         } else {
             addr1 += memory_region_get_ram_addr(mr);
             /* ROM/RAM case */
-- 
2.4.3

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

[Laurent Vivier]

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>

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

[Alexander Graf]

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


Alex

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

[Paolo Bonzini]

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