Messages in this thread | | | Subject | Re: [PATCH v5 20/23] KVM: arm64: GICv4.1: Plumb SGI implementation selection in the distributor | From | Zenghui Yu <> | Date | Mon, 23 Mar 2020 16:11:00 +0800 |
| |
Hi Marc,
On 2020/3/20 17:01, Marc Zyngier wrote: > 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.
Indeed. I didn't realize it.
> >> 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.
This is really a nice point to address my concern! I'm happy to see this in v6 now.
> > What do you think?
I agreed with you, with a bit nervous though. Some old guests (which have no knowledge about GICv4.1 vSGIs and don't care about nASSGIcap at all) will also fail to migrate from A to B, just because now we present two different (unused) GICD_TYPER2 registers to them.
Is it a little unfair to them :-) ?
Thanks, Zenghui
| |