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

[Marc Zyngier]

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).

         M.
-- 
Jazz is not dead. It just smells funny...