On 09/06/2013 12:24 AM, Alexey Kardashevskiy wrote: > On 09/05/2013 11:08 PM, Alexander Graf wrote: >> >> On 05.09.2013, at 14:49, Alexey Kardashevskiy wrote: >> >>> On 09/05/2013 10:16 PM, Alexander Graf wrote: >>>> >>>> On 05.09.2013, at 14:04, Alexey Kardashevskiy wrote: >>>> >>>>> On 09/05/2013 08:21 PM, Alexander Graf wrote: >>>>>> >>>>>> On 05.09.2013, at 12:17, Alexey Kardashevskiy wrote: >>>>>> >>>>>>> On 09/05/2013 07:27 PM, Alexander Graf wrote: >>>>>>>> >>>>>>>> On 05.09.2013, at 09:40, Alexey Kardashevskiy wrote: >>>>>>>> >>>>>>>>> On 09/05/2013 05:08 PM, Alexander Graf wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Am 05.09.2013 um 07:58 schrieb Alexey Kardashevskiy <a...@ozlabs.ru>: >>>>>>>>>> >>>>>>>>>>> On the real hardware, RTAS is called in real mode and therefore >>>>>>>>>>> ignores top 4 bits of the address passed in the call. >>>>>>>>>> >>>>>>>>>> Shouldn't we ignore the upper 4 bits for every memory access in real >>>>>>>>>> mode, not just that one parameter? >>>>>>>>> >>>>>>>>> We probably should but I just do not see any easy way of doing this. >>>>>>>>> Yet >>>>>>>>> another "Ignore N bits on the top" memory region type? No idea. >>>>>>>> >>>>>>>> Well, it already works for code that runs inside of guest context, >>>>>>>> because there the softmmu code for real mode strips the upper 4 bits. >>>>>>>> >>>>>>>> I basically see 2 ways of fixing this "correctly": >>>>>>>> >>>>>>> >>>>>>>> 1) Don't access memory through cpu_physical_memory_rw or ldx_phys but >>>>>>>> instead through real mode wrappers that strip the upper 4 bits, similar >>>>>>>> to how we handle virtual memory differently from physical memory >>>>>>> >>>>>>> But there is no a ready wrapper for this, correct? I could not find >>>>>>> any. I >>>>>>> would rather do this, looks nicer than 2). >>>>>>> >>>>>>> >>>>>>>> 2) Create 15 aliases to system_memory at the upper 4 bits of address >>>>>>>> space. That should at the end of the day give you the same effect >>>>>>> >>>>>>> Wow. Is not that too much? >>>>>>> Ooor since I am normally making bad decisions, I should do this :) >>>>>>> >>>>>>> >>>>>>>> The fix as you're proposing it wouldn't work for indirect memory >>>>>>>> descriptors. Imagine you have an "address" parameter that gives you a >>>>>>>> pointer to a struct in memory that again contains a pointer. You still >>>>>>>> want that pointer be interpreted correctly, no? >>>>>>> >>>>>>> Yes I do. I just think that having non zero bits at the top is a bug >>>>>>> and I >>>>>>> would not want the guest to continue sending bad addresses to the host. >>>>>>> Or >>>>>>> at least I want to know if it still happening. >>>>>>> >>>>>>> Now we know that the only occasion of this misbehaviour is the >>>>>>> "stop-self" >>>>>>> call and others works just fine. If something new comes up (what is >>>>>>> pretty >>>>>>> unlikely, otherwise we would have noticed this issue a loong time ago >>>>>>> AND >>>>>>> Paul already made&posted a patch for the host to fix __pa() so it is not >>>>>>> going to happen on new kernels either), ok, we will think of fixing >>>>>>> this. >>>>>>> >>>>>>> Doing in QEMU what the hardware does is a good thing but here I would >>>>>>> think >>>>>>> twice. >>>>>> >>>>>> Well, the idea behind RTAS is that everything RTAS does is usually run >>>>>> in IR=0 DR=0 inside of guest context, so that's the view of the world we >>>>>> should expose. >>>>>> >>>>>> Which makes me think. >>>>>> >>>>> >>>>>> Couldn't we just set IR=0 DR=0 when getting an RTAS call and use the >>>>>> virtual memory access functions? Those will already strip the upper 4 >>>>>> bits. >>>>> >>>>> Ok. We reached the border where my ignorance starts :) Never could >>>>> understand the concept of the guest virtual memory in QEMU. >>>>> >>>>> So we clear IR/DR and call what API? This is not address_space_rw() and >>>>> company, right? >>>> >>>> Nono, we basically route things through the same accesses that >>>> instructions inside of guest context would call. Something like >>>> >>>> cpu_ldl_data() >>>> >>>> for example. IIRC there is also an #ifdef that allows you to just run >>>> ldl(). >>> >>> cpu_ldl_data() is defined for CONFIG_USER_ONLY. But ok, it is defined >>> simply as ldl_p(): >>> >>> #define cpu_ldl_data(env, addr) ldl_raw(addr) >>> #define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + GUEST_BASE)) >>> #define laddr(x) g2h(x) >>> #define ldl_raw(p) ldl_p(laddr((p))) >>> >>> static inline int ldl_p(const void *ptr) >>> { >>> int32_t r; >>> memcpy(&r, ptr, sizeof(r)); >>> return r; >>> } >>> >>> So it tries accessing memory @ptr (which is the guest physical) and - >>> crashes :) So I need an address converter which is not there. >>> >>> What do I miss? Thanks. >> >> It should be defined through a bunch of macros and incomprehensible >> #include's and glue()'s for softmmu too. Just try and see if it works for >> you. > > > Hm. I was not clear. I tried. It crashed in ldl_p() and I explained why > exactly. I understand what you expected but it should be different set of > macros than the one you proposed.
Oh. Figured it out, that actually works. I just looked at wrong definition (which does not use CPU state) of cpu_ldl_data() because cscope and grep just could not the correct one. I had to put a breakpoint in ppc_hash64_handle_mmu_fault() to find a cpu_ldl_code, then I tried to define the _data versions of cpu_lXX_code via exec/exec-all.h (this is where the _code versions are defined) but it turned out that they are already defined in "exec/softmmu_exec.h" :-/ The glue() macro is a pure, refined evil, there should be at least a comment saying what those wonderful macros define :( -- Alexey