[RFC PATCH 2/3] ARM/ARM64: KVM: Add support for PSCI v0.2 emulation

[christoffer.dall@linaro.org (Christoffer Dall)]

On Wed, Jan 29, 2014 at 10:22:47AM +0530, Anup Patel wrote:
> On Wed, Jan 29, 2014 at 2:34 AM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Tue, Jan 21, 2014 at 06:31:40PM +0530, Anup Patel wrote:

[...]

> >
> > Which tree does this patch apply to?  It looks like you'll get a
> > conflict with:
> > 478a823 arm: KVM: Don't return PSCI_INVAL if waitqueue is inactive
> 
> This patchset applies on v3.13 tag of Torvalds tree.

That would not contain anything in kvm/next or kvm-arm-next.

> 
> I generally base my patches on latest stable/rc tag of Torvalds tree
> so that I can provide KVM patches to folks interested in trying KVM
> on X-Gene with latest Linux stable/rc.

If you want to make it slightly easier for me or Marc to apply your
patches in general I would recommend basing them off kvm/next or
kvm-arm-next, but it's no big deal.

In this case all you need to consider is already in linus/master.

> 
> I will make sure that revised patchset applies on top of
> 478a823 arm: KVM: Don't return PSCI_INVAL if waitqueue is inactive
> 
> >
> >>       }
> >>
> >> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c
> >> index 0881bf1..ee044a3 100644
> >> --- a/arch/arm/kvm/psci.c
> >> +++ b/arch/arm/kvm/psci.c
> >> @@ -55,13 +55,13 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> >>       }
> >>
> >>       if (!vcpu)
> >> -             return KVM_PSCI_RET_INVAL;
> >> +             return KVM_PSCI_RET_INVALID_PARAMS;
> >>
> >>       target_pc = *vcpu_reg(source_vcpu, 2);
> >>
> >>       wq = kvm_arch_vcpu_wq(vcpu);
> >>       if (!waitqueue_active(wq))
> >> -             return KVM_PSCI_RET_INVAL;
> >> +             return KVM_PSCI_RET_INVALID_PARAMS;
> >>
> >>       kvm_reset_vcpu(vcpu);
> >>
> >> @@ -84,17 +84,49 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> >>       return KVM_PSCI_RET_SUCCESS;
> >>  }
> >>
> >> -/**
> >> - * kvm_psci_call - handle PSCI call if r0 value is in range
> >> - * @vcpu: Pointer to the VCPU struct
> >> - *
> >> - * Handle PSCI calls from guests through traps from HVC instructions.
> >> - * The calling convention is similar to SMC calls to the secure world where
> >> - * the function number is placed in r0 and this function returns true if the
> >> - * function number specified in r0 is withing the PSCI range, and false
> >> - * otherwise.
> >> - */
> >> -bool kvm_psci_call(struct kvm_vcpu *vcpu)
> >> +static bool kvm_psci_0_2_call(struct kvm_vcpu *vcpu)
> >> +{
> >> +     unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
> >> +     unsigned long val;
> >> +
> >> +     switch (psci_fn) {
> >> +     case KVM_PSCI_0_2_FN_PSCI_VERSION:
> >> +             /*
> >> +              * Bits[31:16] = Major Version = 0
> >> +              * Bits[15:0] = Minor Version = 2
> >> +              */
> >> +             val = 2;
> >> +             break;
> >> +     case KVM_PSCI_0_2_FN_CPU_OFF:
> >> +             kvm_psci_vcpu_off(vcpu);
> >> +             val = KVM_PSCI_RET_SUCCESS;
> >> +             break;
> >> +     case KVM_PSCI_0_2_FN_CPU_ON:
> >> +     case KVM_PSCI_0_2_FN64_CPU_ON:
> >> +             val = kvm_psci_vcpu_on(vcpu);
> >> +             break;
> >> +     case KVM_PSCI_0_2_FN_CPU_SUSPEND:
> >> +     case KVM_PSCI_0_2_FN_AFFINITY_INFO:
> >> +     case KVM_PSCI_0_2_FN_MIGRATE:
> >> +     case KVM_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> >> +     case KVM_PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> >> +     case KVM_PSCI_0_2_FN_SYSTEM_OFF:
> >> +     case KVM_PSCI_0_2_FN_SYSTEM_RESET:
> >> +     case KVM_PSCI_0_2_FN64_CPU_SUSPEND:
> >> +     case KVM_PSCI_0_2_FN64_AFFINITY_INFO:
> >> +     case KVM_PSCI_0_2_FN64_MIGRATE:
> >> +     case KVM_PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> >> +             val = KVM_PSCI_RET_NOT_SUPPORTED;
> >> +             break;
> >> +     default:
> >> +             return false;
> >> +     }
> >> +
> >> +     *vcpu_reg(vcpu, 0) = val;
> >> +     return true;
> >> +}
> >> +
> >> +static bool kvm_psci_0_1_call(struct kvm_vcpu *vcpu)
> >>  {
> >>       unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
> >>       unsigned long val;
> >> @@ -109,9 +141,8 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu)
> >>               break;
> >>       case KVM_PSCI_FN_CPU_SUSPEND:
> >>       case KVM_PSCI_FN_MIGRATE:
> >> -             val = KVM_PSCI_RET_NI;
> >> +             val = KVM_PSCI_RET_NOT_SUPPORTED;
> >>               break;
> >> -
> >>       default:
> >>               return false;
> >>       }
> >> @@ -119,3 +150,21 @@ bool kvm_psci_call(struct kvm_vcpu *vcpu)
> >>       *vcpu_reg(vcpu, 0) = val;
> >>       return true;
> >>  }
> >> +
> >> +/**
> >> + * kvm_psci_call - handle PSCI call if r0 value is in range
> >> + * @vcpu: Pointer to the VCPU struct
> >> + *
> >> + * Handle PSCI calls from guests through traps from HVC instructions.
> >> + * The calling convention is similar to SMC calls to the secure world where
> >> + * the function number is placed in r0 and this function returns true if the
> >> + * function number specified in r0 is withing the PSCI range, and false
> >> + * otherwise.
> >> + */
> >> +bool kvm_psci_call(struct kvm_vcpu *vcpu)
> >> +{
> >> +     if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features))
> >> +             return kvm_psci_0_2_call(vcpu);
> >> +
> >> +     return kvm_psci_0_1_call(vcpu);
> >> +}
> >
> > Why don't we just try one after the other?  Do they conflict in some
> > way?
> 
> Atleast the functions IDs are totally different in v0.2 and v0.1
> 
> Also, in v0.2 we have separate function IDs for 32bit and 64bit
> VCPU calling same PSCI function.
> 

So we could just do:

{
	ret = kvm_psci_0_2_call(vcpu);
	if (ret)
		return ret;

	ret = kvm_psci_0_1_call(vcpu);
	if (ret)
		return ret;

	return false;
}

and be rid of the vcpu feature, or?  I thought this was Marc's point in
the last KVM/ARM call?

> >
> > I assume PSCI calls are never going to be in the critical path and calls
> > into PSCI are pretty much expected to be slow as a dog anyhow, so if we
> > could avoid the extra churn in user space code and potential user
> > confusion (providing PSCI 0.2 kernel but too old user space tool for
> > example), I think that would be preferred.
> 
> Yes, PSCI calls will not be in critical path except few functions such as
> PSCI CPU_SUSPEND and CPU_ON.
> 
> For example,
> On real HW, people are very much interested in time taken to resume a
> HW CPU from suspended state because this affects responsiveness of
> a system.
> 

In which case time taken to wake up from suspended state in a VM will
for sure not be dominated by an extra call to a psci function id
checking function.

Thanks,
-Christoffer