On Tue, May 10, 2016 at 06:18:46PM +0100, Andre Przywara wrote:
> When userland sets the base addresses for the GIC register frames,
> the kernel tries to make sure that the regions for the distributor and
> the one for the CPU interface or the redistributors do not overlap.
> Currently this check only works properly for GICv2.
> For a GICv3 we need the number of VCPUs to compute the size of the
> redistributor region, which we only know for sure at init time.
> So move the overlap check from kvm_vgic_addr() to the model specific
> map_resources() implementation.
> This also allows us to simplify the existing checking code a bit.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> Hi,
>
> so another rework, now hopefully really covering GICv3 properly.
> For consistency I also moved the GICv2 check to init time. This is
> a slightly different behaviour, but shouldn't make a difference, as
> the init call can fail as well - for instance if no addresses have been
> set up at all.
>
> Cheers,
> Andre.
>
> virt/kvm/arm/vgic/vgic-kvm-device.c | 50 ++++++++-----------------------------
> virt/kvm/arm/vgic/vgic-v2.c | 22 ++++++++++++++++
> virt/kvm/arm/vgic/vgic-v3.c | 27 ++++++++++++++++++++
> 3 files changed, 60 insertions(+), 39 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 2122ff2..9e736a7 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -19,43 +19,19 @@
> #include <asm/kvm_mmu.h>
> #include "vgic.h"
>
> -/* common helpers */
> -
> -static int vgic_ioaddr_overlap(struct kvm *kvm)
> -{
> - phys_addr_t dist = kvm->arch.vgic.vgic_dist_base;
> - phys_addr_t cpu = kvm->arch.vgic.vgic_cpu_base;
> -
> - if (IS_VGIC_ADDR_UNDEF(dist) || IS_VGIC_ADDR_UNDEF(cpu))
> - return 0;
> - if ((dist <= cpu && dist + KVM_VGIC_V2_DIST_SIZE > cpu) ||
> - (cpu <= dist && cpu + KVM_VGIC_V2_CPU_SIZE > dist))
> - return -EBUSY;
> - return 0;
> -}
> -
> -static int vgic_ioaddr_assign(struct kvm *kvm, phys_addr_t *ioaddr,
> - phys_addr_t addr, phys_addr_t size)
> +static int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr,
> + phys_addr_t addr, phys_addr_t alignment)
> {
> - int ret;
> -
> if (addr & ~KVM_PHYS_MASK)
> return -E2BIG;
>
> - if (addr & (SZ_4K - 1))
> + if (!IS_ALIGNED(addr, alignment))
> return -EINVAL;
>
> if (!IS_VGIC_ADDR_UNDEF(*ioaddr))
> return -EEXIST;
> - if (addr + size < addr)
> - return -EINVAL;
> -
> - *ioaddr = addr;
> - ret = vgic_ioaddr_overlap(kvm);
> - if (ret)
> - *ioaddr = VGIC_ADDR_UNDEF;
>
> - return ret;
> + return 0;
> }
>
> /**
> @@ -70,40 +46,38 @@ static int vgic_ioaddr_assign(struct kvm *kvm, phys_addr_t *ioaddr,
> * interface in the VM physical address space. These addresses are properties
> * of the emulated core/SoC and therefore user space initially knows this
> * information.
> + * Check them for sanity (alignment, double assignment). We can't check for
> + * overlapping regions in case of a virtual GICv3 here, since we don't know
> + * the number of VCPUs yet, so we defer this check to map_resources().
> */
> 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;
> - phys_addr_t alignment;
> + phys_addr_t *addr_ptr, alignment;
>
> mutex_lock(&kvm->lock);
> switch (type) {
> case KVM_VGIC_V2_ADDR_TYPE_DIST:
> type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
> addr_ptr = &vgic->vgic_dist_base;
> - block_size = KVM_VGIC_V2_DIST_SIZE;
> alignment = SZ_4K;
> break;
> case KVM_VGIC_V2_ADDR_TYPE_CPU:
> type_needed = KVM_DEV_TYPE_ARM_VGIC_V2;
> addr_ptr = &vgic->vgic_cpu_base;
> - block_size = KVM_VGIC_V2_CPU_SIZE;
> alignment = SZ_4K;
> break;
> #ifdef CONFIG_KVM_ARM_VGIC_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;
> alignment = SZ_64K;
> 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;
> alignment = SZ_64K;
> break;
> #endif
> @@ -118,11 +92,9 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
> }
>
> if (write) {
> - if (!IS_ALIGNED(*addr, alignment))
> - r = -EINVAL;
> - else
> - r = vgic_ioaddr_assign(kvm, addr_ptr,
> - *addr, block_size);
> + r = vgic_check_ioaddr(kvm, addr_ptr, *addr, alignment);
> + if (!r)
> + *addr_ptr = *addr;
> } else {
> *addr = *addr_ptr;
> }
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 4493593..fe6e3bd 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -225,6 +225,22 @@ void vgic_v2_enable(struct kvm_vcpu *vcpu)
> vcpu->arch.vgic_cpu.vgic_v2.vgic_hcr = GICH_HCR_EN;
> }
>
> +/* check for overlapping regions and for regions crossing the end of memory */
> +static bool vgic_check_addresses(gpa_t dist_base, gpa_t cpu_base)
> +{
> + if (dist_base + KVM_VGIC_V2_DIST_SIZE < dist_base)
> + return false;
> + if (cpu_base + KVM_VGIC_V2_CPU_SIZE < cpu_base)
> + return false;
> +
> + if (dist_base + KVM_VGIC_V2_DIST_SIZE <= cpu_base)
> + return true;
> + if (cpu_base + KVM_VGIC_V2_CPU_SIZE <= dist_base)
> + return true;
> +
> + return false;
> +}
> +
> int vgic_v2_map_resources(struct kvm *kvm)
> {
> struct vgic_dist *dist = &kvm->arch.vgic;
> @@ -240,6 +256,12 @@ int vgic_v2_map_resources(struct kvm *kvm)
> goto out;
> }
>
> + if (!vgic_check_addresses(dist->vgic_dist_base, dist->vgic_cpu_base)) {
> + kvm_err("VGIC CPU and dist frames overlap\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> /*
> * Initialize the vgic if this hasn't already been done on demand by
> * accessing the vgic state from userspace.
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 6d7422f..fa444a7 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -221,6 +221,27 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
> vgic_v3->vgic_hcr = ICH_HCR_EN;
> }
>
> +/* check for overlapping regions and for regions crossing the end of memory */
> +static bool vgic_check_addresses(struct kvm *kvm)
> +{
> + struct vgic_dist *d = &kvm->arch.vgic;
> + gpa_t redist_size = KVM_VGIC_V3_REDIST_SIZE;
> +
> + redist_size = KVM_VGIC_V3_REDIST_SIZE * atomic_read(&kvm->online_vcpus);
this looks horrible IMHO, you can fit the full calculations on exactly
80 chars, which must be a sign of what was supposed to happen ;)
> +
> + if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE < d->vgic_dist_base)
> + return false;
> + if (d->vgic_redist_base + redist_size < d->vgic_redist_base)
> + return false;
> +
> + if (d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= d->vgic_redist_base)
> + return true;
> + if (d->vgic_redist_base + redist_size <= d->vgic_dist_base)
> + return true;
> +
> + return false;
> +}
> +
> int vgic_v3_map_resources(struct kvm *kvm)
> {
> int ret = 0;
> @@ -236,6 +257,12 @@ int vgic_v3_map_resources(struct kvm *kvm)
> goto out;
> }
>
> + if (!vgic_check_addresses(kvm)) {
> + kvm_err("VGIC redist and dist frames overlap\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> /*
> * For a VGICv3 we require the userland to explicitly initialize
> * the VGIC before we need to use it.
> --
> 2.7.3
>
I would have named the check_addresses functions with the specific vgic
version, but anyhow:
Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
--
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
