Am 06.09.2013 um 07:04 schrieb Alexey Kardashevskiy <a...@ozlabs.ru>:
> 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 :( Yeah :). With this change we might need to do a cpu_register_sync on every RTAS call however which might incur bad performance penalties. Unless we manually define msr.dr=0. But I'd certainly prefer to reuse the existing real mode special casing code. Also, keep in mind that we might need something to handle this in the in-kernel rtas handlers too. Alex > > > -- > Alexey