Hej Christoffer,
On 07/11/14 16:15, Christoffer Dall wrote:
> On Fri, Oct 31, 2014 at 05:26:54PM +0000, Andre Przywara wrote:
>> With everything in place we allow userland to request the kernel
>> using a virtual GICv3 in the guest, which finally lifts the 8 vCPU
>> limit for a guest.
>
> You're actually not explicitly allowing this in this patch, you're
> implicitly allowing it because init would fail without the vgic
> distributor base address being set already.
>
> Either re-arrange your patches or fix the commit message.
The latter ;-)
>> Also we provide the necessary support for guests setting the memory
>> addresses for the virtual distributor and redistributors.
>> This requires some userland code to make use of that feature and
>> explicitly ask for a virtual GICv3.
>
> You need to add documentation for this new device type and the userspace
> ABI.
Will do.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> arch/arm64/include/uapi/asm/kvm.h | 7 ++++++
>> include/kvm/arm_vgic.h | 4 ++--
>> virt/kvm/arm/vgic-v3-emul.c | 3 +++
>> virt/kvm/arm/vgic.c | 46 ++++++++++++++++++++++++++-----------
>> 4 files changed, 45 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index 8e38878..2ed873a 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -78,6 +78,13 @@ struct kvm_regs {
>> #define KVM_VGIC_V2_DIST_SIZE 0x1000
>> #define KVM_VGIC_V2_CPU_SIZE 0x2000
>>
>> +/* Supported VGICv3 address types */
>> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
>> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
>> +
>> +#define KVM_VGIC_V3_DIST_SIZE SZ_64K
>> +#define KVM_VGIC_V3_REDIST_SIZE (2 * SZ_64K)
>> +
>> #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 */
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index c303083..e2e432c 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -35,8 +35,8 @@
>> #define VGIC_MAX_IRQS 1024
>>
>> /* Sanity checks... */
>> -#if (KVM_MAX_VCPUS > 8)
>> -#error Invalid number of CPU interfaces
>> +#if (KVM_MAX_VCPUS > 255)
>> +#error Too many KVM VCPUs, the VGIC only supports up to 255 VCPUs for now
>
> what happens now if you add more vcpus after having created a GICv2 with
> 8 vcpus?
On adding a VCPU we check the number of allowed VCPUs for this
particular guest (see arch/arm/kvm/arm.c:kvm_arch_vcpu_create() in patch
09/19). On creating a virtual GICv2 we set the limit to 8, so any
KVM_VCPU_CREATE afterwards will fail.
But indeed I found other issues in this sequence of VCPU/VGIC init,
which dissolved "magically" by the rework around (or actually the drop
of) init_emul() and friends.
Thanks,
Andre.
>> #endif
>>
>> #if (VGIC_NR_IRQS_LEGACY & 31)
>> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
>> index bcb5374..ba6b0b5 100644
>> --- a/virt/kvm/arm/vgic-v3-emul.c
>> +++ b/virt/kvm/arm/vgic-v3-emul.c
>> @@ -870,6 +870,9 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>> case KVM_VGIC_V2_ADDR_TYPE_DIST:
>> case KVM_VGIC_V2_ADDR_TYPE_CPU:
>> return -ENXIO;
>> + case KVM_VGIC_V3_ADDR_TYPE_DIST:
>> + case KVM_VGIC_V3_ADDR_TYPE_REDIST:
>> + return 0;
>> }
>> break;
>> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 16d7c9d..a5abef1 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1647,7 +1647,7 @@ static int vgic_ioaddr_assign(struct kvm *kvm, phys_addr_t *ioaddr,
>> /**
>> * kvm_vgic_addr - set or get vgic VM base addresses
>> * @kvm: pointer to the vm struct
>> - * @type: the VGIC addr type, one of KVM_VGIC_V2_ADDR_TYPE_XXX
>> + * @type: the VGIC addr type, one of KVM_VGIC_V[23]_ADDR_TYPE_XXX
>> * @addr: pointer to address value
>> * @write: if true set the address in the VM address space, if false read the
>> * address
>> @@ -1661,29 +1661,49 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>> {
>> int r = 0;
>> struct vgic_dist *vgic = &kvm->arch.vgic;
>> + int type_needed;
>> + phys_addr_t *addr_ptr, block_size;
>>
>> mutex_lock(&kvm->lock);
>> switch (type) {
>> case KVM_VGIC_V2_ADDR_TYPE_DIST:
>> - if (write) {
>> - r = vgic_ioaddr_assign(kvm, &vgic->vgic_dist_base,
>> - *addr, KVM_VGIC_V2_DIST_SIZE);
>> - } else {
>> - *addr = vgic->vgic_dist_base;
>> - }
>> + type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
>> + addr_ptr = &vgic->vgic_dist_base;
>> + block_size = KVM_VGIC_V2_DIST_SIZE;
>> break;
>> case KVM_VGIC_V2_ADDR_TYPE_CPU:
>> - if (write) {
>> - r = vgic_ioaddr_assign(kvm, &vgic->vgic_cpu_base,
>> - *addr, KVM_VGIC_V2_CPU_SIZE);
>> - } else {
>> - *addr = vgic->vgic_cpu_base;
>> - }
>> + type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
>> + addr_ptr = &vgic->vgic_cpu_base;
>> + block_size = KVM_VGIC_V2_CPU_SIZE;
>> break;
>> +#ifdef CONFIG_ARM_GIC_V3
>> + case KVM_VGIC_V3_ADDR_TYPE_DIST:
>> + type_needed = KVM_DEV_TYPE_ARM_VGIC_V3;
>> + addr_ptr = &vgic->vgic_dist_base;
>> + block_size = KVM_VGIC_V3_DIST_SIZE;
>> + break;
>> + case KVM_VGIC_V3_ADDR_TYPE_REDIST:
>> + type_needed = KVM_DEV_TYPE_ARM_VGIC_V3;
>> + addr_ptr = &vgic->vgic_redist_base;
>> + block_size = KVM_VGIC_V3_REDIST_SIZE;
>> + break;
>> +#endif
>> default:
>> r = -ENODEV;
>> + goto out;
>> + }
>> +
>> + if (vgic->vgic_model != type_needed) {
>> + r = -ENODEV;
>> + goto out;
>> }
>>
>> + if (write)
>> + r = vgic_ioaddr_assign(kvm, addr_ptr, *addr, block_size);
>> + else
>> + *addr = *addr_ptr;
>> +
>> +out:
>> mutex_unlock(&kvm->lock);
>> return r;
>> }
>> --
>> 1.7.9.5
>>
>
> Otherwise looks good to me.
>
> Thanks,
> -Christoffer
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
