Re: [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface.

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

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

> On 05/07/17 12:23, wanghaibin wrote:
>> Access GICV_APRn hardware register, SPEC says:
>>
>> When System register access is disabled for EL2, these registers access GICH_APR<n>,
>> and all active priorities for virtual machines are held in GICH_APR<n> regardless of
>> interrupt group.
>> When System register access is enabled for EL2, these registers access ICH_AP1R<n>_EL2,
>> and all active priorities for virtual machines are held in ICH_AP1R<n>_EL2 regardless
>> of interrupt group.
>>
>> Accordingly, the following two cases are implemented:
>> (1) GICv2 on GICv2, uaccess GICC_APRn info from GICH_APRn, and,
>> (2) GICv2 on GICv3, uaccess GICC_APRn info from ICH_AP1Rn.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 ++++++++++++++++++++--
>>  virt/kvm/arm/vgic/vgic-mmio.c    | 28 ++++++++++++++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.h         |  3 +++
>>  3 files changed, 51 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index 63e0bbd..176b93f 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -303,6 +303,23 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
>>  	vgic_set_vmcr(vcpu, &vmcr);
>>  }
>>  
>> +static unsigned long vgic_v2_uaccess_read_activeprio(struct kvm_vcpu *vcpu,
>> +						     gpa_t addr, unsigned int len)
>> +{
>> +	u32 idx = (addr & 0x0c) / sizeof(u32);
>> +
>> +	return vgic_get_apr(vcpu, 0, idx);
>> +}
>> +
>> +static void vgic_v2_uaccess_write_activeprio(struct kvm_vcpu *vcpu,
>> +					     gpa_t addr, unsigned int len,
>> +					     unsigned long val)
>> +{
>> +       u32 idx = (addr & 0x0c) / sizeof(u32);
>> +
>> +       return vgic_set_apr(vcpu, 0, idx, val);
>> +}
>> +
>>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
>>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>> @@ -361,8 +378,9 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
>>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ALIAS_BINPOINT,
>>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>>  		VGIC_ACCESS_32bit),
>> -	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
>> +	REGISTER_DESC_WITH_LENGTH_UACCESS(GIC_CPU_ACTIVEPRIO,
>> +		vgic_mmio_read_raz, vgic_mmio_write_wi,
>> +		vgic_v2_uaccess_read_activeprio, vgic_v2_uaccess_write_activeprio, 16,
> 
> This 16 feels wrong. I now see why you had this index in the first
> patch. I think it begs the questions of what we're emulating. Are we
> showing a virtual GICv2 with only 5 bits of priority? Or something else?


Hi, Marc:

At present, Yes, I think it's only 5 bits of priority to show for GICv2.

But， the QEMU gicc apr put/get access code just like:

        /* s->apr[n][cpu] -> GICC_APRn */
        for (i = 0; i < 4; i++) {
            reg = s->apr[i][cpu];
            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, true);
        }

It get/put all gicc apr registers (the spec implement GICC_APR<n> (n = 0 - 3) to provides information about
interrupt active priorities), So I think the 16 is correct.


BTW:  I notice that we show the 5 priority bits by VGIC_PRI_BITS to filter emulate write GICD_IPRIORITYRn bits （vgic_mmio_write_priority）
both emulate GICv2 and emulate GICv3.

Should we cancel this limit when (GICv2 on GICv3) and (GICv3 on GICv3) ?
I think we can show the bits which the hardware ICH_VTR_EL2.PRIbits support when the hardware is GICv3 to avoid priority bits lost.


Thanks.

> 
>>  		VGIC_ACCESS_32bit),
>>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
>>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index 1c17b2a..42fe00d 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -454,6 +454,34 @@ static int match_region(const void *key, const void *elt)
>>  		       sizeof(regions[0]), match_region);
>>  }
>>  
>> +void vgic_set_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx, u32 val)
> 
> Same remark as before about "apr".
> 
>> +{
>> +	u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
>> +
>> +	if (kvm_vgic_global_state.type == VGIC_V2)
>> +		vgic_v2_set_apr(vcpu, idx, val);
>> +	else {
>> +		if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>> +			vgic_v3_set_apr(vcpu, 1, idx, val);
>> +		else
>> +			vgic_v3_set_apr(vcpu, apr, idx, val);
>> +	}
>> +}
>> +
>> +u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx)
>> +{
>> +	u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
>> +
>> +	if (kvm_vgic_global_state.type == VGIC_V2)
>> +		return vgic_v2_get_apr(vcpu, idx);
>> +	else {
>> +		if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>> +			return vgic_v3_get_apr(vcpu, 1, idx);
>> +		else
>> +			return vgic_v3_get_apr(vcpu, apr, idx);
>> +	}
>> +}
>> +
>>  void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
>>  {
>>  	if (kvm_vgic_global_state.type == VGIC_V2)
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index 91d66ec..3cdc448 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -213,6 +213,9 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
>>  int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>>  				    u32 intid, u64 *val);
>>  int kvm_register_vgic_device(unsigned long type);
>> +
>> +void vgic_set_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx, u32 val);
>> +u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx);
>>  void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
>>  void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
>>  int vgic_lazy_init(struct kvm *kvm);
>>
> 
> Thanks,
> 
> 	M.



_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm