Re: [Qemu-devel] Re: phys_page_find bug?

[Bob Breuer <breuerr@mc.net>]

Blue Swirl wrote:
> On Mon, Nov 8, 2010 at 6:55 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
>   
>> On Fri, May 7, 2010 at 6:26 PM, Artyom Tarasenko
>> <atar4qemu@googlemail.com> wrote:
>>     
>>> phys_page_find (exec.c) returns sometimes a page for addresses where
>>> nothing is connected.
>>>
>>> One example, done with qemu-system-sparc -M SS-20
>>>
>>> ok f13ffff0 2f spacec@ .
>>>
>>> // The address translates correctly, in cpu_physical_memory_rw
>>> // addr== 0xff13ffff0 (where nothing is connected)
>>> // but then phys_page_find returns a nonzero and produces
>>>
>>> Unassigned mem read access of 1 byte to 0000000ff15ffff0 from xxxxx
>>>
>>> (note the "5" in the line above where "3" is expected)
>>>
>>> I wonder if this is only true for non-wired addresses, or whether
>>> phys_page_find can also
>>> find wrong pages for the addresses where something is connected?
>>>
>>> Or is my assumption is wrong and phys_page_find can return a page for
>>> not-connected
>>> addresses and the bug is actually in cpu_physical_memory_rw ?
>>>
>>> Is the qemu algorithm of working with the physical address space
>>> described somewhere?
>>>       
>> I tried to switch devices off and found that the bug is triggered by
>> registering escc.
>> It's harder to debug without escc, so I can't tell whether something
>> else is causing
>> the problem too.
>>
>> Is escc addressing somehow special?
>>     
>
> I don't think so, except that it lies close to the top of the physical
> address space.
>
>   
>>> Is the qemu algorithm of working with the physical address space described somewhere?
>>>       
>> I guess no one knows it anymore, since no-one cared to answer within a
>> half year :-/.
>>     
>
> There's of course good old exec.c, plenty of code and even some comments. ;-)
>   

You can also see this in SS-20 when OBP probes all the sbus slots.  Slot
2 with the tcx graphics shows an unexpected address:
Unassigned mem read access of 1 byte to 0000000e00000000 from ffd3f5e4
Unassigned mem read access of 1 byte to 0000000e10000000 from ffd3f5e4
Unassigned mem read access of 1 byte to 0000000020200000 from ffd3f5e4
Unassigned mem read access of 1 byte to 0000000e30000000 from ffd3f5e4

The 0202 should be e200 instead.

There's two bugs in phys_page_find_alloc().  When the bottom level L2
table is populated with IO_MEM_UNASSIGNED, region_offset is then used
for reporting the physical address.  First, region_offset may not be
aligned to the base address of the L2 region.  And second, region_offset
won't hold the full 36-bit address on a 32-bit host.

It seems that both can be fixed by returning NULL for unassigned
addresses from phys_page_find().  All callers already handle a NULL
return value.  Would this allow any further optimizations to be made?

Here's a patch to try:

diff --git a/exec.c b/exec.c
index 49c28b1..77b49c8 100644
--- a/exec.c
+++ b/exec.c
@@ -434,7 +434,11 @@ static PhysPageDesc
*phys_page_find_alloc(target_phys_addr_t index, int alloc)
 
 static inline PhysPageDesc *phys_page_find(target_phys_addr_t index)
 {
-    return phys_page_find_alloc(index, 0);
+    PhysPageDesc *pd = phys_page_find_alloc(index, 0);
+    if (pd && pd->phys_offset == IO_MEM_UNASSIGNED) {
+        return NULL;
+    }
+    return pd;
 }
 
 static void tlb_protect_code(ram_addr_t ram_addr);