[Qemu-devel] Re: [PATCH] kvm: ppc: fixes for KVM_SET_SREGS on init

[Alexander Graf]

On 08.04.2011, at 23:48, Scott Wood wrote:

> Classic/server ppc has had SREGS for a while now (though I think not
> always?), but it's still missing for booke.  Check the capability before
> calling KVM_SET_SREGS.
> 
> Without this, booke kvm fails to boot as of commit
> 84b4915dd2c0eaa86c970ffc42a68ea8ba9e48b5 (kvm: Handle kvm_init_vcpu
> errors).

Oops :).

> 
> Also, don't write random stack state into the non-PVR sregs fields --
> have kvm fill it in first.
> 
> Eventually booke will have sregs and it will have its own capability to
> be tested here.  However, we will want a way for platform code to request
> to look like the actual CPU we're running on, especially if SoC devices
> are being directly assigned.
> 
> Signed-off-by: Scott Wood <address@hidden>
> ---
> target-ppc/kvm.c |   33 ++++++++++++++++++++++++++++++---
> 1 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 2cfb24b..5401536 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -77,13 +77,40 @@ int kvm_arch_init(KVMState *s)
>     return 0;
> }
> 
> -int kvm_arch_init_vcpu(CPUState *cenv)
> +static int kvm_arch_sync_sregs(CPUState *cenv)

huh? So what about the previous caller of this?

> {
> -    int ret = 0;
>     struct kvm_sregs sregs;
> +    int ret;

Eh - this makes the patch less readable :)

> +
> +#ifdef TARGET_PPC
> +#ifdef KVM_CAP_PPC_SEGSTATE

This code never gets compiled without TARGET_PPC?

> +    if (!kvm_check_extension(cenv->kvm_state, KVM_CAP_PPC_SEGSTATE)) {
> +        return 0;
> +    }
> +#else
> +    return 0;

Doing a simple return 0 might lead to warnings (which become errors with 
-Werror) due to unused variables. I'm not sure how to handle this well. Maybe 
define KVM_CAP_PPC_SEGSTATE to something invalid when it's not defined? That 
way the capability check would fail and we don't need the duplicate code paths.

> +#endif
> +#else /* TARGET_PPCEMB */

I guess you were #ifdefing on PPCEMB before? I don't think you really need to 
care about PPCEMB. The only reason it exists is for page size < 4k, which you 
don't hit on e500 IIUC.

> +    return 0;
> +#endif
> +    
> +    ret = kvm_vcpu_ioctl(cenv, KVM_GET_SREGS, &sregs);
> +    if (ret) {
> +        return ret;
> +    }
> 
>     sregs.pvr = cenv->spr[SPR_PVR];
> -    ret = kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs);
> +    return kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs);
> +}
> +
> +int kvm_arch_init_vcpu(CPUState *cenv)
> +{
> +    int ret;
> +
> +    ret = kvm_arch_sync_sregs(cenv);
> +    if (ret) {
> +        return ret;
> +    }
> 
>     idle_timer = qemu_new_timer_ns(vm_clock, kvm_kick_env, cenv);
> 
> -- 
> 1.7.1
> 


Alex