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

[Bob Breuer]

Artyom Tarasenko wrote:
> On Mon, Jan 10, 2011 at 10:39 PM, Blue Swirl <address@hidden> wrote:
>   
>> On Mon, Jan 10, 2011 at 3:57 AM, Bob Breuer <address@hidden> wrote:
>>     
>>> Blue Swirl wrote:
>>>       
>>>> On Mon, Nov 8, 2010 at 6:55 PM, Artyom Tarasenko <address@hidden> wrote:
>>>>
>>>>         
>>>>> On Fri, May 7, 2010 at 6:26 PM, Artyom Tarasenko
>>>>> <address@hidden> 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.
>>>       
>> I see, the bug is only visible on 32 bit hosts with guest address
>> space larger than 32 bits. Also, the effect seems to be that the
>> physical address for unassigned memory accesses is reported
>> incorrectly. This may make some difference for guest fault handlers.
>>     
>
> The machine where I observed the initial bug was x86-64. Qemu was
> compiled 64 bits too.
>   

Notice that I said there were _two_ bugs.  It also goes wrong in
phys_page_find_alloc() when index & (L2_SIZE-1) !=0 and alloc is true. 
Follow the tcx mapping of 0xe20200000 around and notice that the last
level for 0xe20000000 gets a region_offset of 0x(e)20200000 instead of
it's physical address.

>>> 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;
>>>  }
>>>       
>> This is repeated quite often:
>>    p = phys_page_find(paddr >> TARGET_PAGE_BITS);
>>    if (!p) {
>>        pd = IO_MEM_UNASSIGNED;
>>    } else {
>>        pd = p->phys_offset;
>>    }
>>
>> Then we could refactor:
>> static inline ram_addr_t phys_page_get_offset(target_phys_addr_t index)
>> {
>>    PhysPageDesc *pd = phys_page_find_alloc(index, 0);
>>
>>    if (!pd || pd->phys_offset == IO_MEM_UNASSIGNED) {
>>        return IO_MEM_UNASSIGNED;
>>    }
>>    return pd->phys_offset;
>> }
>>
>>