Re: [Qemu-devel] [PATCH] spapr-rtas: reset top 4 bits in parameters address

[Alexey Kardashevskiy]

On 09/06/2013 04:22 PM, Alexander Graf wrote:
> 
> 
> 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 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