Re: [Qemu-devel] [PATCH] target-arm: Add guest cpu endianness determination for virtio in KVM ARM64

[Peter Maydell]

On 28 October 2014 06:38, Pranavkumar Sawargaonkar
<address@hidden> wrote:
> This patch implements a fucntion pointer virtio_is_big_endian()
> from "CPUClass" structure for arm64.
> Function aarch64_cpu_virtio_endianness() is added to determine and
> returns the guest cpu endianness to virtio.
> This is required for running cross endian guests with virtio on ARM64.

Thanks for this patch (and the associated testing).
Is this the only thing we need for big-endian guest kernels,
or are there other patches to follow? I thought we needed something
in the kernel loader code too...

> Signed-off-by: Pranavkumar Sawargaonkar <address@hidden>
> ---
>  include/hw/virtio/virtio-access.h |  2 ++
>  target-arm/cpu64.c                | 41 
> +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/include/hw/virtio/virtio-access.h 
> b/include/hw/virtio/virtio-access.h
> index 46456fd..84fa701 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -23,6 +23,8 @@ static inline bool virtio_access_is_big_endian(VirtIODevice 
> *vdev)
>      return virtio_is_big_endian(vdev);
>  #elif defined(TARGET_WORDS_BIGENDIAN)
>      return true;
> +#elif defined(TARGET_AARCH64)
> +    return virtio_is_big_endian(vdev);

As Greg says, we can just have the ARM cpu.h define TARGET_BIENDIAN.

>  #else
>      return false;
>  #endif
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index c30f47e..789f886 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -192,6 +192,43 @@ static void aarch64_cpu_set_pc(CPUState *cs, vaddr value)
>      }
>  }
>
> +#ifndef CONFIG_USER_ONLY
> +
> +#define KVM_REG_ARM64_SCTLR_EL1 3, 0, 1, 0, 0
> +
> +static bool aarch64_cpu_virtio_endianness(CPUState *cs)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    uint64_t sctlr;
> +
> +    cpu_synchronize_state(cs);
> +
> +    /* Check if we are running 32bit guest or not */
> +    if (!is_a64(env))
> +        return (env->pstate & CPSR_E) ? 1 : 0;

If you run your patches through checkpatch.pl you'll find
they don't correspond with QEMU's coding style here.

> +
> +    /* Ideally we do not need to call IOCTL again to read SCTLR_EL1 value.
> +     * cpu_synchronize_state() should fill the env->cp15.c1_sys
> +     * to get this value but this path is currently not implemented for 
> arm64.
> +     * Hence this is a temporary fix.
> +     */
> +
> +    reg.id = ARM64_SYS_REG(KVM_REG_ARM64_SCTLR_EL1);

(This won't compile on non-linux hosts, incidentally, because
the ARM64_SYS_REG macro is in the kvm headers.)

> +    reg.addr = (uint64_t) &sctlr;
> +    kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);

As you say really we need to implement sysreg state
synchronization properly (IIRC Alex B had some patches
for this). I don't think we want to take this patch as-is,
though it's very helpful for demonstrating that everything
else works.

> +
> +    if ((env->pstate & 0xf) == PSTATE_MODE_EL0t)
> +        sctlr &= (1U <<24);
> +    else
> +        sctlr &= (1U <<25);

We have SCTLR_E0E and SCTLR_EE defines in cpu.h for these bits now.

> +
> +    /* If BIG-ENDIAN return 1 */
> +    return sctlr ? 1 : 0;
> +}
> +#endif
> +
>  static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      CPUClass *cc = CPU_CLASS(oc);
> @@ -203,6 +240,10 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->gdb_write_register = aarch64_cpu_gdb_write_register;
>      cc->gdb_num_core_regs = 34;
>      cc->gdb_core_xml_file = "aarch64-core.xml";
> +#ifndef CONFIG_USER_ONLY
> +    cc->virtio_is_big_endian = aarch64_cpu_virtio_endianness;

An implementation that doesn't need to work around our lack of
aarch64 sysreg syncing could be implemented in the ARM cpu class
so it worked for both 32 bit and 64 bit CPUs, I think.

> +#endif
> +
>  }
>
>  static void aarch64_cpu_register(const ARMCPUInfo *info)

thanks
-- PMM