Messages in this thread | | | Subject | Re: [PATCH v7 11/11] KVM: xen: allow vcpu_info content to be 'safely' copied | From | David Woodhouse <> | Date | Wed, 08 Nov 2023 21:56:17 +0000 |
| |
On Wed, 2023-11-08 at 12:55 -0800, Sean Christopherson wrote: > > Any objection to splitting out the vcpu_info chunk to a separate function? That'd > eliminate the subtle gpc+lock switch, and provide a convenient and noticeable > location to explain why it's ok for setting the vcpu_info to fail.
No objection, that looks entirely sane to me.
> E.g. > > --- > arch/x86/kvm/xen.c | 102 ++++++++++++++++++++++++++------------------- > 1 file changed, 58 insertions(+), 44 deletions(-) > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > index d65e89e062e4..cd2021ba0ae3 100644 > --- a/arch/x86/kvm/xen.c > +++ b/arch/x86/kvm/xen.c > @@ -1621,6 +1621,57 @@ static void kvm_xen_check_poller(struct kvm_vcpu *vcpu, int port) > } > } > > +static int kvm_xen_needs_a_name(struct kvm_vcpu *vcpu, int port_word_bit) >
kvm_xen_deliver_vcpu_evtchn_pending_sel() ?
...
> - out_rcu: > + out_unlock: > read_unlock_irqrestore(&gpc->lock, flags); > + > + /* Paul to add comment explaining why it's ok kvm_xen_needs_a_name() fails */
Basically the point here is that it *doesn't* fail. Either it delivers directly to the vcpu_info at the appropriate GPA via the GPC, or it will set the corresponding bit in the 'shadow' vcpu->arch.xen.evtchn_pending_sel to be delivered later by kvm_xen_inject_pending_events(). Either way, it's 'success'.
I do want to vet the code path where userspace forgets to reinstate the vcpu_info before running the vCPU again. Looking at kvm_xen_inject_pending_events(), I think the kvm_gpc_check() for the vcpu_info will fail, so it'll just return, and KVM will enter the vCPU with vcpu->arch.xen.evtchn_pending_sel still unchanged? The bits won't get delivered until the next time the vCPU is entered after the vcpu_info is set.
Which I *think* is fine. Userspace has to stop running the vCPU in order to actually set the vcpu_info again, when it finally decides to do so. And if it never does, that works out as well as can be expected.
Probably does want adding to the selftest.
[unhandled content-type:application/pkcs7-signature] | |