[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH] spapr-rtas: reset top 4 bits in parameters addres

From: Alexander Graf
Subject: Re: [Qemu-ppc] [PATCH] spapr-rtas: reset top 4 bits in parameters address
Date: Fri, 6 Sep 2013 08:22:03 +0200

Am 06.09.2013 um 07:04 schrieb Alexey Kardashevskiy <address@hidden>:

> 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 
>>>>>>>>>>> <address@hidden>:
>>>>>>>>>>>> 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 

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.


> -- 
> Alexey

reply via email to

[Prev in Thread] Current Thread [Next in Thread]