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

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

[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.

This fixes QEMU to do the same thing.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr_rtas.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index eb542f2..ab03d67 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -240,7 +240,8 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
         struct rtas_call *call = rtas_table + (token - TOKEN_BASE);
 
         if (call->fn) {
-            call->fn(cpu, spapr, token, nargs, args, nret, rets);
+            call->fn(cpu, spapr, token, nargs, args & 0x0FFFFFFFFFFFFFFFUL,
+                     nret, rets);
             return H_SUCCESS;
         }
     }
-- 
1.8.4.rc4

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

[Alexander Graf]

Am 05.09.2013 um 07:58 schrieb Alexey Kardashevskiy <aik@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?

Alex

> 
> This fixes QEMU to do the same thing.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> hw/ppc/spapr_rtas.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index eb542f2..ab03d67 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -240,7 +240,8 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>         struct rtas_call *call = rtas_table + (token - TOKEN_BASE);
> 
>         if (call->fn) {
> -            call->fn(cpu, spapr, token, nargs, args, nret, rets);
> +            call->fn(cpu, spapr, token, nargs, args & 0x0FFFFFFFFFFFFFFFUL,
> +                     nret, rets);
>             return H_SUCCESS;
>         }
>     }
> -- 
> 1.8.4.rc4
> 

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

[Alexey Kardashevskiy]

On 09/05/2013 05:08 PM, Alexander Graf wrote:
> 
> 
> Am 05.09.2013 um 07:58 schrieb Alexey Kardashevskiy <aik@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.

This particular patch was born after discovering GCC 4.8.0 bug with not
doing -0xc000.0000.0000.0000 correctly and this would not be a problem on
the real hardware. So I would think there are kernel somewhere which have
this bug. And there are few (honestly I found only one place and the patch
fixes it) places which can fail because of this GCC bug. So the patch does
make sense for Paul and myself.

btw the patch is wrong, I should do this in a different place, sorry about
that :)


> 
> Alex
> 
>>
>> This fixes QEMU to do the same thing.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> hw/ppc/spapr_rtas.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index eb542f2..ab03d67 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -240,7 +240,8 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>         struct rtas_call *call = rtas_table + (token - TOKEN_BASE);
>>
>>         if (call->fn) {
>> -            call->fn(cpu, spapr, token, nargs, args, nret, rets);
>> +            call->fn(cpu, spapr, token, nargs, args & 0x0FFFFFFFFFFFFFFFUL,
>> +                     nret, rets);
>>             return H_SUCCESS;
>>         }
>>     }
>> -- 
>> 1.8.4.rc4
>>


-- 
Alexey

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

[Alexander Graf]

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 <aik@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

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

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?


Alex

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

[Alexey Kardashevskiy]

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 <aik@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.


-- 
Alexey

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

[Alexander Graf]

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 <aik@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.


Alex

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

[Alexey Kardashevskiy]

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 <aik@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?



-- 
Alexey

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

[Alexander Graf]

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 <aik@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().

It automatically uses the current virtual layout the same way that the instruction emulator would do it - which is pretty much what we want.

IIRC you also have to enter RTAS calls with DR=0, so we wouldn't even need to flip any MSR bits when emulating RTAS calls, right?


Alex

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

[Alexey Kardashevskiy]

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 <aik@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 automatically uses the current virtual layout the same way that the instruction emulator would do it - which is pretty much what we want.
> 

> IIRC you also have to enter RTAS calls with DR=0, so we wouldn't even
> need to flip any MSR bits when emulating RTAS calls, right?

Probably. Right now cpu->env.msr==0x0 in rtas handler but not sure that I
see the real value.


-- 
Alexey

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

[Alexander Graf]

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 <aik@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.

> 
> 
>> It automatically uses the current virtual layout the same way that the instruction emulator would do it - which is pretty much what we want.
>> 
> 
>> IIRC you also have to enter RTAS calls with DR=0, so we wouldn't even
>> need to flip any MSR bits when emulating RTAS calls, right?
> 
> Probably. Right now cpu->env.msr==0x0 in rtas handler but not sure that I
> see the real value.

Make sure you run cpu_synchronize_state() before you look at cpu->env.msr.


Alex

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

[Alexey Kardashevskiy]

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 <aik@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.



>>> It automatically uses the current virtual layout the same way that the instruction emulator would do it - which is pretty much what we want.
>>>
>>
>>> IIRC you also have to enter RTAS calls with DR=0, so we wouldn't even
>>> need to flip any MSR bits when emulating RTAS calls, right?
>>
>> Probably. Right now cpu->env.msr==0x0 in rtas handler but not sure that I
>> see the real value.
> 
> Make sure you run cpu_synchronize_state() before you look at cpu->env.msr.

Oh, right.


-- 
Alexey

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

[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 <aik@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 :(


-- 
Alexey

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

[Alexander Graf]

Am 06.09.2013 um 07:04 schrieb Alexey Kardashevskiy <aik@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 <aik@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

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 <aik@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 <aik@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.


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