Hi Eric,
thanks for the review!
On 06/09/2015 09:52 AM, Eric Auger wrote:
> On 05/29/2015 11:53 AM, Andre Przywara wrote:
>> The ARM GICv3 ITS controller requires a separate register frame to
>> cover ITS specific registers. Add a new VGIC address type and store
>> the address in a field in the vgic_dist structure.
>> Provide a function to check whether userland has provided the address,
>> so ITS functionality can be guarded by that check.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> Documentation/virtual/kvm/devices/arm-vgic.txt | 7 +++++++
>> arch/arm64/include/uapi/asm/kvm.h | 3 +++
>> include/kvm/arm_vgic.h | 3 +++
>> virt/kvm/arm/vgic-v3-emul.c | 1 +
>> virt/kvm/arm/vgic.c | 17 +++++++++++++++++
>> virt/kvm/arm/vgic.h | 1 +
>> 6 files changed, 32 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
>> index 3fb9054..1f89001 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
>> @@ -39,6 +39,13 @@ Groups:
>> Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
>> This address needs to be 64K aligned.
>>
>> + KVM_VGIC_V3_ADDR_TYPE_ITS (rw, 64-bit)
>> + Base address in the guest physical address space of the GICv3 ITS
>> + register frame. The ITS allows MSI(-X) interrupts to be injected
>> + into guests. This extension is optional, if the kernel does not
>> + support the ITS, the call returns -ENODEV.
>> + Only valid for KVM_DEV_TYPE_ARM_VGIC_V3.
>> + This address needs to be 64K aligned and the region covers 64 KByte.
> I would emphasize this is the control registers (ITS_Base) hence a
> single page, not comprising the ITS translation register page.
Good point, will do.
>>
>> KVM_DEV_ARM_VGIC_GRP_DIST_REGS
>> Attributes:
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index d268320..e42435c 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -82,8 +82,11 @@ struct kvm_regs {
>> #define KVM_VGIC_V3_ADDR_TYPE_DIST 2
>> #define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
>>
> extra white line?
>> +#define KVM_VGIC_V3_ADDR_TYPE_ITS 4
>> +
>> #define KVM_VGIC_V3_DIST_SIZE SZ_64K
>> #define KVM_VGIC_V3_REDIST_SIZE (2 * SZ_64K)
>> +#define KVM_VGIC_V3_ITS_SIZE 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 */
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index b18e2c5..37725bb 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -178,6 +178,9 @@ struct vgic_dist {
>> phys_addr_t vgic_redist_base;
>> };
>>
>> + /* The base address for the MSI control block (V2M/ITS) */
> why V2M here? It's it the GITS_TRANSLATER page that has a fellow page in
> case of V2M?
Ah yes, this was a leftover from a previous version of the series. I had
V2M in-kernel emulation implemented at some time, but later dropped that.
Thanks for spotting.
>> + phys_addr_t vgic_its_base;
>> +
>> /* Distributor enabled */
>> u32 enabled;
>>
>> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
>> index fbfdd6f..16c6d8a 100644
>> --- a/virt/kvm/arm/vgic-v3-emul.c
>> +++ b/virt/kvm/arm/vgic-v3-emul.c
>> @@ -1012,6 +1012,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>> return -ENXIO;
>> case KVM_VGIC_V3_ADDR_TYPE_DIST:
>> case KVM_VGIC_V3_ADDR_TYPE_REDIST:
>> + case KVM_VGIC_V3_ADDR_TYPE_ITS:
>> return 0;
>> }
>> break;
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index 6ea30e0..2e9723aa 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -932,6 +932,16 @@ int vgic_register_kvm_io_dev(struct kvm *kvm, gpa_t base, int len,
>> return ret;
>> }
>>
>> +bool vgic_has_its(struct kvm *kvm)
>> +{
>> + struct vgic_dist *dist = &kvm->arch.vgic;
>> +
>> + if (dist->vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
>> + return false;
>> +
>> + return !IS_VGIC_ADDR_UNDEF(dist->vgic_its_base);
>> +}
>> +
>> static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>> {
>> return dist->nr_irqs - VGIC_NR_PRIVATE_IRQS;
>> @@ -1835,6 +1845,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>> kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>> kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>> kvm->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
>> + kvm->arch.vgic.vgic_its_base = VGIC_ADDR_UNDEF;
> minor: given the fact we have specialized
> init_vgic_model/vgic_vx_init_emulation wouldn't it make sense to move
> those VGIC v3 specific assignment there? also CPU base is specific to v2?
Yes, that seems to make some sense. The original idea was to make sure
that all of those addresses are properly initialized regardless of the
chosen guest model, but I see that this gets messy and would probably be
better solved by moving those assignments.
Thanks,
Andre.
>>
>> out_unlock:
>> for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
>> @@ -1932,6 +1943,12 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>> block_size = KVM_VGIC_V3_REDIST_SIZE;
>> alignment = SZ_64K;
>> break;
>> + case KVM_VGIC_V3_ADDR_TYPE_ITS:
>> + type_needed = KVM_DEV_TYPE_ARM_VGIC_V3;
>> + addr_ptr = &vgic->vgic_its_base;
>> + block_size = KVM_VGIC_V3_ITS_SIZE;
>> + alignment = SZ_64K;
>> + break;
>> #endif
>> default:
>> r = -ENODEV;
>> diff --git a/virt/kvm/arm/vgic.h b/virt/kvm/arm/vgic.h
>> index 0df74cb..a093f5c 100644
>> --- a/virt/kvm/arm/vgic.h
>> +++ b/virt/kvm/arm/vgic.h
>> @@ -136,5 +136,6 @@ int vgic_get_common_attr(struct kvm_device *dev, struct kvm_device_attr *attr);
>> int vgic_init(struct kvm *kvm);
>> void vgic_v2_init_emulation(struct kvm *kvm);
>> void vgic_v3_init_emulation(struct kvm *kvm);
>> +bool vgic_has_its(struct kvm *kvm);
>>
>> #endif
>>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
