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

[Blue Swirl]

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.

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