On Fri, Apr 13, 2018 at 10:20:52AM +0200, Eric Auger wrote:
> We introduce a new helper that creates and inserts a new redistributor
> region into the rdist region list. This helper both handles the case
> where the redistributor region size is known at registration time
> and the legacy case where it is not (eventually depending on the number
> of online vcpus). Depending on pfns, we perform all the possible checks
> that we can do:
>
> - end of memory crossing
> - incorrect alignment of the base address
> - collision with distributor region if already defined
> - collision with already registered rdist regions
> - check of the new index
>
> Rdist regions must be inserted by increasing order of indices. Indices
> must be contiguous.
>
> We also introduce vgic_v3_rdist_region_from_index() which will be used
> from the vgic kvm-device, later on.
>
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 95 +++++++++++++++++++++++++++++++++-------
> virt/kvm/arm/vgic/vgic-v3.c | 29 ++++++++++++
> virt/kvm/arm/vgic/vgic.h | 14 ++++++
> 3 files changed, 122 insertions(+), 16 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index ce5c927..5273fb8 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -680,14 +680,66 @@ static int vgic_register_all_redist_iodevs(struct kvm *kvm)
> return ret;
> }
>
> -int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
> +/**
> + * vgic_v3_insert_redist_region - Insert a new redistributor region
> + *
> + * Performs various checks before inserting the rdist region in the list.
> + * Those tests depend on whether the size of the rdist region is known
> + * (ie. count != 0). The list is sorted by rdist region index.
> + *
> + * @kvm: kvm handle
> + * @index: redist region index
> + * @base: base of the new rdist region
> + * @count: number of redistributors the region is made of (of 0 in the old style
> + * single region, whose size is induced from the number of vcpus)
> + *
> + * Return 0 on success, < 0 otherwise
> + */
> +static int vgic_v3_insert_redist_region(struct kvm *kvm, uint32_t index,
> + gpa_t base, uint32_t count)
> {
> - struct vgic_dist *vgic = &kvm->arch.vgic;
> + struct vgic_dist *d = &kvm->arch.vgic;
> struct vgic_redist_region *rdreg;
> + struct list_head *rd_regions = &d->rd_regions;
> + struct list_head *last = rd_regions->prev;
> +
nit: extra blank line?
> + gpa_t new_start, new_end;
> + size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
> int ret;
>
> - /* vgic_check_ioaddr makes sure we don't do this twice */
> - if (!list_empty(&vgic->rd_regions))
> + /* single rdist region already set ?*/
> + if (!count && !list_empty(rd_regions))
> + return -EINVAL;
> +
> + /* cross the end of memory ? */
> + if (base + size < base)
> + return -EINVAL;
what is the size of memory? This seems to check for a gpa_t overflow,
but not againt the IPA space of the VM...
> +
> + if (list_empty(rd_regions)) {
> + if (index != 0)
> + return -EINVAL;
note, I think this can be simplified if we can rid of the index.
> + } else {
> + rdreg = list_entry(last, struct vgic_redist_region, list);
you can use list_last_entry here and get rid of the 'last' temporary
variable above.
> + if (index != rdreg->index + 1)
> + return -EINVAL;
> +
> + /* Cannot add an explicitly sized regions after legacy region */
> + if (!rdreg->count)
> + return -EINVAL;
> + }
> +
> + /*
> + * collision with already set dist region ?
> + * this assumes we know the size of the new rdist region (pfns != 0)
> + * otherwise we can only test this when all vcpus are registered
> + */
I don't really understand this commentary... :(
> + if (!count && !IS_VGIC_ADDR_UNDEF(d->vgic_dist_base) &&
> + (!(d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE <= base)) &&
> + (!(base + size <= d->vgic_dist_base)))
> + return -EINVAL;
Can't you call vgic_v3_check_base() here instead?
> +
> + /* collision with any other rdist region? */
> + if (vgic_v3_rdist_overlap(kvm, base, size))
> return -EINVAL;
>
> rdreg = kzalloc(sizeof(*rdreg), GFP_KERNEL);
> @@ -696,17 +748,32 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
>
> rdreg->base = VGIC_ADDR_UNDEF;
>
> - ret = vgic_check_ioaddr(kvm, &rdreg->base, addr, SZ_64K);
> + ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K);
> if (ret)
> - goto out;
> + goto free;
>
> - rdreg->base = addr;
> - if (!vgic_v3_check_base(kvm)) {
> - ret = -EINVAL;
> - goto out;
> - }
> + rdreg->base = base;
> + rdreg->count = count;
> + rdreg->free_index = 0;
> + rdreg->index = index;
>
> - list_add(&rdreg->list, &vgic->rd_regions);
> + new_start = base;
> + new_end = base + size - 1;
What are these variables used for?
> +
> + list_add_tail(&rdreg->list, rd_regions);
> + return 0;
> +free:
> + kfree(rdreg);
> + return ret;
> +}
> +
> +int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
> +{
> + int ret;
> +
> + ret = vgic_v3_insert_redist_region(kvm, 0, addr, 0);
> + if (ret)
> + return ret;
>
> /*
> * Register iodevs for each existing VCPU. Adding more VCPUs
> @@ -717,10 +784,6 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u64 addr)
> return ret;
>
> return 0;
> -
> -out:
> - kfree(rdreg);
> - return ret;
> }
>
> int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 820012a..dbcba5f 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -410,6 +410,21 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
> return 0;
> }
>
> +/* return true if there is an overlap between any rdist */
Checks if base+size overlaps with any existing redistributor.
> +bool vgic_v3_rdist_overlap(struct kvm *kvm, gpa_t base, size_t size)
> +{
> + struct vgic_dist *d = &kvm->arch.vgic;
> + struct vgic_redist_region *rdreg;
> +
> + list_for_each_entry(rdreg, &d->rd_regions, list) {
> + if ((base + size <= rdreg->base) ||
> + (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <= base))
> + continue;
> + return true;
can you invert the check above and return false instead of the continue?
(DeMorgan's law should be handy here.)
> + }
> + return false;
> +}
> +
> /*
> * Check for overlapping regions and for regions crossing the end of memory
> * for base addresses which have already been set.
> @@ -461,6 +476,20 @@ struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rd_regions)
> return NULL;
> }
>
> +struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
> + uint32_t index)
> +{
> + struct list_head *rd_regions = &kvm->arch.vgic.rd_regions;
> + struct vgic_redist_region *rdreg;
> +
> + list_for_each_entry(rdreg, rd_regions, list) {
> + if (rdreg->index == index)
> + return rdreg;
> + }
if this ends up being a common operation, we could allocate an array of
pointers for constant-time lookup instead. Let's hope it's not too
common.
> + return NULL;
> +}
> +
> +
> int vgic_v3_map_resources(struct kvm *kvm)
> {
> int ret = 0;
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index fea32cb..95b8345 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -262,6 +262,20 @@ vgic_v3_redist_region_full(struct vgic_redist_region *region)
>
> struct vgic_redist_region *vgic_v3_rdist_free_slot(struct list_head *rdregs);
>
> +static inline size_t
> +vgic_v3_rd_region_size(struct kvm *kvm, struct vgic_redist_region *rdreg)
> +{
> + if (!rdreg->count)
> + return atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE;
> + else
> + return rdreg->count * KVM_VGIC_V3_REDIST_SIZE;
> +}
> +
> +struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm,
> + uint32_t index);
> +
> +bool vgic_v3_rdist_overlap(struct kvm *kvm, gpa_t base, size_t size);
> +
> int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
> u32 devid, u32 eventid, struct vgic_irq **irq);
> struct vgic_its *vgic_msi_to_its(struct kvm *kvm, struct kvm_msi *msi);
> --
> 2.5.5
>
Thanks,
-Christoffer
