January 2013 - OpenBIOS - mail.coreboot.org

Re: [OpenBIOS] PPC: QEMU breaks static variables in OpenBIOS
by Mark Cave-Ayland Jan. 6, 2013

Jan. 6, 2013

On 05/01/13 20:21, Blue Swirl wrote: (removed Ben from the CC as this is clearly an OpenBIOS issue) >> Actually here's a revised version that moves the __next_grab_slot back into >> C which is simpler still and continues to work for me. I guess in this form >> the same change could be applied as-is to hash_page_64()? > > The address arithmetic looks very similar to va2pa() or ea_to_phys(). > Could one of those be used instead? I thought about that, but figured that because it was a hot-path I'd do it inline to save some cycles invoking another function. Since it uses the same OF_* constants as those functions then it should just work. > Also since this is pretty hot code path, could you initialize the > pointer and perform the calculations just once during init? Unfortunately not :( The whole issue is that the exception handlers are invoked in real mode with MMU disabled, and hence to locate your global/static pointer you'd still have to convert its address from virtual to real before you could do anything with it. So for me it made sense to just do the increment on the variable directly. ATB, Mark.

2 8

[commit] r1081 - trunk/openbios-devel/arch/ppc/qemu
by repository service Jan. 6, 2013

Jan. 6, 2013

Author: agraf Date: Sun Jan 6 14:29:59 2013 New Revision: 1081 URL: http://tracker.coreboot.org/trac/openbios/changeset/1081 Log: PPC: Fix next slot eviction This patch fixes next slot eviction in the MMU code when our HTAB is full. The basic problem behind this is that the next slot id is stored in .bss which is automatically resolved to an address that resides in a r/o area. This is not an issue usually, because in virtual addressing mode OpenBIOS swizzles accesses to the r/o ROM area to the r/w shadow in RAM. But since the MMU code runs in real mode, this logic doesn't apply. The solution is simple: Access the global variable using its shadowed address, not the ROM address. Based upon an original patch by Mark Cave-Ayland <mark.cave-ayland(a)ilande.co.uk> Signed-off-by: Alexander Graf <agraf(a)suse.de> Modified: trunk/openbios-devel/arch/ppc/qemu/ofmem.c Modified: trunk/openbios-devel/arch/ppc/qemu/ofmem.c ============================================================================== --- trunk/openbios-devel/arch/ppc/qemu/ofmem.c Thu Dec 13 22:30:25 2012 (r1080) +++ trunk/openbios-devel/arch/ppc/qemu/ofmem.c Sun Jan 6 14:29:59 2013 (r1081) @@ -311,10 +311,32 @@ return phys; } +/* Converts a global variable (from .data or .bss) into a pointer that + can be accessed from real mode */ +static void * +global_ptr_real(void *p) +{ + return (void*)((uintptr_t)p - OF_CODE_START + get_rom_base()); +} + +/* Return the next slot to evict, in the range of [0..7] */ +static int +next_evicted_slot(void) +{ + static int next_grab_slot; + int *next_grab_slot_va; + int r; + + next_grab_slot_va = global_ptr_real(&next_grab_slot); + r = *next_grab_slot_va; + *next_grab_slot_va = (r + 1) % 8; + + return r; +} + static void hash_page_64(unsigned long ea, phys_addr_t phys, ucell mode) { - static int next_grab_slot = 0; uint64_t vsid_mask, page_mask, pgidx, hash; uint64_t htab_mask, mask, avpn; unsigned long pgaddr; @@ -349,10 +371,8 @@ found = 1; /* out of slots, just evict one */ - if (!found) { - i = next_grab_slot + 1; - next_grab_slot = (next_grab_slot + 1) % 8; - } + if (!found) + i = next_evicted_slot() + 1; i--; { mPTE_64_t p = { @@ -381,7 +401,6 @@ hash_page_32(unsigned long ea, phys_addr_t phys, ucell mode) { #ifndef __powerpc64__ - static int next_grab_slot = 0; unsigned long *upte, cmp, hash1; int i, vsid, found; mPTE_t *pp; @@ -407,10 +426,8 @@ found = 1; /* out of slots, just evict one */ - if (!found) { - i = next_grab_slot + 1; - next_grab_slot = (next_grab_slot + 1) % 8; - } + if (!found) + i = next_evicted_slot() + 1; i--; upte[i * 2] = cmp; upte[i * 2 + 1] = (phys & ~0xfff) | mode;

1 0

Re: [OpenBIOS] PPC: QEMU breaks static variables in OpenBIOS
by Mark Cave-Ayland Jan. 5, 2013

Jan. 5, 2013

On 04/01/13 20:31, Blue Swirl wrote: >> Reading the comments for SPARC in arch/sparc32/entry.S, it looks as if the >> pages are marked writeable in the MMU and there is some kind of relocation >> into RAM occurring - Blue, can you comment on this? > > At first, I tried to use ROM for .text and .rodata, eliminate all > .data and only put .bss in RAM, but because of some problems it was > easier to copy the whole ROM to RAM. It wastes some guest RAM but IIRC > Linux wanted to patch some areas. > > I'd copy the ROM to RAM for PPC also. On Sparc, MMU is always enabled > and guests take care to preserve our mappings, but maybe this is not > the case for PPC. Then fully relocatable code would make sense there. Yes, I think this is the same approach as PPC (see my previous email). The trick being used is to ensure that the the virtual addresses containing the OpenBIOS ROM image are remapped to the RAM copy once the MMU is enabled. ATB, Mark.

1 0

PPC: QEMU breaks static variables in OpenBIOS
by Mark Cave-Ayland Jan. 4, 2013

Jan. 4, 2013

Hi all, Futher to John's report of next_grab_slot not changing value within the PPC MMU hash_page_32() function, I've done some more experimentation and determined that there is indeed a problem with all OpenBIOS static variables on PPC. The issue is that OpenBIOS uses static variables in several places to keep track of various bits of information, and at compile-time these variables are given addresses within the OpenBIOS image itself, e.g. (gdb) p &next_grab_slot $1 = (int *) 0xfffdd000 However as John points out, the MemoryRegion associated with OpenBIOS is set to read-only. Single-stepping using gdb shows that while the store to update the value is executed, it is discarded by QEMU and so therefore static variables never change from their initial value. Rather amusingly, it seems that setting the variable value via gdb bypasses the MemoryRegion read-only check which made this more interesting to debug. I've done a quick hack on QEMU which is to comment out the following line from hw/ppc_oldworld.c in order to make the region read/write again: memory_region_set_readonly(bios, true); I've confirmed in gdb that this fixes the issue with static variables being able to update their value, and now with the latest round of OS X patches applied, the Darwin kernel boots although panics fairly early on. At this point, it seems that we have a decision to make. Is the bug here that the OpenBIOS ROM is marked as read-only for PPC, or OpenBIOS's use of static variables? If it's the latter, I think there is going to have to be a fairly major patch to remove static variables completely from the target image codebase. Thoughts? Mark.

7 19

Re: [OpenBIOS] static and global variable problem
by Programmingkid Jan. 1, 2013

Jan. 1, 2013

On Dec 21, 2012, at 8:14 AM, Mark Cave-Ayland wrote: > On 21/12/12 13:09, Programmingkid wrote: > >>> No, you've got this the wrong way around. The default CFLAGS for OpenBIOS uses -Os which causes heavy optimisation and confuses gdb causing it to give incorrect output. So if you want to use gdb to debug OpenBIOS, you need to rebuild it with -O0 and use the generated .nostrip unstripped binary. >> >> I have done that and the problem is still there. > > Okay - did you make sure you did a complete rebuild so the flags got picked up by the C compiler? > > rm -rf obj-ppc > vi Makefile.target > (change -Os to -O0) > ./config/scripts/switch-arch ppc > make > > If you're sure, post a message with detailed instructions to the list so that others can try and reproduce what you are seeing. > How to reproduce the problem: 1. Start QEMU for debugging: qemu-system-ppc -m 128 -bios openbios-qemu.elf.nostrip -cdrom darwin8.iso -s -S 2. Start up gdb to read the firmware file: powerpc-linux-gdb openbios-qemu.elf.nostrip 3. Connect to QEMU using this command inside gdb: target remote localhost:1234 4. Set a breakpoint at this place: b ofmem.c:414 5. Have OpenBIOS start the boot process by entering this in QEMU: boot cd:,\\:tbxi 6. When the breakpoint in tripped, type this in gdb: print &next_grab_slot. next_grab_slot will have an address that isn't available to QEMU. The value I see is 0xfffda060. QEMU would have to have over 4 GB of ram in order to access this address.

2 4