Re: [Qemu-devel] [PATCH v3 05/11] AARCH64: Add AARCH64 CPU initialization, get and put registers support

[Peter Maydell]

On 27 September 2013 11:10, Mian M. Hamayun
<address@hidden> wrote:
> From: "Mian M. Hamayun" <address@hidden>
>
> The cpu init function tries to initialize with all possible cpu types, as
> KVM does not provide a means to detect the real cpu type and simply refuses
> to initialize on cpu type mis-match. By using the loop based init function,
> we avoid the need to modify code if the underlying platform is different,
> such as Fast Models instead of Foundation Models.
>
> Get and Put Registers deal with the basic state of AARCH64 CPUs.

Hi; this is another review mail mostly to flag up a few things I'm
fixing up as I go along with these patches.

>  int kvm_arch_put_registers(CPUState *cs, int level)
>  {
> -    return 0;
> +    struct kvm_one_reg reg;
> +    int i;
> +    int ret;
> +
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +
> +    for (i = 0; i < ARRAY_SIZE(env->xregs); i++) {

This upper bound using ARRAY_SIZE means we'll try
to do a read/write of xregs[31], which isn't a core register
(it's SP for QEMU, as you can see below).

> +        reg.id = AARCH64_CORE_REG(regs.regs[i]);
> +        reg.addr = (uintptr_t) &env->xregs[i];
> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
> +    reg.id = AARCH64_CORE_REG(regs.sp);
> +    reg.addr = (uintptr_t) &env->xregs[31];
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret) {
> +        return ret;
> +    }

> +    reg.id = AARCH64_CORE_REG(regs.pstate);
> +    reg.addr = (uintptr_t) &env->pstate;

This will tell the kernel to read/write 64 bits of data into
env->pstate, which is a uint32_t. We need to use a local
uint64_t to pass the data through.

(I don't know why the kernel thinks PSTATE is a 64 bit
register...)

thanks
-- PMM