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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
