Re: [PATCH 04/13] KVM: arm64: Introduce new MMIO region for the ITS base address
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 >> --- >> 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_REDIST3 >> > 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_SIZESZ_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_mod
Re: [PATCH 04/13] KVM: arm64: Introduce new MMIO region for the ITS base address
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 > --- > 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. > >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_ITS4 > + > #define KVM_VGIC_V3_DIST_SIZESZ_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? > + 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? Eric > > 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; >
[PATCH 04/13] KVM: arm64: Introduce new MMIO region for the ITS base address
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 --- 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. 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 +#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) */ + 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; 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_e