Re: [Qemu-devel] [PATCH v2] spapr-rtas: use softmmu for accessing rtas call parameters

[Alexander Graf]

On 06.09.2013, at 10:10, Alexey Kardashevskiy wrote:

> 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 use softmmu which can chop top 4 bits
> if MSR DR is not set.
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
> Changes:
> v2:
> * masking from replaced with the use of cpu_ldl_data which can handle
> realmode case properly
> ---
> hw/ppc/spapr_hcall.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 063bd36..30f90bf 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -4,6 +4,7 @@
> #include "hw/ppc/spapr.h"
> #include "mmu-hash64.h"
> #include "cpu-models.h"
> +#include "exec/softmmu_exec.h"
> 
> #include <libfdt.h>
> 
> @@ -523,10 +524,11 @@ static target_ulong h_cede(PowerPCCPU *cpu, 
> sPAPREnvironment *spapr,
> static target_ulong h_rtas(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                            target_ulong opcode, target_ulong *args)
> {
> +    CPUPPCState *env = &cpu->env;
>     target_ulong rtas_r3 = args[0];
> -    uint32_t token = ldl_be_phys(rtas_r3);
> -    uint32_t nargs = ldl_be_phys(rtas_r3 + 4);
> -    uint32_t nret = ldl_be_phys(rtas_r3 + 8);
> +    uint32_t token = cpu_ldl_data(env, rtas_r3);
> +    uint32_t nargs = cpu_ldl_data(env, rtas_r3 + 4);
> +    uint32_t nret = cpu_ldl_data(env, rtas_r3 + 8);

Wow - this really aren't that many memory accesses.

So looking through rtas calls that come after this dispatch, from what I see 
they mostly use rtas_ld and rtas_st. They are defined as

static inline uint32_t rtas_ld(target_ulong phys, int n)
{
    return ldl_be_phys(phys + 4*n);
}

static inline void rtas_st(target_ulong phys, int n, uint32_t val)
{
    stl_be_phys(phys + 4*n, val);
}

which means we need to change them as well to make rtas subcall memory accesses 
resolve properly.

Which brings me to my next question: Why don't we use rtas_ld in the 3 calls 
above? It looks like a perfect fit really.

Also, just changing the call to cpu_ldl_data will potentially break I think. 
Imagine you do cpu_synchronize_state with DR=1 in a previous call. Now comes an 
RTAS call. Because you don't do a cpu_synchronize_state() call here, you will 
still have the DR=1 in MSR, accessing virtual memory at a potentially very low 
address that may not be mapped in.

So before you do the cpu_ldl_data you need to either do cpu_synchronize_state() 
- which can be slow - or you need to manually change MSR.DR to 0 so that we end 
up accessing the correct memory location always. If you encapsulate that in 
rtas_ld and rtas_st it might end up looking quite simple:

static inline uint32_t rtas_ld(CPUPPCState *env, target_ulong phys, int n)
{
    uint32_t r;
    target_ulong msr = env->msr;

    /* Make sure we access RTAS data in guest real mode */
    env->msr ~= MSR_DR;
    r = ldl_be_phys(phys + 4*n);

    env->msr = msr;
    return r;
}

Or if you think it's too complicated, you can simply make it

static inline uint64_t ppc64_phys_to_real(uint64_t addr)
{
    return addr & ~0xF000000000000000ULL;
}

static inline uint32_t rtas_ld(target_ulong phys, int n)
{
    return ldl_be_phys(ppc64_phys_to_real(phys) + 4*n);
}

All things considered, I might even prefer the latter solution as it makes the 
code flow more obvious. But I'll leave the final call to you.


Alex