From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752294AbeDXVIQ (ORCPT ); Tue, 24 Apr 2018 17:08:16 -0400 Received: from foss.arm.com ([217.140.101.70]:59994 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752193AbeDXVIM (ORCPT ); Tue, 24 Apr 2018 17:08:12 -0400 Date: Tue, 24 Apr 2018 23:08:09 +0200 From: Christoffer Dall To: Eric Auger 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 09/12] KVM: arm/arm64: Check all vcpu redistributors are set on map_resources Message-ID: <20180424210809.GJ4533@C02W217FHV2R.local> References: <1523607658-9166-1-git-send-email-eric.auger@redhat.com> <1523607658-9166-10-git-send-email-eric.auger@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1523607658-9166-10-git-send-email-eric.auger@redhat.com> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 13, 2018 at 10:20:55AM +0200, Eric Auger wrote: > On vcpu first run, we eventually know the actual number of vcpus. > This is a synchronization point to check all redistributors regions > were assigned. Isn't it the other way around? We want to check that all redistributors (one for every VCPU) has its base address set? (I don't suppose we care about unused redistributor regions). > On kvm_vgic_map_resources() we check both dist and > redist were set, eventually check potential base address inconsistencies. Do we need to check that again? Didn't we check that at creation/register time? > > Signed-off-by: Eric Auger > --- > virt/kvm/arm/vgic/vgic-v3.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index b80f650..feabc24 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -484,16 +484,25 @@ struct vgic_redist_region *vgic_v3_rdist_region_from_index(struct kvm *kvm, > > int vgic_v3_map_resources(struct kvm *kvm) > { > - int ret = 0; > struct vgic_dist *dist = &kvm->arch.vgic; > - struct vgic_redist_region *rdreg = > - list_first_entry(&dist->rd_regions, > - struct vgic_redist_region, list); > + struct kvm_vcpu *vcpu; > + int ret = 0; > + int c; > > if (vgic_ready(kvm)) > goto out; > > - if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) || !rdreg) { > + kvm_for_each_vcpu(c, vcpu, kvm) { > + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + > + if (IS_VGIC_ADDR_UNDEF(vgic_cpu->rd_iodev.base_addr)) { > + kvm_err("vcpu %d redistributor base not set\n", c); Shouldn't this be a kvm_debug instead? I think the user can just provoke this by failing to register enough redistributor regions. I'm also wondering if we could check this on vgic_init time for gicv3, which should have a defined vgic init ordering requirement. That would make debugging it slightly easier to debug than "my machine isn't starting, and I get -ENXIO, and it can mean anything". > + ret = -ENXIO; > + goto out; > + } > + } > + > + if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base)) { > kvm_err("Need to set vgic distributor addresses first\n"); > ret = -ENXIO; > goto out; > -- > 2.5.5 > Thanks, -Christoffer