[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: Alexey Kardashevskiy
Subject: Re: [Qemu-ppc] [PATCH] spapr-rtas: reset top 4 bits in parameters address
Date: Fri, 06 Sep 2013 15:04:29 +1000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7

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 :(


reply via email to

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