Hi Zenghui,

On 2020-03-20 03:53, Zenghui Yu wrote:

Hi Marc,

On 2020/3/19 20:10, Marc Zyngier wrote:

quoted

quoted

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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel