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

[Zenghui Yu <yuzenghui@huawei.com>]

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@kernel.org>
>>
>> [...]
>>
>>> 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