Hi PMM,
On 28 October 2014 16:18, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
> On 28 October 2014 06:38, Pranavkumar Sawargaonkar
> <pranavkumar@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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, ®);
>
> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
