[PATCH v4 18/19] arm/arm64: KVM: enable kernel side of GICv3 emulation

[christoffer.dall@linaro.org (Christoffer Dall)]

On Mon, Nov 24, 2014 at 05:41:07PM +0000, Andre Przywara wrote:
> (Marc, can you comment on the last question below about the unaligned
> GICV mapping?)
> 
> Hi Christoffer,
> 
> On 24/11/14 09:09, Christoffer Dall wrote:
> > On Fri, Nov 14, 2014 at 10:08:02AM +0000, Andre Przywara wrote:
> >> With all the necessary GICv3 emulation code in place, we can now
> >> connect the code to the GICv3 backend in the kernel.
> >> The LR register handling is different depending on the emulated GIC
> >> model, so provide different implementations for each.
> >> Also allow non-v2-compatible GICv3 implementations (which don't
> >> provide MMIO regions for the virtual CPU interface in the DT), but
> >> restrict those hosts to support GICv3 guests only.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >> Changelog v3...v4:
> >> - handle differences between GICv2-on-v3 and GICv3-on-v3 in existing functions
> >> - remove init_*_emul() functions
> >> - remove max_vcpus setting (done in earlier patches now)
> >> - adapt to new vgic_v<n>_init_emulation behaviour
> >>
> >>  virt/kvm/arm/vgic-v3.c |   83 ++++++++++++++++++++++++++++++++----------------
> >>  virt/kvm/arm/vgic.c    |    5 +++
> >>  2 files changed, 60 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> >> index a04d208..4894c59 100644
> >> --- a/virt/kvm/arm/vgic-v3.c
> >> +++ b/virt/kvm/arm/vgic-v3.c
> >> @@ -34,6 +34,7 @@
> >>  #define GICH_LR_VIRTUALID		(0x3ffUL << 0)
> >>  #define GICH_LR_PHYSID_CPUID_SHIFT	(10)
> >>  #define GICH_LR_PHYSID_CPUID		(7UL << GICH_LR_PHYSID_CPUID_SHIFT)
> >> +#define ICH_LR_VIRTUALID_MASK		(BIT_ULL(32) - 1)
> >>  
> >>  /*
> >>   * LRs are stored in reverse order in memory. make sure we index them
> >> @@ -48,12 +49,17 @@ static struct vgic_lr vgic_v3_get_lr(const struct kvm_vcpu *vcpu, int lr)
> >>  	struct vgic_lr lr_desc;
> >>  	u64 val = vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)];
> >>  
> >> -	lr_desc.irq	= val & GICH_LR_VIRTUALID;
> >> -	if (lr_desc.irq <= 15)
> >> -		lr_desc.source	= (val >> GICH_LR_PHYSID_CPUID_SHIFT) & 0x7;
> >> +	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> >> +		lr_desc.irq = val & ICH_LR_VIRTUALID_MASK;
> >>  	else
> >> -		lr_desc.source = 0;
> >> -	lr_desc.state	= 0;
> >> +		lr_desc.irq = val & GICH_LR_VIRTUALID;
> >> +
> >> +	lr_desc.source = 0;
> >> +	if (lr_desc.irq <= 15 &&
> >> +	    vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> >> +		lr_desc.source	= (val >> GICH_LR_PHYSID_CPUID_SHIFT) & 0x7;
> >> +
> >> +	lr_desc.state   = 0;
> > 
> > super-nit-only-if-you-respin: you have a couple of tabs and extra spaces
> > in the two lines above that need to just be a single space before the
> > assignment operator on each line.
> > 
> >>  
> >>  	if (val & ICH_LR_PENDING_BIT)
> >>  		lr_desc.state |= LR_STATE_PENDING;
> >> @@ -68,8 +74,20 @@ static struct vgic_lr vgic_v3_get_lr(const struct kvm_vcpu *vcpu, int lr)
> >>  static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
> >>  			   struct vgic_lr lr_desc)
> >>  {
> >> -	u64 lr_val = (((u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT) |
> >> -		      lr_desc.irq);
> >> +	u64 lr_val;
> >> +
> >> +	lr_val = lr_desc.irq;
> >> +
> >> +	/*
> >> +	 * currently all guest IRQs are Group1, as Group0 would result
> > 
> > I guess you couldn't guess my comment, from last time, can you please
> > begin sentences with upper-case?  (only if you re-spin).
> > 
> >> +	 * in a FIQ in the guest, which it wouldn't expect.
> >> +	 * Eventually we want to make this configurable, so we may revisit
> >> +	 * this in the future.
> >> +	 */
> >> +	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> >> +		lr_val |= ICH_LR_GROUP;
> >> +	else
> >> +		lr_val |= (u32)lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT;
> >>  
> >>  	if (lr_desc.state & LR_STATE_PENDING)
> >>  		lr_val |= ICH_LR_PENDING_BIT;
> >> @@ -154,7 +172,14 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
> >>  	 */
> >>  	vgic_v3->vgic_vmcr = 0;
> >>  
> >> -	vgic_v3->vgic_sre = 0;
> >> +	/*
> >> +	 * Set the SRE_EL1 value depending on the configured
> >> +	 * emulated vGIC model.
> >> +	 */
> >> +	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
> >> +		vgic_v3->vgic_sre = ICC_SRE_EL1_SRE;
> > 
> > I think we left some of my questions from last round unanswered here
> > (you said you needed to go and think about it).  If the guest sets SRE=0
> > we will not currently preserve this value I think.
> 
> Which is intentional, as we are following "4.4.7 Implementations with
> Fixed System Register Enables", which allows RAO/WI semantics for the
> SRE bit if the GIC implementation does not care about GICv2
> compatibility (which is what we do for the guest).
> Does that make sense or am I missing something?
> 

I really hope you didn't mean that you left my question unanswered
intentionally, but that the code behaves the way it does, on purpose ;)

So I think the comment should focus on the non-trivial part, the code
easily reads as doing something based on the vgic_model, but your
*implementation choice* of the vgic is not trivial, and that is what
deserves a comment.

Hint: how many times did we come back to this so far?

> > The comment should clearly indicate that we're choosing a reset value
> > of the register for our implementation of the gic.
> > 
> > I'm fine with this change, but would like to know what the rationale
> > behind it is; wouldn't guests always initialize this value?
> > 
> >> +	else
> >> +		vgic_v3->vgic_sre = 0;
> >>  
> >>  	/* Get the show on the road... */
> >>  	vgic_v3->vgic_hcr = ICH_HCR_EN;
> >> @@ -215,28 +240,30 @@ int vgic_v3_probe(struct device_node *vgic_node,
> >>  
> >>  	gicv_idx += 3; /* Also skip GICD, GICC, GICH */
> >>  	if (of_address_to_resource(vgic_node, gicv_idx, &vcpu_res)) {
> >> -		kvm_err("Cannot obtain GICV region\n");
> >> -		ret = -ENXIO;
> >> -		goto out;
> >> -	}
> >> -
> >> -	if (!PAGE_ALIGNED(vcpu_res.start)) {
> >> -		kvm_err("GICV physical address 0x%llx not page aligned\n",
> >> -			(unsigned long long)vcpu_res.start);
> >> -		ret = -ENXIO;
> >> -		goto out;
> >> -	}
> >> -
> >> -	if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
> >> -		kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> >> -			(unsigned long long)resource_size(&vcpu_res),
> >> -			PAGE_SIZE);
> >> -		ret = -ENXIO;
> >> -		goto out;
> >> +		kvm_info("GICv3: GICv2 emulation not available\n");
> >> +		vgic->vcpu_base = 0;
> >> +	} else {
> >> +		if (!PAGE_ALIGNED(vcpu_res.start)) {
> >> +			kvm_err("GICV physical address 0x%llx not page aligned\n",
> >> +				(unsigned long long)vcpu_res.start);
> >> +			ret = -ENXIO;
> >> +			goto out;
> > 
> > shouldn't we be allowing an emulated gicv3 using the system registers in
> > this case then?
> 
> I guess not. If we have a GICV address that is not passing those tests,
> we should warn the user about it instead of unexpectedly restricting the
> virtualization capabilities, right?
> If the user desperately wants to use GIC virtualization despite having
> the odd mapping, he could remove the GICC/GICH/GICV at all from the DTB.
> 

You can still inform the user but allow the system register interface at
the same time.

We already have hardware where they got this wrong on gicv2, so there's
a chance we'll see it again on gicv3, and I see no technical reason why
the two things are related?  We may as well disable the entire gic then
and panic the kernel if we're going down that road, which we obviously
don't want to do.

-Christoffer