Re: [PATCH] KVM: x86/xen: Add KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3985 bytes --]

On Fri, 2026-06-26 at 06:40 -0700, Sean Christopherson wrote:
> On Fri, Jun 26, 2026, David Woodhouse wrote:
> > On Thu, 2026-06-25 at 16:09 -0700, Sean Christopherson wrote:
> > > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > > > index 91fd3673c09a..c16b4560c9e7 100644
> > > > --- a/arch/x86/kvm/xen.c
> > > > +++ b/arch/x86/kvm/xen.c
> > > > @@ -907,6 +907,13 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> > > >  {
> > > >  	int idx, r = -ENOENT;
> > > >  
> > > > +	/*
> > > > +	 * kvm_xen_write_hypercall_page() manages its own locking.
> > > > +	 * Handle it before taking xen_lock to avoid a deadlock.
> > > 
> > > Do we actually want the side effects that necessitate taking xen.xen_lock?  From
> > > a uAPI perspective, it's odd to effectively bundle KVM_XEN_ATTR_TYPE_LONG_MODE
> > > into KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE.
> > 
> > That's *guest* ABI, and it's derived from Xen behaviour. Xen will
> > 'latch' its idea of whether a guest VM is 32-bit or 64-bit, for the
> > purpose of shared data structures (shared_info page, vcpu_info,
> > runstate).
> > 
> > Xen latches this from the current mode of the running vCPU in *two*
> > places:
> >  • When the hypercall MSR is invoked
> >  • When the guest sets the event channel GSI (HVM_PARAM_CALLBACK_IRQ).
> > 
> > Thus far, the former has been handled in the kernel (in the code you're
> > looking at), while the latter is why we have the ioctl to explicitly
> > latch the guest's long_mode from userspace too, as userspace handles
> > the HVMOP_set_param calls.
> 
> Right, and I'm pointing out that from a KVM uAPI perspective, bundling the first
> one in a "write hypercall page" call is rather odd, especially since there's
> already uAPI to handle the latching.

Mostly just because they came in the opposite order. The kernel was
writing the hypercall page for itself for about a decade before there
was anything to latch. And userspace wasn't *involved* when it happened
(initially because there was no facility for userspace intercepting MSR
writes).

> > > The other question is, why does kvm_xen_write_hypercall_page() drop xen_lock
> > > when writing guest memory?  That seems odd and unnecessary.
> > 
> > Huh? It takes the lock to do the thing that needs the lock, then drops
> > it. That is not "odd and unnecessary" at all.
> > 
> > You've been spending too long with these scope-guarded locks.
> 
> No, I'm asking why KVM doesn't serialize the writes to guest memory.  Usually
> when KVM writes to guest memory, KVM is emulating something that is very much
> vCPU-specific, and so if there are races it's the guest's problem to deal with.
> 
> The Xen MSR here is clearly VM-scoped though, which is why it feels odd to take
> a per-VM lock, and then deliberately drop the lock before completing the operation,
> In practice it shouldn't matter, since it sounds like the same repeating 16 byte
> pattern will be written every time, but it was a bit head-scratching when reading
> the code.

The lock protects the long_mode latching. It wouldn't make sense to
hold it around the actual writing of the hypercall page. It's not
intended to be an *atomic* write, and holding some random lock around
it wouldn't make it atomic with respect to all the other things that
can write guest memory anyway.

> We can/should return whatever kvm_vcpu_write_guest() returns, i.e. literally
> return its result directly.  Which of course is only ever going to be -EFAULT,
> but in the extremely unlikely case that ever changes, we won't have to worry
> about creating misleading behavior in the Xen code.

Yes, and if we do it with a wrapper as you suggested, we can return a
proper error code and the MSR handler can convert that to 0/1.

As it is, we were doing the conversion the other way round, which is
why we were making up error numbers for the failure cause.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]