[Qemu-devel] phys_page_find bug?

[Qemu-devel] phys_page_find bug?

[Artyom Tarasenko]

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?

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Qemu-devel] Re: phys_page_find bug?

[Artyom Tarasenko]

2010/5/7 Artyom Tarasenko <atar4qemu@googlemail.com>:
> 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'm surprised that no one is interested in discussing this issue. It
may affect other targets too.

After some debugging I see that page 0xff15ff000 is allocated twice
when emulating SS-20. Can this be a problem?

>From the phys_page_find logic it looks like the pages are expected  to
be allocated in the natural order: the loop descends till the page
hits a search mask. sun4m_hw_init initializes devices in a more or
less random order. Can this be a problem?

Also the function cpu_register_physical_memory_offset the following comment:

...Both start_addr and region_offset are rounded down to a page boundary
   before calculating this offset.  This should not be a problem unless
   the low bits of start_addr and region_offset differ.  */

What is meant here by "low bits"? I put a check

if((region_offset & TARGET_PAGE_MASK)!=(start_addr &
TARGET_PAGE_MASK)) printf...

and it gets hit a lot within the address range 0xd00000512-ff1800000 .
Does it indicate a problem?


-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Qemu-devel] Re: phys_page_find bug?

[Artyom Tarasenko]

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?

>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 :-/.

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

[Qemu-devel] Re: phys_page_find bug?

[Blue Swirl]

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

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

[Bob Breuer]

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

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

[Blue Swirl]

On Mon, Jan 10, 2011 at 3:57 AM, Bob Breuer <breuerr@mc.net> wrote:
> 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.

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

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

[Bob Breuer]

Blue Swirl wrote:
> On Mon, Jan 10, 2011 at 3:57 AM, Bob Breuer <breuerr@mc.net> wrote:
>   
>> 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.
>>     
>
> 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;
> }
>   

Might work, but most callers also need region_offset for valid IO
devices.  Maybe try:
    pd = phys_page_get_offset(paddr >> TARGET_PAGE_BITS, &p);
in the caller so they could still get p->region_offset later.

On second thought, when gcc inlines phys_page_find(), can it optimize
away the extra check for NULL?

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

[Artyom Tarasenko]

On Mon, Jan 10, 2011 at 10:39 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Jan 10, 2011 at 3:57 AM, Bob Breuer <breuerr@mc.net> wrote:
>> 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.
>
> 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.

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



-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/

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

[Bob Breuer]

Artyom Tarasenko wrote:
> On Mon, Jan 10, 2011 at 10:39 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>   
>> On Mon, Jan 10, 2011 at 3:57 AM, Bob Breuer <breuerr@mc.net> wrote:
>>     
>>> 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.
>>>       
>> 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;
>> }
>>
>>     

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

[Artyom Tarasenko]

Tested-by: Artyom Tarasenko <atar4qemu@gmail.com>

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



-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/