Re: [Qemu-devel] [PATCH V2 2/2] target-arm: Guest cpu endianness determination for virtio KVM ARM/ARM64

[Peter Maydell]

On 22 December 2014 at 09:04, Pranavkumar Sawargaonkar
<address@hidden> wrote:
> This patch implements a fucntion pointer virtio_is_big_endian()
> from "CPUClass" structure for arm/arm64.
> Function arm_cpu_virtio_endianness() is added to determine and
> return the guest cpu endianness to virtio.
> This is required for running cross endian guests with virtio on ARM/ARM64.
>
> Changelog:

The changelog should go below the "---" line, ideally.

> V2:
> - Change the patch to be used generically for both ARM and ARM64.
> - Address the review comments received in v1.
>
> V1:
> - First patch in this series.
> - http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg03321.html
>
> Signed-off-by: Pranavkumar Sawargaonkar <address@hidden>
> ---
>  target-arm/cpu.c | 27 +++++++++++++++++++++++++++
>  target-arm/cpu.h |  2 ++
>  2 files changed, 29 insertions(+)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index d3db279..7145e02 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -320,6 +320,32 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, 
> int level)
>      kvm_set_irq(kvm_state, kvm_irq, level ? 1 : 0);
>  #endif
>  }
> +
> +static bool arm_cpu_virtio_endianness(CPUState *cs)

You should call this arm_cpu_is_big_endian(), which lines
up with the PPC function name and is closer to the QOM
method name we're implementing.

> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    uint64_t sctlr;
> +
> +    cpu_synchronize_state(cs);
> +
> +    /* Check if we are running 32bit guest or not */

I don't think this comment is particularly informative.

> +    if (!is_a64(env)) {
> +        return (env->pstate & CPSR_E) ? 1 : 0;
> +    }
> +
> +    sctlr = env->cp15.sctlr_el[1];
> +
> +    if ((env->pstate & 0xf) == PSTATE_MODE_EL0t) {
> +        sctlr &= SCTLR_E0E;
> +    } else {
> +        sctlr &= SCTLR_EE;
> +    }

We might as well make this robust for when we implement EL2 and EL3:
    int cur_el;
    ...

    cur_el = arm_current_el(env);
    if (cur_el == 0) {
        return (env->cp15.sctlr_el[1] & SCTLR_E0E) != 0;
    } else {
        return (env->cp15.sctlr_el[cur_el] & SCTLR_EE) != 0;
    }

> +
> +    /* If BIG-ENDIAN return 1 */

...and with the new function name this comment isn't necessary
either, because it's obvious.

> +    return sctlr ? 1 : 0;
> +}
> +
>  #endif
>
>  static inline void set_feature(CPUARMState *env, int feature)
> @@ -1157,6 +1183,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->do_interrupt = arm_cpu_do_interrupt;
>      cc->get_phys_page_debug = arm_cpu_get_phys_page_debug;
>      cc->vmsd = &vmstate_arm_cpu;
> +    cc->virtio_is_big_endian = arm_cpu_virtio_endianness;
>  #endif
>      cc->gdb_num_core_regs = 26;
>      cc->gdb_core_xml_file = "arm-core.xml";
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 7ba55f0..f32a5b6 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -32,6 +32,8 @@
>  #  define ELF_MACHINE EM_ARM
>  #endif
>
> +#define TARGET_IS_BIENDIAN 1
> +
>  #define CPUArchState struct CPUARMState
>
>  #include "qemu-common.h"
> --
> 1.9.1

thanks
-- PMM