Artyom Tarasenko wrote: > On Mon, Jan 10, 2011 at 10:39 PM, Blue Swirl <blauwir...@gmail.com> wrote: > >> On Mon, Jan 10, 2011 at 3:57 AM, Bob Breuer <breu...@mc.net> wrote: >> >>> Blue Swirl wrote: >>> >>>> On Mon, Nov 8, 2010 at 6:55 PM, Artyom Tarasenko <atar4q...@gmail.com> >>>> wrote: >>>> >>>> >>>>> On Fri, May 7, 2010 at 6:26 PM, Artyom Tarasenko >>>>> <atar4q...@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; >> } >> >>