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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Christoffer,

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:
>> Currently, the in-kernel PSCI emulation provides PSCI v0.1 interface to
>> VCPUs. This patch extends current in-kernel PSCI emulation to provide
>> PSCI v0.2 interface to VCPUs.
>>
>> By default, ARM/ARM64 KVM will always provide PSCI v0.1 interface for
>> keeping the ABI backward-compatible.
>>
>> To select PSCI v0.2 interface for VCPUs, the user space (i.e. QEMU or
>> KVMTOOL) will have to set KVM_ARM_VCPU_PSCI_0_2 feature when doing VCPU
>> init using KVM_ARM_VCPU_INIT ioctl.
>>
>> Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxx>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>
>> ---
>>  arch/arm/include/asm/kvm_host.h   |    2 +-
>>  arch/arm/include/uapi/asm/kvm.h   |   39 ++++++++++++++++--
>>  arch/arm/kvm/arm.c                |    6 ++-
>>  arch/arm/kvm/psci.c               |   79 ++++++++++++++++++++++++++++++-------
>>  arch/arm64/include/asm/kvm_host.h |    2 +-
>>  arch/arm64/include/uapi/asm/kvm.h |   39 ++++++++++++++++--
>>  6 files changed, 143 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 8a6f6db..0239ac5 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -36,7 +36,7 @@
>>  #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
>>  #define KVM_HAVE_ONE_REG
>>
>> -#define KVM_VCPU_MAX_FEATURES 1
>> +#define KVM_VCPU_MAX_FEATURES 2
>>
>>  #include <kvm/arm_vgic.h>
>>
>> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
>> index c498b60..d9eb74c 100644
>> --- a/arch/arm/include/uapi/asm/kvm.h
>> +++ b/arch/arm/include/uapi/asm/kvm.h
>> @@ -83,6 +83,7 @@ struct kvm_regs {
>>  #define KVM_VGIC_V2_CPU_SIZE         0x2000
>>
>>  #define KVM_ARM_VCPU_POWER_OFF               0 /* CPU is started in OFF state */
>> +#define KVM_ARM_VCPU_PSCI_0_2                1 /* CPU uses PSCI v0.2 */
>>
>>  struct kvm_vcpu_init {
>>       __u32 target;
>> @@ -164,7 +165,7 @@ struct kvm_arch_memory_slot {
>>  /* Highest supported SPI, from VGIC_NR_IRQS */
>>  #define KVM_ARM_IRQ_GIC_MAX          127
>>
>> -/* PSCI interface */
>> +/* PSCI v0.1 interface */
>>  #define KVM_PSCI_FN_BASE             0x95c1ba5e
>>  #define KVM_PSCI_FN(n)                       (KVM_PSCI_FN_BASE + (n))
>>
>> @@ -173,9 +174,41 @@ struct kvm_arch_memory_slot {
>>  #define KVM_PSCI_FN_CPU_ON           KVM_PSCI_FN(2)
>>  #define KVM_PSCI_FN_MIGRATE          KVM_PSCI_FN(3)
>>
>> +/* PSCI v0.2 interface */
>> +#define KVM_PSCI_0_2_FN_BASE         0x84000000
>> +#define KVM_PSCI_0_2_FN(n)           (KVM_PSCI_0_2_FN_BASE + (n))
>> +#define KVM_PSCI_0_2_FN64_BASE               0xC4000000
>> +#define KVM_PSCI_0_2_FN64(n)         (KVM_PSCI_0_2_FN64_BASE + (n))
>> +
>> +#define KVM_PSCI_0_2_FN_PSCI_VERSION KVM_PSCI_0_2_FN(0)
>> +#define KVM_PSCI_0_2_FN_CPU_SUSPEND  KVM_PSCI_0_2_FN(1)
>> +#define KVM_PSCI_0_2_FN_CPU_OFF              KVM_PSCI_0_2_FN(2)
>> +#define KVM_PSCI_0_2_FN_CPU_ON               KVM_PSCI_0_2_FN(3)
>> +#define KVM_PSCI_0_2_FN_AFFINITY_INFO        KVM_PSCI_0_2_FN(4)
>> +#define KVM_PSCI_0_2_FN_MIGRATE              KVM_PSCI_0_2_FN(5)
>> +#define KVM_PSCI_0_2_FN_MIGRATE_INFO_TYPE \
>> +                                     KVM_PSCI_0_2_FN(6)
>> +#define KVM_PSCI_0_2_FN_MIGRATE_INFO_UP_CPU \
>> +                                     KVM_PSCI_0_2_FN(7)
>> +#define KVM_PSCI_0_2_FN_SYSTEM_OFF   KVM_PSCI_0_2_FN(8)
>> +#define KVM_PSCI_0_2_FN_SYSTEM_RESET KVM_PSCI_0_2_FN(9)
>> +
>> +#define KVM_PSCI_0_2_FN64_CPU_SUSPEND        KVM_PSCI_0_2_FN64(1)
>> +#define KVM_PSCI_0_2_FN64_CPU_ON     KVM_PSCI_0_2_FN64(3)
>> +#define KVM_PSCI_0_2_FN64_AFFINITY_INFO      KVM_PSCI_0_2_FN64(4)
>> +#define KVM_PSCI_0_2_FN64_MIGRATE    KVM_PSCI_0_2_FN64(5)
>> +#define KVM_PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU \
>> +                                     KVM_PSCI_0_2_FN64(7)
>> +
>> +/* PSCI return values */
>>  #define KVM_PSCI_RET_SUCCESS         0
>> -#define KVM_PSCI_RET_NI                      ((unsigned long)-1)
>> -#define KVM_PSCI_RET_INVAL           ((unsigned long)-2)
>> +#define KVM_PSCI_RET_NOT_SUPPORTED   ((unsigned long)-1)
>> +#define KVM_PSCI_RET_INVALID_PARAMS  ((unsigned long)-2)
>>  #define KVM_PSCI_RET_DENIED          ((unsigned long)-3)
>> +#define KVM_PSCI_RET_ALREADY_ON              ((unsigned long)-4)
>> +#define KVM_PSCI_RET_ON_PENDING              ((unsigned long)-5)
>> +#define KVM_PSCI_RET_INTERNAL_FAILURE        ((unsigned long)-6)
>> +#define KVM_PSCI_RET_NOT_PRESENT     ((unsigned long)-7)
>> +#define KVM_PSCI_RET_DISABLED                ((unsigned long)-8)
>>
>>  #endif /* __ARM_KVM_H__ */
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 2a700e0..0b7817a 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -193,6 +193,7 @@ int kvm_dev_ioctl_check_extension(long ext)
>>       case KVM_CAP_DESTROY_MEMORY_REGION_WORKS:
>>       case KVM_CAP_ONE_REG:
>>       case KVM_CAP_ARM_PSCI:
>> +     case KVM_CAP_ARM_PSCI_0_2:
>>               r = 1;
>>               break;
>>       case KVM_CAP_COALESCED_MMIO:
>> @@ -483,7 +484,10 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>        * PSCI code.
>>        */
>>       if (test_and_clear_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features)) {
>> -             *vcpu_reg(vcpu, 0) = KVM_PSCI_FN_CPU_OFF;
>> +             if (test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features))
>> +                     *vcpu_reg(vcpu, 0) = KVM_PSCI_0_2_FN_CPU_OFF;
>> +             else
>> +                     *vcpu_reg(vcpu, 0) = KVM_PSCI_FN_CPU_OFF;
>>               kvm_psci_call(vcpu);
>
> 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
>
>>       }
>>
>> 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?
>
> 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.

I gave more thought to this.

Currently, there is no conflict of function IDs between PSCI v0.1 and v0.2
but what if in future some new PSCI vX.Y comes-up with function IDs which
conflict with existing PSCI vA.B.

I think it is very likely that we will have more versions of PSCI and each
subsequent of version of PSCI will generally extend functions IDs defined
by PSCI v0.2

I will keep the KVM_ARM_VCPU_PSCI_0_2 feature for v2 patchset and
wait for more comments on this.

Regards,
Anup

>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 0a1d697..92242ce 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -39,7 +39,7 @@
>>  #include <kvm/arm_vgic.h>
>>  #include <kvm/arm_arch_timer.h>
>>
>> -#define KVM_VCPU_MAX_FEATURES 2
>> +#define KVM_VCPU_MAX_FEATURES 3
>>
>>  struct kvm_vcpu;
>>  int kvm_target_cpu(void);
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index d9f026b..0eb254d 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -77,6 +77,7 @@ struct kvm_regs {
>>
>>  #define KVM_ARM_VCPU_POWER_OFF               0 /* CPU is started in OFF state */
>>  #define KVM_ARM_VCPU_EL1_32BIT               1 /* CPU running a 32bit VM */
>> +#define KVM_ARM_VCPU_PSCI_0_2                2 /* CPU uses PSCI v0.2 */
>>
>>  struct kvm_vcpu_init {
>>       __u32 target;
>> @@ -150,7 +151,7 @@ struct kvm_arch_memory_slot {
>>  /* Highest supported SPI, from VGIC_NR_IRQS */
>>  #define KVM_ARM_IRQ_GIC_MAX          127
>>
>> -/* PSCI interface */
>> +/* PSCI v0.1 interface */
>>  #define KVM_PSCI_FN_BASE             0x95c1ba5e
>>  #define KVM_PSCI_FN(n)                       (KVM_PSCI_FN_BASE + (n))
>>
>> @@ -159,10 +160,42 @@ struct kvm_arch_memory_slot {
>>  #define KVM_PSCI_FN_CPU_ON           KVM_PSCI_FN(2)
>>  #define KVM_PSCI_FN_MIGRATE          KVM_PSCI_FN(3)
>>
>> +/* PSCI v0.2 interface */
>> +#define KVM_PSCI_0_2_FN_BASE         0x84000000
>> +#define KVM_PSCI_0_2_FN(n)           (KVM_PSCI_0_2_FN_BASE + (n))
>> +#define KVM_PSCI_0_2_FN64_BASE               0xC4000000
>> +#define KVM_PSCI_0_2_FN64(n)         (KVM_PSCI_0_2_FN64_BASE + (n))
>> +
>> +#define KVM_PSCI_0_2_FN_PSCI_VERSION KVM_PSCI_0_2_FN(0)
>> +#define KVM_PSCI_0_2_FN_CPU_SUSPEND  KVM_PSCI_0_2_FN(1)
>> +#define KVM_PSCI_0_2_FN_CPU_OFF              KVM_PSCI_0_2_FN(2)
>> +#define KVM_PSCI_0_2_FN_CPU_ON               KVM_PSCI_0_2_FN(3)
>> +#define KVM_PSCI_0_2_FN_AFFINITY_INFO        KVM_PSCI_0_2_FN(4)
>> +#define KVM_PSCI_0_2_FN_MIGRATE              KVM_PSCI_0_2_FN(5)
>> +#define KVM_PSCI_0_2_FN_MIGRATE_INFO_TYPE \
>> +                                     KVM_PSCI_0_2_FN(6)
>> +#define KVM_PSCI_0_2_FN_MIGRATE_INFO_UP_CPU \
>> +                                     KVM_PSCI_0_2_FN(7)
>> +#define KVM_PSCI_0_2_FN_SYSTEM_OFF   KVM_PSCI_0_2_FN(8)
>> +#define KVM_PSCI_0_2_FN_SYSTEM_RESET KVM_PSCI_0_2_FN(9)
>> +
>> +#define KVM_PSCI_0_2_FN64_CPU_SUSPEND        KVM_PSCI_0_2_FN64(1)
>> +#define KVM_PSCI_0_2_FN64_CPU_ON     KVM_PSCI_0_2_FN64(3)
>> +#define KVM_PSCI_0_2_FN64_AFFINITY_INFO      KVM_PSCI_0_2_FN64(4)
>> +#define KVM_PSCI_0_2_FN64_MIGRATE    KVM_PSCI_0_2_FN64(5)
>> +#define KVM_PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU \
>> +                                     KVM_PSCI_0_2_FN64(7)
>> +
>> +/* PSCI return values */
>>  #define KVM_PSCI_RET_SUCCESS         0
>> -#define KVM_PSCI_RET_NI                      ((unsigned long)-1)
>> -#define KVM_PSCI_RET_INVAL           ((unsigned long)-2)
>> +#define KVM_PSCI_RET_NOT_SUPPORTED   ((unsigned long)-1)
>> +#define KVM_PSCI_RET_INVALID_PARAMS  ((unsigned long)-2)
>>  #define KVM_PSCI_RET_DENIED          ((unsigned long)-3)
>> +#define KVM_PSCI_RET_ALREADY_ON              ((unsigned long)-4)
>> +#define KVM_PSCI_RET_ON_PENDING              ((unsigned long)-5)
>> +#define KVM_PSCI_RET_INTERNAL_FAILURE        ((unsigned long)-6)
>> +#define KVM_PSCI_RET_NOT_PRESENT     ((unsigned long)-7)
>> +#define KVM_PSCI_RET_DISABLED                ((unsigned long)-8)
>>
>>  #endif
>>
>> --
>> 1.7.9.5
>>
>
> Thanks,
> --
> Christoffer
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [CentOS ARM]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]     [Photos]