[PATCH 22/23] KVM: arm64: Add a rVIC/rVID in-kernel implementation

[Lorenzo Pieralisi]

On Tue, Sep 29, 2020 at 04:27:45PM +0100, Marc Zyngier wrote:
> Hi Lorenzo,
> 
> On 2020-09-29 16:13, Lorenzo Pieralisi wrote:
> > On Thu, Sep 03, 2020 at 04:26:09PM +0100, Marc Zyngier wrote:
> > 
> > [...]
> > 
> > > +static void __rvic_sync_hcr(struct kvm_vcpu *vcpu, struct rvic *rvic,
> > > +			    bool was_signaling)
> > > +{
> > > +	struct kvm_vcpu *target = kvm_rvic_to_vcpu(rvic);
> > > +	bool signal = __rvic_can_signal(rvic);
> > > +
> > > +	/* We're hitting our own rVIC: update HCR_VI locally */
> > > +	if (vcpu == target) {
> > > +		if (signal)
> > > +			*vcpu_hcr(vcpu) |= HCR_VI;
> > > +		else
> > > +			*vcpu_hcr(vcpu) &= ~HCR_VI;
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Remote rVIC case:
> > > +	 *
> > > +	 * We kick even if the interrupt disappears, as ISR_EL1.I must
> > > +	 * always reflect the state of the rVIC. This forces a reload
> > > +	 * of the vcpu state, making it consistent.
> > 
> > Forgive me the question but this is unclear to me. IIUC here we do _not_
> > want to change the target_vcpu.hcr and we force a kick to make sure it
> > syncs the hcr (so the rvic) state on its own upon exit. Is that correct
> > ?
> 
> This is indeed correct. Changing the vcpu's hcr is racy as we sometimes
> update it on vcpu exit, so directly updating this field would require
> introducing atomic accesses between El1 and EL2. Not happening.
> 
> Instead, we force the vcpu to reload its own state as it *reenters*
> the guest (and not on exit). This way, no locking, no cmpxchg, everything
> is still single threaded.
> 
> > Furthermore, I think it would be extremely useful to elaborate (ie
> > rework the comment) further on ISR_EL1.I and how it is linked to this
> > code path - I think it is key to understanding it.
> 
> I'm not really sure what to add here, apart from paraphrasing the ARM ARM.
> ISR_EL1 always represents the interrupt input to the CPU, virtual or not,
> and we must honor this requirements by making any change of output of
> the rVIC directly observable (i.e. update HCR_EL2.VI).

Ok got it. Basically we kick the target_vcpu because there is a change
in its IRQ input signal that has to be signalled (by updating the
HCR_EL2.VI that in turns updates the ISR_EL1.I). I read the comment as
if we don't care about the target_vcpu.hcr_el2 update and was struggling
to understand the whole code path.

Back to reading the code (properly :)).

Thanks,
Lorenzo