From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59778) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHplo-0007im-D3 for qemu-devel@nongnu.org; Fri, 06 Sep 2013 02:43:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VHplg-00036B-BF for qemu-devel@nongnu.org; Fri, 06 Sep 2013 02:43:52 -0400 Received: from mail-pb0-f53.google.com ([209.85.160.53]:37523) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHplg-00035v-2H for qemu-devel@nongnu.org; Fri, 06 Sep 2013 02:43:44 -0400 Received: by mail-pb0-f53.google.com with SMTP id up15so2794104pbc.40 for ; Thu, 05 Sep 2013 23:43:43 -0700 (PDT) Message-ID: <52297999.4080309@ozlabs.ru> Date: Fri, 06 Sep 2013 16:43:37 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1378360700-4300-1-git-send-email-aik@ozlabs.ru> <5228357E.7000106@ozlabs.ru> <41FD8D0F-A89D-4F2C-93D1-CE196B8B5D52@suse.de> <52285A2B.1080707@ozlabs.ru> <52287364.7050909@ozlabs.ru> <97E06F25-B308-421D-ADC8-337FDFF58BE8@suse.de> <52287DBC.2090303@ozlabs.ru> <1E8477AC-7AF1-43AE-BD1C-013E86D7A806@suse.de> <52289419.8080009@ozlabs.ru> <5229625D.9050607@ozlabs.ru> <54DD33A4-43E4-42AD-BC27-AF8F587D2294@suse.de> In-Reply-To: <54DD33A4-43E4-42AD-BC27-AF8F587D2294@suse.de> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] spapr-rtas: reset top 4 bits in parameters address List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: "qemu-ppc@nongnu.org" , Paul Mackerras , "qemu-devel@nongnu.org" On 09/06/2013 04:22 PM, Alexander Graf wrote: > > > Am 06.09.2013 um 07:04 schrieb Alexey Kardashevskiy : > >> 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 : >>>>>>>>>>>> >>>>>>>>>>>>> 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. Ok. So. I made a patch, I'll post it soon. I just though that it might make sense to fix all rtas_ld()/rtas_st() to do the same thing. But the patch which for this is about 1000 lines long as CPUPPCState* needs to be passed to every call of rtas_ld/rtas_st, and there are many of those. And it would not fix any actual problem so I am in doubts about it. Or may be there is some thread local storage in QEMU where I could keep PPCCPUState and use it in rtas_ld/rtas_st? -- Alexey