Date:   Thu, 01 Apr 2021 18:30:39 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Auger Eric <eric.auger@...hat.com>
Cc:     eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, kvmarm@...ts.cs.columbia.edu,
        drjones@...hat.com, alexandru.elisei@....com, james.morse@....com,
        suzuki.poulose@....com, shuah@...nel.org, pbonzini@...hat.com
Subject: Re: [PATCH v4 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace

On Thu, 01 Apr 2021 18:03:25 +0100,
Auger Eric <eric.auger@...hat.com> wrote:
> 
> Hi Marc,
> 
> On 4/1/21 3:42 PM, Marc Zyngier wrote:
> > Hi Eric,
> > 
> > On Thu, 01 Apr 2021 09:52:37 +0100,
> > Eric Auger <eric.auger@...hat.com> wrote:
> >>
> >> Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the
> >> reporting of GICR_TYPER.Last for userspace") temporarily fixed
> >> a bug identified when attempting to access the GICR_TYPER
> >> register before the redistributor region setting, but dropped
> >> the support of the LAST bit.
> >>
> >> Emulating the GICR_TYPER.Last bit still makes sense for
> >> architecture compliance though. This patch restores its support
> >> (if the redistributor region was set) while keeping the code safe.
> >>
> >> We introduce a new helper, vgic_mmio_vcpu_rdist_is_last() which
> >> computes whether a redistributor is the highest one of a series
> >> of redistributor contributor pages.
> >>
> >> The spec says "Indicates whether this Redistributor is the
> >> highest-numbered Redistributor in a series of contiguous
> >> Redistributor pages."
> >>
> >> The code is a bit convulated since there is no guarantee
> > 
> > nit: convoluted
> > 
> >> redistributors are added in a given reditributor region in
> >> ascending order. In that case the current implementation was
> >> wrong. Also redistributor regions can be contiguous
> >> and registered in non increasing base address order.
> >>
> >> So the index of redistributors are stored in an array within
> >> the redistributor region structure.
> >>
> >> With this new implementation we do not need to have a uaccess
> >> read accessor anymore.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@...hat.com>
> > 
> > This patch also hurt my head, a lot more than the first one.  See
> > below.
> > 
> >> ---
> >>  arch/arm64/kvm/vgic/vgic-init.c    |  7 +--
> >>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 97 ++++++++++++++++++++----------
> >>  arch/arm64/kvm/vgic/vgic.h         |  1 +
> >>  include/kvm/arm_vgic.h             |  3 +
> >>  4 files changed, 73 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> >> index cf6faa0aeddb2..61150c34c268c 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-init.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> >> @@ -190,6 +190,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> >>  	int i;
> >>  
> >>  	vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
> >> +	vgic_cpu->index = vcpu->vcpu_id;
> > 
> > Is it so that vgic_cpu->index is always equal to vcpu_id? If so, why
> > do we need another field? We can always get to the vcpu using a
> > container_of().
> > 
> >>  
> >>  	INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
> >>  	raw_spin_lock_init(&vgic_cpu->ap_list_lock);
> >> @@ -338,10 +339,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
> >>  	dist->vgic_dist_base = VGIC_ADDR_UNDEF;
> >>  
> >>  	if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> >> -		list_for_each_entry_safe(rdreg, next, &dist->rd_regions, list) {
> >> -			list_del(&rdreg->list);
> >> -			kfree(rdreg);
> >> -		}
> >> +		list_for_each_entry_safe(rdreg, next, &dist->rd_regions, list)
> >> +			vgic_v3_free_redist_region(rdreg);
> > 
> > Consider moving the introduction of vgic_v3_free_redist_region() into
> > a separate patch. On its own, that's a good readability improvement.
> > 
> >>  		INIT_LIST_HEAD(&dist->rd_regions);
> >>  	} else {
> >>  		dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
> >> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> index 987e366c80008..f6a7eed1d6adb 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> @@ -251,45 +251,57 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
> >>  		vgic_enable_lpis(vcpu);
> >>  }
> >>  
> >> +static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu)
> >> +{
> >> +	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
> >> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> >> +	struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
> >> +
> >> +	if (!rdreg)
> >> +		return false;
> >> +
> >> +	if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) {
> >> +		/* check whether there is no other contiguous rdist region */
> >> +		struct list_head *rd_regions = &vgic->rd_regions;
> >> +		struct vgic_redist_region *iter;
> >> +
> >> +		list_for_each_entry(iter, rd_regions, list) {
> >> +			if (iter->base == rdreg->base + rdreg->count * KVM_VGIC_V3_REDIST_SIZE &&
> >> +				iter->free_index > 0) {
> >> +			/* check the first rdist index of this region, if any */
> >> +				if (vgic_cpu->index < iter->rdist_indices[0])
> >> +					return false;
> > 
> > rdist_indices[] contains the vcpu_id of the vcpu associated with a
> > given RD in the region. At this stage, you have established that there
> > is another region that is contiguous with the one associated with our
> > vcpu. You also know that this adjacent region has a vcpu mapped in
> > (free_index isn't 0). Isn't that enough to declare that our vcpu isn't
> > last?  I definitely don't understand what the index comparison does
> > here.
> Assume the following case:
> 2 RDIST region
> region #0 contains rdist 1, 2, 4
> region #1, adjacent to #0 contains rdist 3
> 
> Spec days:
> "Indicates whether this Redistributor is the
> highest-numbered Redistributor in a series of contiguous
> Redistributor pages."
> 
> To me 4 is last and 3 is last too.

No, only 3 is last, assuming that region 0 is full. I think the
phrasing in the spec is just really bad. What this describes is that
at the end of a set of contiguous set of RDs, that last RD has Last
set. If two regions are contiguous, that's undistinguishable from a
single, larger region.

There is no such thing as a "redistributor number" anyway. The closest
thing there is would be "processor number", but that has nothing to do
with the RD itself.

> 
> 
> > 
> > It also seem to me that some of the complexity could be eliminated if
> > the regions were kept ordered at list insertion time.
> yes
> > 
> >> +			}
> >> +		}
> >> +	} else if (vgic_cpu->rdreg_index < rdreg->free_index - 1) {
> >> +		/* look at the index of next rdist */
> >> +		int next_rdist_index = rdreg->rdist_indices[vgic_cpu->rdreg_index + 1];
> >> +
> >> +		if (vgic_cpu->index < next_rdist_index)
> >> +			return false;
> > 
> > Same thing here. We are in the middle of the allocated part of a
> > region, which means we cannot be last. I still don't get the index
> > check.
> Because within a region, nothing hinders rdist from being allocated in
> non ascending order. I exercise those cases in the kvmselftests
> 
> one single RDIST region with the following rdists allocated there:
> 1, 3, 2
> 
> 3 and 2 are "last", right? Or did I miss something. Yes that's totally
> not natural to do that kind of allocation but the API allows to do that.

No, only 2 is last. I think you got tripped by the bizarre language in
the spec, and the behaviour of this Last bit is much simpler than what
you ended up with.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists