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

[Pranavkumar Sawargaonkar]

Hi PMM,

On 28 October 2014 16:18, Peter Maydell <address@hidden> wrote:
> 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).

Thanks for reviewing this patch.
On testing to add a note, I have tested this patch with 3.17 LE host
kernel on a mustang board
and booted of LE and BE 3.17 kernels with virtio disk and net.

> 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...

This is the only patch needed for big-endian guest kernels to use virtio.
Most of the BE things are already taken care by KVM and kernel code only.

>
>> 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.

Sure I will change this.

>
>>  #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.

Oh sorry, I will correct this in next revision.

>
>> +
>> +    /* 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.

For the time-being I will post a v2 patch with incorporation of all
the remaining comments.
Once we have sysreg state implementation merged by Alex B, I will
revise v2 to post mergable version.

>
>> +
>> +    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.

Sure I will use those in place of hard-coding.
>
>> +
>> +    /* 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.

Yeah, we can to fix this for both arm32 and arm64 once the work-around
is removed.

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

Thanks,
Pranav