Re: [PATCH v3 09/12] KVM: arm/arm64: Check all vcpu redistributors are set on map_resources

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

On 04/24/2018 11:08 PM, Christoffer Dall wrote:
> 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).
Yes I meant "to check all redistributors were assigned"
> 
>> 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?
Yes we need to, to handle the case where the userspace does not provide
sufficient rdist region space

create 8 vcpus (no iodev registered)
define a redist region for 4 (4 iodevs registered)
start the vcpus
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  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.
yes indeed.
> 
> 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".

Well at vgic_init time, vcpus are frozen but dist and redist regions are
not forced to be set, hence the late check we do in map_resources, on
1st VCPU run. Not sure I get this one?

Thanks

Eric
> 
>> +			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
>