Received: by 10.192.165.148 with SMTP id m20csp1840078imm; Thu, 26 Apr 2018 03:05:50 -0700 (PDT) X-Google-Smtp-Source: AIpwx495w3/oHRUz4wWrbovGQgfMBP7GgOJsn+Rql/r/vkm+h9P+Hsvjy/WBP4Kf9VXyFgQfY3sg X-Received: by 2002:a17:902:6c0b:: with SMTP id q11-v6mr21960872plk.135.1524737150383; Thu, 26 Apr 2018 03:05:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524737150; cv=none; d=google.com; s=arc-20160816; b=OdWyVOk70HX5viBN1jUQ/Aisa4750JycOvuDeD2lRCgMTxm+0pMNyC+aEdhQ4ovdi9 cpjJ8iCWAQm7MHalr9kxUAHsmXEp8ZyVjT/3B/uOzp+oOJQDKQwdIYx7sXhpOjALh7Hh LTIdhI+QyupyPvPSk5VOmdRwo5KfzWZYOnsHmFXmw12yIiTT9j0UQncHZyiG/xwEWVFp rNRN660Rbs7fdqrivk1hJGSNbwuBtMemu0OZo6lUzawoV7S09ExdaM3rUvsz8noLijLY R2MPgXemo9mHDkKbd42kIgo3RwMM/Gzzd/BmYG9n2l1/hQOsASKKTGlRQYguXOaUS/z9 a0Dw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=cEaTzqq3cx/Hym+a/8hPamMpLcMjzdaIcSYwTYD4jGU=; b=hU6fiUEQyUiIO35cTpFle8pPbizxdXi2LhxeE6UJ3QszhnPysdS2p8cIsbJg6+uJ6f W7b3D1iw1kOp2C4S0yEgDKs5n6OwpA1XgXV9FPVQgANmUKHUz0HuIujxfLM8R+AqSjlz gFdTs/7+kzf/H99YW2rysvqT+FYZktx27zj3MiQ7ijvGtwHUYKwajV992Ctm+4kvRJnH x7wUQBezUsrfiGyGtKKjYmorW9ZY4PvHXmwNXydWep6J/Te0QCY6aDYAuOA6uPLXcNsK c5McUQLTD20A2YC8hO2lCxfslNgJV5FummrpUYivAAY3T77tSwszBB2UEG19kNYvIZhM oJCw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id e33-v6si18780200pld.404.2018.04.26.03.05.36; Thu, 26 Apr 2018 03:05:50 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754484AbeDZKEb (ORCPT + 99 others); Thu, 26 Apr 2018 06:04:31 -0400 Received: from foss.arm.com ([217.140.101.70]:50374 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753677AbeDZKE3 (ORCPT ); Thu, 26 Apr 2018 06:04:29 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id EE84E15AD; Thu, 26 Apr 2018 03:04:28 -0700 (PDT) Received: from localhost (unknown [10.37.12.162]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2C7613F487; Thu, 26 Apr 2018 03:04:27 -0700 (PDT) Date: Thu, 26 Apr 2018 12:04:25 +0200 From: Christoffer Dall To: Auger Eric Cc: eric.auger.pro@gmail.com, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, marc.zyngier@arm.com, cdall@kernel.org, peter.maydell@linaro.org, andre.przywara@arm.com, drjones@redhat.com, wei@redhat.com Subject: Re: [PATCH v3 06/12] KVM: arm/arm64: Helper to register a new redistributor region Message-ID: <20180426100425.GA19872@C02W217FHV2R.local> References: <1523607658-9166-1-git-send-email-eric.auger@redhat.com> <1523607658-9166-7-git-send-email-eric.auger@redhat.com> <20180424164712.GB4533@C02W217FHV2R.local> <8ad17ccb-9880-ca67-8ed8-25fb9557942c@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8ad17ccb-9880-ca67-8ed8-25fb9557942c@redhat.com> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 26, 2018 at 09:32:49AM +0200, Auger Eric wrote: > Hi Christoffer, > > On 04/24/2018 06:47 PM, Christoffer Dall wrote: > > 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 > >> --- > >> | 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... > Yes it checks for a gpa_t overflow. This check is currently done in > vgic_v3_check_base() for dist and redist region and I replicated it. fair enough, the comment is a bit misleading though. We could also consider checking against KVM_PHYS_SHIFT. > > > >> + > >> + if (list_empty(rd_regions)) { > >> + if (index != 0) > >> + return -EINVAL; > > > > note, I think this can be simplified if we can rid of the index. > So I eventually keep the index. Yes. > > > >> + } 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. > definitively, thank you for the nit. > > > >> + 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... :( > I meant we cannot perform the check below if we are inserting a unique > legacy rdist region (old API), whose size is not explicitly set but > induced from the number of online vcpus. > ok, given the complexity of the logic below, I think you should just explain it: /* * For legacy single-region redistributor regions (!count), * check that the redistributor region does not overlap with the * distributor's address space. */ > > > >> + 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? > no I can't because vgic_v3_check_base() currently only works with the > unique legacy rdist region. There, redist_size is > atomic_read(&kvm->online_vcpus) * KVM_VGIC_V3_REDIST_SIZE. Hmmm, ok. I'm not completely clear if that can be reworked to be reused or not, but perhaps you could just introduce a primitive ? static bool redist_overlaps_dist(struct kvm *kvm, gpa_t rd_base, size_t rd_size); > > > >> + > >> + /* 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? > Hum reminder from an old version :-( > > > >> + > >> + 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.) > sure > > > >> + } > >> + 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. > This is only used when reading the characteristics of a redist region > from userspace so I don't think we care. > ok, fine. Thanks, -Christoffer