[PATCH 8/8] KVM: arm-vgic: Support CPU interface reg access

[Alexander Graf]

On 26.09.2013, at 03:36, Alexander Graf wrote:

> 
> On 26.09.2013, at 03:15, Alexander Graf wrote:
> 
>> 
>> On 26.09.2013, at 02:54, Christoffer Dall wrote:
>> 
>>> On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote:
>>>> 
>>>> On 25.09.2013, at 23:30, Christoffer Dall wrote:
>>>> 
>>>>> On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote:
>>>>>> 
>>>>>> On 23.08.2013, at 20:20, Christoffer Dall wrote:
>>>>>> 
>>>>>>> Implement support for the CPU interface register access driven by MMIO
>>>>>>> address offsets from the CPU interface base address.  Useful for user
>>>>>>> space to support save/restore of the VGIC state.
>>>>>>> 
>>>>>>> This commit adds support only for the same logic as the current VGIC
>>>>>>> support, and no more.  For example, the active priority registers are
>>>>>>> handled as RAZ/WI, just like setting priorities on the emulated
>>>>>>> distributor.
>>>>>>> 
>>>>>>> Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
>>>>>>> ---
>>>>>>> virt/kvm/arm/vgic.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>>> 1 file changed, 62 insertions(+), 4 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>>>> index d44b5a1..257dbae 100644
>>>>>>> --- a/virt/kvm/arm/vgic.c
>>>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>>>> @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>>>>>>> static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu,
>>>>>>> 				 struct kvm_exit_mmio *mmio, phys_addr_t offset)
>>>>>>> {
>>>>>>> -	return true;
>>>>>>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>>>>> +	u32 reg, mask = 0, shift = 0;
>>>>>>> +	bool updated = false;
>>>>>>> +
>>>>>>> +	switch (offset & ~0x3) {
>>>>>>> +	case GIC_CPU_CTRL:
>>>>>>> +		mask = GICH_VMCR_CTRL_MASK;
>>>>>>> +		shift = GICH_VMCR_CTRL_SHIFT;
>>>>>>> +		break;
>>>>>>> +	case GIC_CPU_PRIMASK:
>>>>>>> +		mask = GICH_VMCR_PRIMASK_MASK;
>>>>>>> +		shift = GICH_VMCR_PRIMASK_SHIFT;
>>>>>>> +		break;
>>>>>>> +	case GIC_CPU_BINPOINT:
>>>>>>> +		mask = GICH_VMCR_BINPOINT_MASK;
>>>>>>> +		shift = GICH_VMCR_BINPOINT_SHIFT;
>>>>>>> +		break;
>>>>>>> +	case GIC_CPU_ALIAS_BINPOINT:
>>>>>>> +		mask = GICH_VMCR_ALIAS_BINPOINT_MASK;
>>>>>>> +		shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT;
>>>>>>> +		break;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (!mmio->is_write) {
>>>>>>> +		reg = (vgic_cpu->vgic_vmcr & mask) >> shift;
>>>>>>> +		memcpy(mmio->data, &reg, sizeof(reg));
>>>>>>> +	} else {
>>>>>>> +		memcpy(&reg, mmio->data, sizeof(reg));
>>>>>>> +		reg = (reg << shift) & mask;
>>>>>>> +		if (reg != (vgic_cpu->vgic_vmcr & mask))
>>>>>>> +			updated = true;
>>>>>>> +		vgic_cpu->vgic_vmcr &= ~mask;
>>>>>>> +		vgic_cpu->vgic_vmcr |= reg;
>>>>>>> +	}
>>>>>>> +	return updated;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static bool handle_mmio_abpr(struct kvm_vcpu *vcpu,
>>>>>>> +			     struct kvm_exit_mmio *mmio, phys_addr_t offset)
>>>>>>> +{
>>>>>>> +	return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu,
>>>>>>> +				  struct kvm_exit_mmio *mmio,
>>>>>>> +				  phys_addr_t offset)
>>>>>>> +{
>>>>>>> +	u32 reg;
>>>>>>> +
>>>>>>> +	if (mmio->is_write)
>>>>>>> +		return false;
>>>>>>> +
>>>>>>> +	reg = 0x0002043B;
>>>>>> 
>>>>>> This wants a comment and probably also a #define :).
>>>>>> 
>>>>> 
>>>>> Marc, where does the 0x4b0 product id code come from for the distributor
>>>>> IIDR?
>>>>> 
>>>>> Would this be satisfying?
>> 
>> ^
>> 
>>>>> 
>>>>> 
>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>> index 5214424..558be38 100644
>>>>> --- a/virt/kvm/arm/vgic.c
>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>> @@ -71,6 +71,9 @@
>>>>> #define VGIC_ADDR_UNDEF		(-1)
>>>>> #define IS_VGIC_ADDR_UNDEF(_x)  ((_x) == VGIC_ADDR_UNDEF)
>>>>> 
>>>>> +#define GIC_PRODUCT_ID		0x4b0
>>>> 
>>>> This is a specific GIC version. PL390 for example is 0x3b0:
>>>> 
>>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeba.html
>>>> 
>>>> That should be reflected in the #define. If it means "GICv2" then it should be GICV2_PRODUCT_ID for example.
>>>> 
>>> 
>>> I know what field in the register it is thanks :)
>>> 
>>> But I couldn't find 0x4b0 anywhere in the docs, so I'm asking
>>> Marc where he got it from.  I don't believe it means GICv2, but a
>> 
>> Ah, ok. Then the answer to your question above is a simple "no" as the name doesn't really tell us everything we want to know yet :).
>> 
>>> specific implementation of a GICv2, and once I have more info I can
>>> change the define name, I suspect this is potentially something made-up
>>> to indicate that this is the KVM VGIC...
>> 
>> Hrm, makes sense. So that also explains why there's a special version field.
> 
> It doesn't explain why it only gets set in one of the IIDR variants though. Is this on purpose? From what I can tell, the CPU and Distributor interfaces both should return the same number here.


Hrm. Curious. According to

  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0438i/BABGBHBG.html

the proper values for IIDR on an A15 are:

  GICD_IIDR 0x0000043B
  GICC_IIDR 0x0002043B

So what do the fields mean in each register?

GICD_IIDR:

  [31:24] ProductID
  [23:20] -
  [19:16] Variant
  [15:12] Revision
  [11:0] Implementer

GICC_IIDR:

  [31:20] ProductID
  [19:16] Architecture version
  [15:12] Revision
  [11:0] Implementer


So while 19:16 in GICC denotes the "Architecture version" (GICv2 for us), it means "Variant" for GICD. What a mess.


Alex