Re: [Qemu-devel] [PATCH 3/5] pseries: Add H_SET_MODE hcall to change guest exception endianness

[Anthony Liguori]

On Tue, Aug 6, 2013 at 7:47 PM, Anton Blanchard <address@hidden> wrote:
> H_SET_MODE is used for controlling various partition settings. One
> of these settings is the endianness a guest takes its exceptions in.
>
> Signed-off-by: Anton Blanchard <address@hidden>
> ---
>  hw/ppc/spapr.c         |  2 +-
>  hw/ppc/spapr_hcall.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h | 12 +++++++++++-
>  3 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 16bfab9..de639f6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -262,7 +262,7 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>      uint32_t start_prop = cpu_to_be32(initrd_base);
>      uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size);
>      char hypertas_prop[] = 
> "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt"
> -        "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk";
> +        "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode";
>      char qemu_hypertas_prop[] = "hcall-memop1";
>      uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)};
>      uint32_t interrupt_server_ranges_prop[] = {0, cpu_to_be32(smp_cpus)};
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 67d6cd9..79e1b61 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -657,6 +657,48 @@ static target_ulong h_logical_dcbf(PowerPCCPU *cpu, 
> sPAPREnvironment *spapr,
>      return H_SUCCESS;
>  }
>
> +static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                               target_ulong opcode, target_ulong *args)
> +{
> +    CPUState *cs;
> +    target_ulong mflags = args[0];
> +    target_ulong resource = args[1];
> +    target_ulong value1 = args[2];
> +    target_ulong value2 = args[3];
> +
> +    if (resource == 4) {

This ought to be a #define.  There's no else here, is that expected?
Should you return failure for a different resource?

> +        if (value1) {
> +            return H_P3;
> +        }
> +        if (value2) {
> +            return H_P4;
> +        }
> +
> +        switch (mflags) {
> +        case 0:
> +            for (cs = first_cpu; cs != NULL; cs = cs->next_cpu) {
> +                PowerPCCPU *cp = POWERPC_CPU(cs);
> +                CPUPPCState *env = &cp->env;
> +                env->spr[SPR_LPCR] &= ~LPCR_ILE;
> +            }
> +            return H_SUCCESS;
> +
> +        case 1:
> +            for (cs = first_cpu; cs != NULL; cs = cs->next_cpu) {
> +                PowerPCCPU *cp = POWERPC_CPU(cs);
> +                CPUPPCState *env = &cp->env;
> +                env->spr[SPR_LPCR] |= LPCR_ILE;
> +            }
> +            return H_SUCCESS;

Without knowing this interface better, a few things come to mind.

Is mflags a boolean?  If so, you can reduce this to a single loop and
drop the switch() statement.  If mflags is truly a set of flags, it
would be nice to use #define to give the flags a proper symbolic name.

Regards,

Anthony Liguori

> +        default:
> +            return H_UNSUPPORTED_FLAG;
> +        }
> +    }
> +
> +    return H_P2;
> +}
> +
>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - 
> KVMPPC_HCALL_BASE + 1];
>
> @@ -734,6 +776,8 @@ static void hypercall_register_types(void)
>
>      /* qemu/KVM-PPC specific hcalls */
>      spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);
> +
> +    spapr_register_hypercall(H_SET_MODE, h_set_mode);
>  }
>
>  type_init(hypercall_register_types)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9fc1972..3ceec7a 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -109,7 +109,16 @@ typedef struct sPAPREnvironment {
>  #define H_NOT_ENOUGH_RESOURCES -44
>  #define H_R_STATE         -45
>  #define H_RESCINDEND      -46
> +#define H_P2              -55
> +#define H_P3              -56
> +#define H_P4              -57
> +#define H_P5              -58
> +#define H_P6              -59
> +#define H_P7              -60
> +#define H_P8              -61
> +#define H_P9              -62
>  #define H_MULTI_THREADS_ACTIVE -9005
> +#define H_UNSUPPORTED_FLAG -256
>
>
>  /* Long Busy is a condition that can be returned by the firmware
> @@ -267,7 +276,8 @@ typedef struct sPAPREnvironment {
>  #define H_GET_EM_PARMS          0x2B8
>  #define H_SET_MPP               0x2D0
>  #define H_GET_MPP               0x2D4
> -#define MAX_HCALL_OPCODE        H_GET_MPP
> +#define H_SET_MODE              0x31C
> +#define MAX_HCALL_OPCODE        H_SET_MODE
>
>  /* The hcalls above are standardized in PAPR and implemented by pHyp
>   * as well.
> --
> 1.8.1.2
>
>