target-arm: Add guest cpu endianness determination for virtio in KVM ARM64

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 <pranavkumar@linaro.org>
---
 include/hw/virtio/virtio-access.h |  2 ++
 target-arm/cpu64.c                | 41 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

On 28 October 2014 06:38, Pranavkumar Sawargaonkar
<pranavkumar@linaro.org> 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 <pranavkumar@linaro.org>
> ---
>  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

Hi PMM,

On 28 October 2014 16:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 October 2014 06:38, Pranavkumar Sawargaonkar
> <pranavkumar@linaro.org> 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 <pranavkumar@linaro.org>
>> ---
>>  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