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@xxxxxxxxxx> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
