diff mbox series

[v2,11/12] KVM: selftests / xen: don't explicitly set the vcpu_info address

Message ID 20230918112148.28855-12-paul@xen.org
State New
Headers
Series KVM: xen: update shared_info and vcpu_info handling |

Commit Message

Paul Durrant Sept. 18, 2023, 11:21 a.m. UTC 
  From: Paul Durrant <pdurrant@amazon.com>

If the vCPU id is set and the shared_info is mapped using HVA then we can
infer that KVM has the ability to use a default vcpu_info mapping. Hence
we can stop setting the address of the vcpu_info structure.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>

v2:
 - New in this version.
---
 tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

David Woodhouse Sept. 18, 2023, 1:21 p.m. UTC | #1 
On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> If the vCPU id is set and the shared_info is mapped using HVA then we can
> infer that KVM has the ability to use a default vcpu_info mapping. Hence
> we can stop setting the address of the vcpu_info structure.

Again that means we're not *testing* it any more when the test is run
on newer kernels. Can we perhaps set it explicitly, after *half* the
tests are done? Maybe to a *different* address than the default which
is derived from the Xen vcpu_id? And check that the memcpy works right
when we do?
Paul Durrant Sept. 18, 2023, 1:26 p.m. UTC | #2 
On 18/09/2023 14:21, David Woodhouse wrote:
> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>> From: Paul Durrant <pdurrant@amazon.com>
>>
>> If the vCPU id is set and the shared_info is mapped using HVA then we can
>> infer that KVM has the ability to use a default vcpu_info mapping. Hence
>> we can stop setting the address of the vcpu_info structure.
> 
> Again that means we're not *testing* it any more when the test is run
> on newer kernels. Can we perhaps set it explicitly, after *half* the
> tests are done? Maybe to a *different* address than the default which
> is derived from the Xen vcpu_id? And check that the memcpy works right
> when we do?
> 

Ok. The VMM is currently responsible for that memcpy. Are you suggesting 
we push that into KVM too?

   Paul
David Woodhouse Sept. 18, 2023, 1:36 p.m. UTC | #3 
On Mon, 2023-09-18 at 14:26 +0100, Paul Durrant wrote:
> On 18/09/2023 14:21, David Woodhouse wrote:
> > On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
> > > From: Paul Durrant <pdurrant@amazon.com>
> > > 
> > > If the vCPU id is set and the shared_info is mapped using HVA then we can
> > > infer that KVM has the ability to use a default vcpu_info mapping. Hence
> > > we can stop setting the address of the vcpu_info structure.
> > 
> > Again that means we're not *testing* it any more when the test is run
> > on newer kernels. Can we perhaps set it explicitly, after *half* the
> > tests are done? Maybe to a *different* address than the default which
> > is derived from the Xen vcpu_id? And check that the memcpy works right
> > when we do?
> > 
> 
> Ok. The VMM is currently responsible for that memcpy. Are you suggesting 
> we push that into KVM too?

Ah OK.

Hm, maybe we should?

What happened before in the case where interrupts were being delivered,
and the vcpu_info address was changed. 

In Xen, I guess it's effectively atomic? Some locking will mean that
the event channel is delivered to the vcpu_info either *before* the
memcpy, or *after* it, but never to the old address after the copy has
been done, so that the event (well the index of it) gets lost?

In KVM before we did the automatic placement, it was the VMM's problem.

If there are any interrupts set up for direct delivery, I suppose the
VMM should have *removed* the vcpu_info mapping before doing the
memcpy, then restored it at the new address? I may have to check qemu
gets that right.

Then again, it's a very hard race to trigger, given that a guest can
only set the vcpu_info once. So it can move it from the shinfo to a
separate address and attempt to trigger this race just that one time.

But in the case where auto-placement has happened, and then the guest
sets an explicit vcpu_info location... are we saying that the VMM must
explicitly *unmap* the vcpu_info first, then memcpy, then set it to the
new location? Or will we handle the memcpy in-kernel?
Paul Durrant Sept. 18, 2023, 1:41 p.m. UTC | #4 
On 18/09/2023 14:36, David Woodhouse wrote:
> On Mon, 2023-09-18 at 14:26 +0100, Paul Durrant wrote:
>> On 18/09/2023 14:21, David Woodhouse wrote:
>>> On Mon, 2023-09-18 at 11:21 +0000, Paul Durrant wrote:
>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>
>>>> If the vCPU id is set and the shared_info is mapped using HVA then we can
>>>> infer that KVM has the ability to use a default vcpu_info mapping. Hence
>>>> we can stop setting the address of the vcpu_info structure.
>>>
>>> Again that means we're not *testing* it any more when the test is run
>>> on newer kernels. Can we perhaps set it explicitly, after *half* the
>>> tests are done? Maybe to a *different* address than the default which
>>> is derived from the Xen vcpu_id? And check that the memcpy works right
>>> when we do?
>>>
>>
>> Ok. The VMM is currently responsible for that memcpy. Are you suggesting
>> we push that into KVM too?
> 
> Ah OK.
> 
> Hm, maybe we should?
> 
> What happened before in the case where interrupts were being delivered,
> and the vcpu_info address was changed.
> 
> In Xen, I guess it's effectively atomic? Some locking will mean that
> the event channel is delivered to the vcpu_info either *before* the
> memcpy, or *after* it, but never to the old address after the copy has
> been done, so that the event (well the index of it) gets lost?
> 
> In KVM before we did the automatic placement, it was the VMM's problem.
> 
> If there are any interrupts set up for direct delivery, I suppose the
> VMM should have *removed* the vcpu_info mapping before doing the
> memcpy, then restored it at the new address? I may have to check qemu
> gets that right.
> 
> Then again, it's a very hard race to trigger, given that a guest can
> only set the vcpu_info once. So it can move it from the shinfo to a
> separate address and attempt to trigger this race just that one time.
> 
> But in the case where auto-placement has happened, and then the guest
> sets an explicit vcpu_info location... are we saying that the VMM must
> explicitly *unmap* the vcpu_info first, then memcpy, then set it to the
> new location? Or will we handle the memcpy in-kernel?
> 

Well, if the VMM is using the default then it can't unmap it. But 
setting a vcpu_info *after* enabling any event channels would be a very 
odd thing for a guest to do and IMO it gets to keep the pieces if it 
does so.

   Paul
David Woodhouse Sept. 18, 2023, 2:05 p.m. UTC | #5 
On 18 September 2023 14:41:08 BST, Paul Durrant <xadimgnik@gmail.com> wrote:
>Well, if the VMM is using the default then it can't unmap it. But setting a vcpu_info *after* enabling any event channels would be a very odd thing for a guest to do and IMO it gets to keep the pieces if it does so.


Hm, I suppose I'm OK with that approach. The fact that both VMM implementations using this KVM/Xen support let the guest keep precisely those pieces is a testament to that :)

But now we're hard-coding the behaviour in the kernel and declaring that no VMM will be *able* to "fix" that case even if it does want to. So perhaps it wants a modicum more thought and at least some explicit documentation to that effect?

And a hand-wavy plan at least for what we'd do if we suddenly did find a reason to care?
Paul Durrant Sept. 18, 2023, 2:14 p.m. UTC | #6 
On 18/09/2023 15:05, David Woodhouse wrote:
> 
> 
> On 18 September 2023 14:41:08 BST, Paul Durrant <xadimgnik@gmail.com> wrote:
>> Well, if the VMM is using the default then it can't unmap it. But setting a vcpu_info *after* enabling any event channels would be a very odd thing for a guest to do and IMO it gets to keep the pieces if it does so.
> 
> 
> Hm, I suppose I'm OK with that approach. The fact that both VMM implementations using this KVM/Xen support let the guest keep precisely those pieces is a testament to that :)
> 

I can have the selftest explicitly set the vcpu_info to point at the one 
that's already in use, I suppose... so the would at least make sure the 
attribute is functioning.

> But now we're hard-coding the behaviour in the kernel and declaring that no VMM will be *able* to "fix" that case even if it does want to. So perhaps it wants a modicum more thought and at least some explicit documentation to that effect?
> 
> And a hand-wavy plan at least for what we'd do if we suddenly did find a reason to care?

Handwavy plan would be for the VMM to:

a) Mask all open event channels targetting the vcpu
b) Copy vcpu_info content to the new location
c) Tell KVM where it is
d) Unmask the masked event channels

Does that sound ok? If so I can stick it in the API documentation.
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index fa829d6e0848..d1c88deec0b2 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -550,11 +550,13 @@  int main(int argc, char *argv[])
 		vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vid);
 	}
 
-	struct kvm_xen_vcpu_attr vi = {
-		.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
-		.u.gpa = VCPU_INFO_ADDR,
-	};
-	vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vi);
+	if (!has_vcpu_id || !has_shinfo_hva) {
+		struct kvm_xen_vcpu_attr vi = {
+			.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
+			.u.gpa = VCPU_INFO_ADDR,
+		};
+		vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &vi);
+	}
 
 	struct kvm_xen_vcpu_attr pvclock = {
 		.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO,