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

[Pranavkumar Sawargaonkar]

Hi Greg,

On 28 October 2014 14:56, Greg Kurz <address@hidden> wrote:
>
> On Tue, 28 Oct 2014 12:08:01 +0530
> 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.
> >
> > 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);
>
> This is code duplication of the TARGET_IS_BIENDIAN case. To be consistent
> with what was done for ppc64, you should have something like this instead:
>
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -27,6 +27,7 @@
>    /* AArch64 definitions */
>  #  define TARGET_LONG_BITS 64
>  #  define ELF_MACHINE EM_AARCH64
> +#  define TARGET_IS_BIENDIAN 1
>  #else
>  #  define TARGET_LONG_BITS 32
>  #  define ELF_MACHINE EM_ARM

Thanks, sure I will make this change in next revision.

Thanks,
Pranav

>
> >  #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;
> > +
> > +    /* 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);
> > +    reg.addr = (uint64_t) &sctlr;
> > +    kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +
> > +    if ((env->pstate & 0xf) == PSTATE_MODE_EL0t)
> > +        sctlr &= (1U <<24);
> > +    else
> > +        sctlr &= (1U <<25);
> > +
> > +    /* 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;
> > +#endif
> > +
> >  }
> >
> >  static void aarch64_cpu_register(const ARMCPUInfo *info)
>