Re: [PATCH v3 9/9] kvmtool: virtio: enable arm/arm64 support for bi-endianness

[Marc Zyngier <marc.zyngier@arm.com>]

Hi Will,

On Tue, May 06 2014 at  3:28:07 pm BST, Will Deacon <will.deacon@arm.com> wrote:
> Hi Marc,
>
> On Thu, Apr 24, 2014 at 07:17:23PM +0100, Marc Zyngier wrote:
>> Implement the kcm_cpu__get_endianness call for both AArch32 and
>
> s/kcm/kvm/

Are you saying that I hace fat fingers? ;-)

>> AArch64, and advertise the bi-endianness support.
>> 
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  tools/kvm/arm/aarch32/kvm-cpu.c                  | 14 +++++++++++++
>>  tools/kvm/arm/aarch64/include/kvm/kvm-cpu-arch.h |  2 ++
>>  tools/kvm/arm/aarch64/kvm-cpu.c                  | 25 ++++++++++++++++++++++++
>>  tools/kvm/arm/include/arm-common/kvm-arch.h      |  2 ++
>>  4 files changed, 43 insertions(+)
>> 
>> diff --git a/tools/kvm/arm/aarch32/kvm-cpu.c b/tools/kvm/arm/aarch32/kvm-cpu.c
>> index bd71037..464b473 100644
>> --- a/tools/kvm/arm/aarch32/kvm-cpu.c
>> +++ b/tools/kvm/arm/aarch32/kvm-cpu.c
>> @@ -1,5 +1,6 @@
>>  #include "kvm/kvm-cpu.h"
>>  #include "kvm/kvm.h"
>> +#include "kvm/virtio.h"
>>  
>>  #include <asm/ptrace.h>
>>  
>> @@ -76,6 +77,19 @@ void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu)
>>  		die_perror("KVM_SET_ONE_REG failed (pc)");
>>  }
>>  
>> +int kvm_cpu__get_endianness(struct kvm_cpu *vcpu)
>> +{
>> +	struct kvm_one_reg reg;
>> +	u32 data;
>> +
>> +	reg.id = ARM_CORE_REG(usr_regs.ARM_cpsr);
>> +	reg.addr = (u64)(unsigned long)&data;
>> +	if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, &reg) < 0)
>> +		die("KVM_GET_ONE_REG failed (cpsr)");
>> +
>> +	return (data & PSR_E_BIT) ? VIRTIO_ENDIAN_BE : VIRTIO_ENDIAN_LE;
>> +}
>> +
>>  void kvm_cpu__show_code(struct kvm_cpu *vcpu)
>>  {
>>  	struct kvm_one_reg reg;
>> diff --git a/tools/kvm/arm/aarch64/include/kvm/kvm-cpu-arch.h b/tools/kvm/arm/aarch64/include/kvm/kvm-cpu-arch.h
>> index 7d70c3b..ed7da45 100644
>> --- a/tools/kvm/arm/aarch64/include/kvm/kvm-cpu-arch.h
>> +++ b/tools/kvm/arm/aarch64/include/kvm/kvm-cpu-arch.h
>> @@ -13,5 +13,7 @@
>>  #define ARM_MPIDR_HWID_BITMASK	0xFF00FFFFFFUL
>>  #define ARM_CPU_ID		3, 0, 0, 0
>>  #define ARM_CPU_ID_MPIDR	5
>> +#define ARM_CPU_CTRL		3, 0, 1, 0
>> +#define ARM_CPU_CTRL_SCTLR	0
>>  
>>  #endif /* KVM__KVM_CPU_ARCH_H */
>> diff --git a/tools/kvm/arm/aarch64/kvm-cpu.c b/tools/kvm/arm/aarch64/kvm-cpu.c
>> index 059e42c..b3ce2c8 100644
>> --- a/tools/kvm/arm/aarch64/kvm-cpu.c
>> +++ b/tools/kvm/arm/aarch64/kvm-cpu.c
>> @@ -1,12 +1,16 @@
>>  #include "kvm/kvm-cpu.h"
>>  #include "kvm/kvm.h"
>> +#include "kvm/virtio.h"
>>  
>>  #include <asm/ptrace.h>
>>  
>>  #define COMPAT_PSR_F_BIT	0x00000040
>>  #define COMPAT_PSR_I_BIT	0x00000080
>> +#define COMPAT_PSR_E_BIT	0x00000200
>>  #define COMPAT_PSR_MODE_SVC	0x00000013
>>  
>> +#define SCTLR_EL1_EE_MASK	(1 << 25)
>> +
>>  #define ARM64_CORE_REG(x)	(KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
>>  				 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>>  
>> @@ -133,6 +137,27 @@ void kvm_cpu__reset_vcpu(struct kvm_cpu *vcpu)
>>  		return reset_vcpu_aarch64(vcpu);
>>  }
>>  
>> +int kvm_cpu__get_endianness(struct kvm_cpu *vcpu)
>> +{
>> +	struct kvm_one_reg reg;
>> +	u64 data;
>> +
>> +	reg.id = ARM64_CORE_REG(regs.pstate);
>> +	reg.addr = (u64)&data;
>> +	if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, &reg) < 0)
>> +		die("KVM_GET_ONE_REG failed (spsr[EL1])");
>
> This bit hurt for a while ;) Can you add a comment mentioning SETEND for
> AArch32 guests please?

Sure.

>> +	if (data & PSR_MODE32_BIT)
>> +		return (data & COMPAT_PSR_E_BIT) ? VIRTIO_ENDIAN_BE : VIRTIO_ENDIAN_LE;
>> +
>> +	reg.id = ARM64_SYS_REG(ARM_CPU_CTRL, ARM_CPU_CTRL_SCTLR); /* SCTLR_EL1 */
>
> We should probably just rename ARM_CPU_CTRL_SCTLR that to include the _EL1
> suffix.

Fair enough.

>> +	reg.addr = (u64)&data;
>> +	if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, &reg) < 0)
>> +		die("KVM_GET_ONE_REG failed (SCTLR_EL1)");
>> +
>> +	return (data & SCTLR_EL1_EE_MASK) ? VIRTIO_ENDIAN_BE : VIRTIO_ENDIAN_LE;
>
> This rules out guests where userspace and kernelspace can run with different
> endinness. Whilst Linux doesn't currently do this, can we support it here?
> It all gets a bit hairy if the guest is using a stage-1 SMMU to let
> userspace play with a virtio device...

Yeah, I suppose we could check either EE or E0 depending on the mode
when the access was made. We already have all the information, just need
to handle the case. I'll respin the series.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.