Re: [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2

[wanghaibin <wanghaibin.wang@huawei.com>]

On 2017/7/7 0:13, Marc Zyngier wrote:

> On 05/07/17 12:23, wanghaibin wrote:
>> For GICv2, there are at most 5 priority bits are implemented in
>> GICH_LR<n>.Priority, so we only need to be concerned with GICH_APR0.
>> The other GICH_APRn access can be treated as raz/wi.
> 
> What is this "other" GICH_APRn?
> 
>>
>> Attention: This patch is untest!
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-v2.c | 21 +++++++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.h    |  2 ++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
>> index e4187e5..11d3b73 100644
>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>> @@ -172,6 +172,27 @@ void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr)
>>  	vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = 0;
>>  }
>>  
>> +void vgic_v2_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
>> +{
>> +	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
>> +
>> +	if (idx == 0)
>> +		cpu_if->vgic_apr = val;
>> +	else
>> +		WARN_ON(val);
> 
> If treated as WI, why do you WARN here? Also, given that there is only
> one register for the active priorities, I don't really see the point in
> having this "idx" parameter.
> 

About idx parameter, the patch3 will reply.

About WARN_ON, GICv2 on GICv2, we just show 5 priority bits currently.
If uaccess set apr[1/2/3] with non-zero value, there must be something wrong,  here take a warning.

Of course, this can be deleted.

Thanks.

>> +}
>> +
>> +u32 vgic_v2_get_apr(struct kvm_vcpu *vcpu, u32 idx)
>> +{
>> +	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
>> +
>> +	if (idx == 0)
>> +		return cpu_if->vgic_apr;
>> +	else
>> +		return 0;
>> +}
>> +
>> +
> 
> Extra whitespace.


will fix.

Thanks.

> 
>>  void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
>>  {
>>  	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index bba7fa2..8791b9a 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -155,6 +155,8 @@ int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>>  			 int offset, u32 *val);
>>  int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>>  			  int offset, u32 *val);
>> +void vgic_v2_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val);
>> +u32 vgic_v2_get_apr(struct kvm_vcpu *vcpu, u32 idx);
>>  void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
>>  void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
>>  void vgic_v2_enable(struct kvm_vcpu *vcpu);
>>
> 
> Thanks,
> 
> 	M.