Re: [PATCH v4 16/20] KVM: arm64: GICv4.1: Allow SGIs to switch between HW and SW interrupts

[Zenghui Yu]

Hi Marc,

On 2020/2/29 3:16, Marc Zyngier wrote:
> Hi Zenghui,
>
> On 2020-02-20 03:55, Zenghui Yu wrote:
> > Hi Marc,
> >
> > On 2020/2/14 22:57, Marc Zyngier wrote:
> > > In order to let a guest buy in the new, active-less SGIs, we
> > > need to be able to switch between the two modes.
> > >
> > > Handle this by stopping all guest activity, transfer the state
> > > from one mode to the other, and resume the guest.
> > >
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> >
> > [...]
> >
> > > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> > > index 1bc09b523486..2c9fc13e2c59 100644
> > > --- a/virt/kvm/arm/vgic/vgic-v3.c
> > > +++ b/virt/kvm/arm/vgic/vgic-v3.c
> > > @@ -540,6 +540,8 @@ int vgic_v3_map_resources(struct kvm *kvm)
> > > ÂÂÂÂÂÂÂÂÂ goto out;
> > > ÂÂÂÂÂ }
> > > Â +ÂÂÂ if (kvm_vgic_global_state.has_gicv4_1)
> > > +ÂÂÂÂÂÂÂ vgic_v4_configure_vsgis(kvm);
> > > ÂÂÂÂÂ dist->ready = true;
> > > ÂÂÂ out:
> >
> > Is there any reason to invoke vgic_v4_configure_vsgis() here?
> > This is called on the first VCPU run, through kvm_vgic_map_resources().
> > Shouldn't the vSGI configuration only driven by a GICD_CTLR.nASSGIreq
> > writing (from guest, or from userspace maybe)?
>
> What I'm trying to catch here is the guest that has been restored with
> nASSGIreq set. At the moment, we don't do anything on the userspace
> side, because the vmm could decide to write that particular bit
> multiple times, and switching between the two modes is expensive (not
> to mention that all the vcpus may not have been created yet).
>
> Moving it to the first run makes all these pitfalls go away (we have the
> final nASSSGIreq value, and all the vcpus are accounted for).

So what will happen on restoration is (roughly):

 - for GICR_ISPENR0: We will restore the pending status of vSGIs into
   software pending_latch, just like what we've done for normal SGIs.
 - for GICD_CTLR.nASSGIreq: We will only record the written value.
   (Note to myself: No invocation of configure_vsgis() in uaccess_write
    callback, I previously mixed it up with the guest write callback.)
 - Finally, you choose the first vcpu run as the appropriate point to
   potentially flush the pending status to HW according to the final
   nASSGIreq value.

> Does this make sense to you?

Yeah, it sounds like a good idea! And please ignore what I've replied to
patch #15, I obviously missed your intention at that time, sorry...

But can we move this hunk to some places more appropriate, for example,
put it together with the GICD_CTLR's uaccess_write change? It might make
things a bit clearer for other reviewers. :-)


Thanks,
Zenghui