lkml.org 
[lkml]   [2018]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 06/12] KVM: arm/arm64: Helper to register a new redistributor region
    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@redhat.com>
    > ---
    > 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

    \
     
     \ /
      Last update: 2018-04-24 18:48    [W:4.082 / U:0.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site