Re: [PATCH v5 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace

[Auger Eric <eric.auger@redhat.com>]

Hi Marc,

On 4/5/21 12:10 PM, Marc Zyngier wrote:
> On Sun, 04 Apr 2021 18:22:42 +0100,
> Eric Auger <eric.auger@redhat.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.
>>
>> With this new implementation we do not need to have a uaccess
>> read accessor anymore.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v4 -> v5:
>> - redist region list now is sorted by @base
>> - change the implementation according to Marc's understanding of
>>   the spec
>> ---
>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 58 +++++++++++++++++-------------
>>  include/kvm/arm_vgic.h             |  1 +
>>  2 files changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> index e1ed0c5a8eaa..03a253785700 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -251,45 +251,52 @@ 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 *iter, *rdreg = vgic_cpu->rdreg;
>> +
>> +	if (!rdreg)
>> +		return false;
>> +
>> +	if (vgic_cpu->rdreg_index < rdreg->free_index - 1) {
>> +		return false;
>> +	} else if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) {
>> +		struct list_head *rd_regions = &vgic->rd_regions;
>> +		gpa_t end = rdreg->base + rdreg->count * KVM_VGIC_V3_REDIST_SIZE;
>> +
>> +		/*
>> +		 * the rdist is the last one of the redist region,
>> +		 * check whether there is no other contiguous rdist region
>> +		 */
>> +		list_for_each_entry(iter, rd_regions, list) {
>> +			if (iter->base == end && iter->free_index > 0)
>> +				return false;
>> +		}
> 
> In the above notes, you state that the region list is now sorted by
> base address, but I really can't see what sorts that list. And the
> lines above indicate that you are still iterating over the whole RD
> regions.
> 
> It's not a big deal (the code is now simple enough), but that's just
> to confirm that I understand what is going on here.

Sorry I should have removed the notes. I made the change but then I
noticed that the list was already sorted by redistributor region index
as the API forbids to register rdist regions in non ascending index
order. So sorting by base address was eventually causing more trouble
than it helped.

Thanks

Eric
> 
> Thanks,
> 
> 	M.
>