Re: [Qemu-ppc] [QEMU-ppc for-2.10][PATCH V4 1/6] target/ppc: Set UPRT and GTSE on all cpus in H_REGISTER_PROCESS_TABLE

[David Gibson]

On Thu, Apr 13, 2017 at 04:02:35PM +1000, Suraj Jitindar Singh wrote:
> The UPRT and GTSE bits are set when a guest calls H_REGISTER_PROCESS_TABLE
> to choose determine how address translation is performed. Currently these
> bits in the LPCR are only set for the cpu which handles the H_CALL, however
> they need to be set for all cpus for that guest as address translation
> cannot be performed differently on a per cpu basis.

Oops, should have spotted that the first time around.  Oh well.

> Update the H_CALL handler to set these bits in the LPCR correctly for all
> cpus of the guest.
> 
> Note it is the reponsibility of the guest to ensure that any secondary cpus
> are suspended when the H_CALL is made and thus we can safely update these
> values here.
> 
> Signed-off-by: Suraj Jitindar Singh <address@hidden>
> 
> ---
> 
> -> V4:
> 
> - Added patch to series
> ---
>  hw/ppc/spapr_hcall.c | 49 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 44f4489..fd8f4b4 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -924,6 +924,38 @@ static void spapr_check_setup_free_hpt(sPAPRMachineState 
> *spapr,
>      return;
>  }
>  
> +typedef struct {
> +    target_ulong mask;
> +    bool set;
> +} SetLPCRState;
> +
> +static void do_set_lpcr(CPUState *cs, run_on_cpu_data arg)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    SetLPCRState *s = arg.host_ptr;
> +
> +    if (s->set) {
> +        env->spr[SPR_LPCR] |= s->mask;
> +    } else {
> +        env->spr[SPR_LPCR] &= ~s->mask;
> +    }
> +}

I'm finding the mask + set/clear flag a little confusing.

I'd suggest instead you supply a mask and value, both target_ulong.
The callback would clear the mask bits, then set the value bits.
i.e. mask is the bits you want to allow to be changed, value is the
value to set them too.

I think that will be a bit clearer, as well as being more flexible.

> +
> +static void ppc_set_lpcr_all(target_ulong mask, bool set)
> +{
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        SetLPCRState s = {
> +            .mask = mask,
> +            .set = set
> +        };
> +
> +        run_on_cpu(cs, do_set_lpcr, RUN_ON_CPU_HOST_PTR(&s));
> +    }
> +}
> +
>  #define FLAGS_MASK              0x01FULL
>  #define FLAG_MODIFY             0x10
>  #define FLAG_REGISTER           0x08
> @@ -936,7 +968,6 @@ static target_ulong h_register_process_table(PowerPCCPU 
> *cpu,
>                                               target_ulong opcode,
>                                               target_ulong *args)
>  {
> -    CPUPPCState *env = &cpu->env;
>      target_ulong flags = args[0];
>      target_ulong proc_tbl = args[1];
>      target_ulong page_size = args[2];
> @@ -992,17 +1023,11 @@ static target_ulong 
> h_register_process_table(PowerPCCPU *cpu,
>      spapr_check_setup_free_hpt(spapr, spapr->patb_entry, cproc);
>  
>      spapr->patb_entry = cproc; /* Save new process table */
> -    if ((flags & FLAG_RADIX) || (flags & FLAG_HASH_PROC_TBL)) {
> -        /* Use Process TBL */
> -        env->spr[SPR_LPCR] |= LPCR_UPRT;
> -    } else {
> -        env->spr[SPR_LPCR] &= ~LPCR_UPRT;
> -    }
> -    if (flags & FLAG_GTSE) { /* Partition Uses Guest Translation Shootdwn */
> -        env->spr[SPR_LPCR] |= LPCR_GTSE;
> -    } else {
> -        env->spr[SPR_LPCR] &= ~LPCR_GTSE;
> -    }
> +
> +    /* Update the UPRT and GTSE bits in the LPCR for all cpus */
> +    ppc_set_lpcr_all(LPCR_UPRT, (flags & FLAG_RADIX) ||
> +                                (flags & FLAG_HASH_PROC_TBL));
> +    ppc_set_lpcr_all(LPCR_GTSE, !!(flags & FLAG_GTSE));
>  
>      if (kvm_enabled()) {
>          return kvmppc_configure_v3_mmu(cpu, flags & FLAG_RADIX,

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature