Fix a '40' vs '0x40' bug in the new Xen shinfo selftest, and clean up some
other oddities that made root causing the problem far more painful than it
needed to be.
Note, Paolo already queued a patch from Vitaly that adds the tests to
.gitignore[*], i.e. patch 01 can likely be dropped. I included it here
for completeness.
[*] https://lkml.kernel.org/r/[email protected]
Sean Christopherson (5):
KVM: selftests: Ignore recently added Xen tests' build output
KVM: selftests: Fix size of memslots created by Xen tests
KVM: selftests: Fix hex vs. decimal snafu in Xen test
KVM: sefltests: Don't bother mapping GVA for Xen shinfo test
KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes
arch/x86/kvm/xen.h | 11 ++++++-----
tools/testing/selftests/kvm/.gitignore | 2 ++
tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 12 +++++-------
tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c | 3 +--
4 files changed, 14 insertions(+), 14 deletions(-)
--
2.30.0.478.g8a0d178c01-goog
Add the new Xen test binaries to KVM selftest's .gitnore.
Signed-off-by: Sean Christopherson <[email protected]>
---
tools/testing/selftests/kvm/.gitignore | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 1b32c97f8c82..3a84394829ea 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -26,6 +26,8 @@
/x86_64/vmx_set_nested_state_test
/x86_64/vmx_tsc_adjust_test
/x86_64/xapic_ipi_test
+/x86_64/xen_shinfo_test
+/x86_64/xen_vmcall_test
/x86_64/xss_msr_test
/x86_64/vmx_pmu_msrs_test
/demand_paging_test
--
2.30.0.478.g8a0d178c01-goog
The Xen shinfo selftest uses '40' when setting the GPA of the vCPU info
struct, but checks for the result at '0x40'. Arbitrarily use the hex
version to resolve the bug.
Fixes: 8d4e7e80838f ("KVM: x86: declare Xen HVM shared info capability and add test case")
Cc: David Woodhouse <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 cb3963957b3b..b2a3be9eba8e 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -102,7 +102,7 @@ int main(int argc, char *argv[])
struct kvm_xen_vcpu_attr vi = {
.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
- .u.gpa = SHINFO_REGION_GPA + 40,
+ .u.gpa = SHINFO_REGION_GPA + 0x40,
};
vcpu_ioctl(vm, VCPU_ID, KVM_XEN_VCPU_SET_ATTR, &vi);
--
2.30.0.478.g8a0d178c01-goog
Don't bother mapping the Xen shinfo pages into the guest, they don't need
to be accessed using the GVAs and passing a define with "GPA" in the name
to addr_gva2hpa() is confusing.
Cc: David Woodhouse <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
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 b2a3be9eba8e..9246ea310587 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -80,7 +80,6 @@ int main(int argc, char *argv[])
/* Map a region for the shared_info page */
vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
SHINFO_REGION_GPA, SHINFO_REGION_SLOT, 2, 0);
- virt_map(vm, SHINFO_REGION_GPA, SHINFO_REGION_GPA, 2, 0);
struct kvm_xen_hvm_config hvmc = {
.flags = KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL,
@@ -147,9 +146,9 @@ int main(int argc, char *argv[])
struct pvclock_wall_clock *wc;
struct pvclock_vcpu_time_info *ti, *ti2;
- wc = addr_gva2hva(vm, SHINFO_REGION_GPA + 0xc00);
- ti = addr_gva2hva(vm, SHINFO_REGION_GPA + 0x40 + 0x20);
- ti2 = addr_gva2hva(vm, PVTIME_ADDR);
+ wc = addr_gpa2hva(vm, SHINFO_REGION_GPA + 0xc00);
+ ti = addr_gpa2hva(vm, SHINFO_REGION_GPA + 0x40 + 0x20);
+ ti2 = addr_gpa2hva(vm, PVTIME_ADDR);
vm_ts.tv_sec = wc->sec;
vm_ts.tv_nsec = wc->nsec;
--
2.30.0.478.g8a0d178c01-goog
Add a 2 byte pad to struct compat_vcpu_info so that the sum size of its
fields is actually 64 bytes. The effective size without the padding is
also 64 bytes due to the compiler aligning evtchn_pending_sel to a 4-byte
boundary, but depending on compiler alignment is subtle and unnecessary.
Opportunistically replace spaces with tables in the other fields.
Cc: David Woodhouse <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/xen.h | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index 4b32489c0cec..b66a921776f4 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -49,11 +49,12 @@ struct compat_arch_vcpu_info {
};
struct compat_vcpu_info {
- uint8_t evtchn_upcall_pending;
- uint8_t evtchn_upcall_mask;
- uint32_t evtchn_pending_sel;
- struct compat_arch_vcpu_info arch;
- struct pvclock_vcpu_time_info time;
+ uint8_t evtchn_upcall_pending;
+ uint8_t evtchn_upcall_mask;
+ uint16_t pad;
+ uint32_t evtchn_pending_sel;
+ struct compat_arch_vcpu_info arch;
+ struct pvclock_vcpu_time_info time;
}; /* 64 bytes (x86) */
struct compat_arch_shared_info {
--
2.30.0.478.g8a0d178c01-goog
On 10/02/21 19:26, Sean Christopherson wrote:
> Fix a '40' vs '0x40' bug in the new Xen shinfo selftest, and clean up some
> other oddities that made root causing the problem far more painful than it
> needed to be.
>
> Note, Paolo already queued a patch from Vitaly that adds the tests to
> .gitignore[*], i.e. patch 01 can likely be dropped. I included it here
> for completeness.
>
> [*] https://lkml.kernel.org/r/[email protected]
>
> Sean Christopherson (5):
> KVM: selftests: Ignore recently added Xen tests' build output
> KVM: selftests: Fix size of memslots created by Xen tests
> KVM: selftests: Fix hex vs. decimal snafu in Xen test
> KVM: sefltests: Don't bother mapping GVA for Xen shinfo test
> KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes
>
> arch/x86/kvm/xen.h | 11 ++++++-----
> tools/testing/selftests/kvm/.gitignore | 2 ++
> tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 12 +++++-------
> tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c | 3 +--
> 4 files changed, 14 insertions(+), 14 deletions(-)
>
Stupid question: how did you notice that? In other words what broke for
you and not for me?
Paolo
On Wed, Feb 10, 2021, Paolo Bonzini wrote:
> On 10/02/21 19:26, Sean Christopherson wrote:
> > Fix a '40' vs '0x40' bug in the new Xen shinfo selftest, and clean up some
> > other oddities that made root causing the problem far more painful than it
> > needed to be.
> >
> > Note, Paolo already queued a patch from Vitaly that adds the tests to
> > .gitignore[*], i.e. patch 01 can likely be dropped. I included it here
> > for completeness.
> >
> > [*] https://lkml.kernel.org/r/[email protected]
> >
> > Sean Christopherson (5):
> > KVM: selftests: Ignore recently added Xen tests' build output
> > KVM: selftests: Fix size of memslots created by Xen tests
> > KVM: selftests: Fix hex vs. decimal snafu in Xen test
> > KVM: sefltests: Don't bother mapping GVA for Xen shinfo test
> > KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes
> >
> > arch/x86/kvm/xen.h | 11 ++++++-----
> > tools/testing/selftests/kvm/.gitignore | 2 ++
> > tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 12 +++++-------
> > tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c | 3 +--
> > 4 files changed, 14 insertions(+), 14 deletions(-)
> >
>
> Stupid question: how did you notice that? In other words what broke for you
> and not for me?
I assume sheer dumb luck? The test checks a single bit, so there's a 50/50
chance it will pass if whatever it's reading is non-zero.
If my math is correct (big if), the net effect is that the check was against
pvclock_vcpu_time_info.tsc_to_system_mul, which on my VM where I'm running the
test is always 0xcd4681c9.
==== Test Assertion Failure ====
x86_64/xen_shinfo_test.c:161: ti->version && !(ti->version & 1)
pid=1236 tid=1236 - Interrupted system call
1 0x0000000000401623: main at xen_shinfo_test.c:160
2 0x00007f303e261bf6: ?? ??:0
3 0x00000000004016f9: _start at ??:?
Bad time_info version cd4681c9
On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote:
> Add a 2 byte pad to struct compat_vcpu_info so that the sum size of its
> fields is actually 64 bytes. The effective size without the padding is
> also 64 bytes due to the compiler aligning evtchn_pending_sel to a 4-byte
> boundary, but depending on compiler alignment is subtle and unnecessary.
I think there's at least one BUILD_BUG_ON() which would have triggered
if the compiler ever did stop honouring the ELF ABI. And in fact in a
parallel universe where the ABI permits such packing, the padding would
be *wrong*, since the original Xen struct doesn't have the padding.
It *does* have an explicit uint8_t to replace evtchn_upcall_mask but it
doesn't have the following two bytes; canonically we *are* supposed to
take our chances with the ABI there. Although of course the relevant
ABI is the *32-bit* ABI in the compat case, not the 64-bit ABI. They
both align 32-bit values to 32 bits though.
uint8_t evtchn_upcall_pending;
#ifdef XEN_HAVE_PV_UPCALL_MASK
uint8_t evtchn_upcall_mask;
#else /* XEN_HAVE_PV_UPCALL_MASK */
uint8_t pad0;
#endif /* XEN_HAVE_PV_UPCALL_MASK */
xen_ulong_t evtchn_pending_sel;
struct arch_vcpu_info arch;
struct vcpu_time_info time;
}; /* 64 bytes (x86) */
So it isn't clear the additionally padding really buys us anything; if
we play this game without knowing the ABI we'd be screwed anyway. But
it doesn't hurt.
> Opportunistically replace spaces with tables in the other fields.
That part I certainly approve of.
Reviewed-by: David Woodhouse <[email protected]>
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote:
> Add the new Xen test binaries to KVM selftest's .gitnore.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
Reviewed-by: David Woodhouse <[email protected]>
On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote:
> The Xen shinfo selftest uses '40' when setting the GPA of the vCPU info
> struct, but checks for the result at '0x40'. Arbitrarily use the hex
> version to resolve the bug.
>
> Fixes: 8d4e7e80838f ("KVM: x86: declare Xen HVM shared info capability and add test case")
> Cc: David Woodhouse <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
Oops, sorry about that. I'm actually kind of impressed that that not
only worked for me every time I tested it, but also IIRC it actually
*failed* for me on all the occasions I expected it to fail.
Reviewed-by: David Woodhouse <[email protected]>
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote:
> Don't bother mapping the Xen shinfo pages into the guest, they don't need
> to be accessed using the GVAs and passing a define with "GPA" in the name
> to addr_gva2hpa() is confusing.
>
> Cc: David Woodhouse <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
Makes sense. We did actually use the mapping in an earlier iteration of
the test when it was still testing the runstate info, but I ripped that
out for the final merge because it needs more thought.
Reviewed-by: David Woodhouse <[email protected]>
> ---
> tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> 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 b2a3be9eba8e..9246ea310587 100644
> --- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> @@ -80,7 +80,6 @@ int main(int argc, char *argv[])
> /* Map a region for the shared_info page */
> vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> SHINFO_REGION_GPA, SHINFO_REGION_SLOT, 2, 0);
> - virt_map(vm, SHINFO_REGION_GPA, SHINFO_REGION_GPA, 2, 0);
>
> struct kvm_xen_hvm_config hvmc = {
> .flags = KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL,
> @@ -147,9 +146,9 @@ int main(int argc, char *argv[])
> struct pvclock_wall_clock *wc;
> struct pvclock_vcpu_time_info *ti, *ti2;
>
> - wc = addr_gva2hva(vm, SHINFO_REGION_GPA + 0xc00);
> - ti = addr_gva2hva(vm, SHINFO_REGION_GPA + 0x40 + 0x20);
> - ti2 = addr_gva2hva(vm, PVTIME_ADDR);
> + wc = addr_gpa2hva(vm, SHINFO_REGION_GPA + 0xc00);
> + ti = addr_gpa2hva(vm, SHINFO_REGION_GPA + 0x40 + 0x20);
> + ti2 = addr_gpa2hva(vm, PVTIME_ADDR);
>
> vm_ts.tv_sec = wc->sec;
> vm_ts.tv_nsec = wc->nsec;
On Wed, Feb 10, 2021, Woodhouse, David wrote:
> On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote:
> So it isn't clear the additionally padding really buys us anything; if
> we play this game without knowing the ABI we'd be screwed anyway. But
> it doesn't hurt.
Ya, this is purely for folks reading the code and wondering how 62==64.
On Wed, 2021-02-10 at 13:13 -0800, Sean Christopherson wrote:
> On Wed, Feb 10, 2021, Woodhouse, David wrote:
> > On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote:
> > So it isn't clear the additionally padding really buys us anything; if
> > we play this game without knowing the ABI we'd be screwed anyway. But
> > it doesn't hurt.
>
> Ya, this is purely for folks reading the code and wondering how 62==64.
Fair enough. That kind of thing is why I littered the code with
assertions based on sizeof() and offsetof() :)
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.