Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor

[Marc Zyngier <maz@kernel.org>]

Hi Zenghui,

On 2020-03-20 03:53, Zenghui Yu wrote:
> Hi Marc,
> 
> On 2020/3/19 20:10, Marc Zyngier wrote:
>>> But I wonder that should we use nassgireq to *only* keep track what
>>> the guest had written into the GICD_CTLR.nASSGIreq.  If not, we may
>>> lose the guest-request bit after migration among hosts with different
>>> has_gicv4_1 settings.
>> 
>> I'm unsure of what you're suggesting here. If userspace tries to set
>> GICD_CTLR.nASSGIreq on a non-4.1 host, this bit will not latch.
> 
> This is exactly what I *was* concerning about.
> 
>> Userspace can check that at restore time. Or we could fail the
>> userspace write, which is a bit odd (the bit is otherwise RES0).
>> 
>> Could you clarify your proposal?
> 
> Let's assume two hosts below. 'has_gicv4_1' is true on host-A while
> it is false on host-B because of lack of HW support or the kernel
> parameter "kvm-arm.vgic_v4_enable=0".
> 
> If we migrate guest through A->B->A, we may end-up lose the initial
> guest-request "nASSGIreq=1" and don't use direct vSGI delivery for
> this guest when it's migrated back to host-A.

My point above is that we shouldn't allow the A->B migration the first
place, and fail it as quickly as possible. We don't know what the guest
has observed in terms of GIC capability, and it may not have enabled the
new flavour of SGIs just yet.

> This can be "fixed" by keep track of what guest had written into
> nASSGIreq. And we need to evaluate the need for using direct vSGI
> for a specified guest by 'has_gicv4_1 && nassgireq'.

It feels odd. It means we have more state than the HW normally has.
I have an alternative proposal, see below.

> But if it's expected that "if userspace tries to set nASSGIreq on
> a non-4.1 host, this bit will not latch", then this shouldn't be
> a problem at all.

Well, that is the semantics of the RES0 bit. It applies from both
guest and userspace.

And actually, maybe we can handle that pretty cheaply. If userspace
tries to restore GICD_TYPER2 to a value that isn't what KVM can
offer, we just return an error. Exactly like we do for GICD_IIDR.
Hence the following patch:

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 28b639fd1abc..e72dcc454247 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -156,6 +156,7 @@ static int vgic_mmio_uaccess_write_v3_misc(struct 
kvm_vcpu *vcpu,
  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;

  	switch (addr & 0x0c) {
+	case GICD_TYPER2:
  	case GICD_IIDR:
  		if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
  			return -EINVAL;

Being a RO register, writing something that isn't compatible with the
possible behaviour of the hypervisor will just return an error.

What do you think?

> Anyway this is not a big deal to me and I won't complain about it
> in the future ;-) Either way, for this patch:
> 
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>

Thanks a lot!

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