* [PATCH 1/5] KVM: selftests: Ignore recently added Xen tests' build output
2021-02-10 18:26 [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Sean Christopherson
@ 2021-02-10 18:26 ` Sean Christopherson
2021-02-10 20:52 ` [EXTERNAL] " David Woodhouse
2021-02-10 18:26 ` [PATCH 2/5] KVM: selftests: Fix size of memslots created by Xen tests Sean Christopherson
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-02-10 18:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, David Woodhouse
Add the new Xen test binaries to KVM selftest's .gitnore.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [EXTERNAL] [PATCH 1/5] KVM: selftests: Ignore recently added Xen tests' build output
2021-02-10 18:26 ` [PATCH 1/5] KVM: selftests: Ignore recently added Xen tests' build output Sean Christopherson
@ 2021-02-10 20:52 ` David Woodhouse
0 siblings, 0 replies; 15+ messages in thread
From: David Woodhouse @ 2021-02-10 20:52 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 249 bytes --]
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 <seanjc@google.com>
> ---
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] KVM: selftests: Fix size of memslots created by Xen tests
2021-02-10 18:26 [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Sean Christopherson
2021-02-10 18:26 ` [PATCH 1/5] KVM: selftests: Ignore recently added Xen tests' build output Sean Christopherson
@ 2021-02-10 18:26 ` Sean Christopherson
2021-02-10 20:53 ` [EXTERNAL] " David Woodhouse
2021-02-10 18:26 ` [PATCH 3/5] KVM: selftests: Fix hex vs. decimal snafu in Xen test Sean Christopherson
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-02-10 18:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, David Woodhouse
For better or worse, the memslot APIs take the number of pages, not the
size in bytes. The Xen tests need 2 pages, not 8192 pages.
Fixes: 8d4e7e80838f ("KVM: x86: declare Xen HVM shared info capability and add test case")
Cc: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 3 +--
tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c | 3 +--
2 files changed, 2 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 bdb3feb86b5b..cb3963957b3b 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -79,8 +79,7 @@ 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 * getpagesize(), 0);
+ 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 = {
diff --git a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
index 86653361c695..8389e0bfd711 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_vmcall_test.c
@@ -102,8 +102,7 @@ int main(int argc, char *argv[])
/* Map a region for the hypercall pages */
vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
- HCALL_REGION_GPA, HCALL_REGION_SLOT,
- 2 * getpagesize(), 0);
+ HCALL_REGION_GPA, HCALL_REGION_SLOT, 2, 0);
virt_map(vm, HCALL_REGION_GPA, HCALL_REGION_GPA, 2, 0);
for (;;) {
--
2.30.0.478.g8a0d178c01-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [EXTERNAL] [PATCH 2/5] KVM: selftests: Fix size of memslots created by Xen tests
2021-02-10 18:26 ` [PATCH 2/5] KVM: selftests: Fix size of memslots created by Xen tests Sean Christopherson
@ 2021-02-10 20:53 ` David Woodhouse
0 siblings, 0 replies; 15+ messages in thread
From: David Woodhouse @ 2021-02-10 20:53 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 454 bytes --]
On Wed, 2021-02-10 at 10:26 -0800, Sean Christopherson wrote:
> For better or worse, the memslot APIs take the number of pages, not the
> size in bytes. The Xen tests need 2 pages, not 8192 pages.
>
> Fixes: 8d4e7e80838f ("KVM: x86: declare Xen HVM shared info capability and add test case")
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/5] KVM: selftests: Fix hex vs. decimal snafu in Xen test
2021-02-10 18:26 [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Sean Christopherson
2021-02-10 18:26 ` [PATCH 1/5] KVM: selftests: Ignore recently added Xen tests' build output Sean Christopherson
2021-02-10 18:26 ` [PATCH 2/5] KVM: selftests: Fix size of memslots created by Xen tests Sean Christopherson
@ 2021-02-10 18:26 ` Sean Christopherson
2021-02-10 20:54 ` Woodhouse, David
2021-02-10 18:26 ` [PATCH 4/5] KVM: sefltests: Don't bother mapping GVA for Xen shinfo test Sean Christopherson
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-02-10 18:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, David Woodhouse
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 <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/5] KVM: selftests: Fix hex vs. decimal snafu in Xen test
2021-02-10 18:26 ` [PATCH 3/5] KVM: selftests: Fix hex vs. decimal snafu in Xen test Sean Christopherson
@ 2021-02-10 20:54 ` Woodhouse, David
0 siblings, 0 replies; 15+ messages in thread
From: Woodhouse, David @ 2021-02-10 20:54 UTC (permalink / raw)
To: seanjc, pbonzini; +Cc: jmattson, joro, kvm, vkuznets, linux-kernel, wanpengli
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 <dwmw@amazon.co.uk>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
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 <dwmw@amazon.co.uk>
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] KVM: sefltests: Don't bother mapping GVA for Xen shinfo test
2021-02-10 18:26 [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Sean Christopherson
` (2 preceding siblings ...)
2021-02-10 18:26 ` [PATCH 3/5] KVM: selftests: Fix hex vs. decimal snafu in Xen test Sean Christopherson
@ 2021-02-10 18:26 ` Sean Christopherson
2021-02-10 21:04 ` [EXTERNAL] " David Woodhouse
2021-02-10 18:26 ` [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes Sean Christopherson
2021-02-10 18:37 ` [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Paolo Bonzini
5 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-02-10 18:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, David Woodhouse
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 <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [EXTERNAL] [PATCH 4/5] KVM: sefltests: Don't bother mapping GVA for Xen shinfo test
2021-02-10 18:26 ` [PATCH 4/5] KVM: sefltests: Don't bother mapping GVA for Xen shinfo test Sean Christopherson
@ 2021-02-10 21:04 ` David Woodhouse
0 siblings, 0 replies; 15+ messages in thread
From: David Woodhouse @ 2021-02-10 21:04 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]
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 <dwmw@amazon.co.uk>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
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 <dwmw@amazon.co.uk>
> ---
> 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;
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes
2021-02-10 18:26 [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Sean Christopherson
` (3 preceding siblings ...)
2021-02-10 18:26 ` [PATCH 4/5] KVM: sefltests: Don't bother mapping GVA for Xen shinfo test Sean Christopherson
@ 2021-02-10 18:26 ` Sean Christopherson
2021-02-10 20:51 ` Woodhouse, David
2021-02-10 18:37 ` [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Paolo Bonzini
5 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-02-10 18:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, David Woodhouse
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 <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes
2021-02-10 18:26 ` [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes Sean Christopherson
@ 2021-02-10 20:51 ` Woodhouse, David
2021-02-10 21:13 ` Sean Christopherson
0 siblings, 1 reply; 15+ messages in thread
From: Woodhouse, David @ 2021-02-10 20:51 UTC (permalink / raw)
To: seanjc, pbonzini; +Cc: jmattson, joro, kvm, vkuznets, linux-kernel, wanpengli
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 <dwmw@amazon.co.uk>
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes
2021-02-10 20:51 ` Woodhouse, David
@ 2021-02-10 21:13 ` Sean Christopherson
2021-02-10 21:20 ` Woodhouse, David
0 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2021-02-10 21:13 UTC (permalink / raw)
To: Woodhouse, David
Cc: pbonzini, jmattson, joro, kvm, vkuznets, linux-kernel, wanpengli
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes
2021-02-10 21:13 ` Sean Christopherson
@ 2021-02-10 21:20 ` Woodhouse, David
0 siblings, 0 replies; 15+ messages in thread
From: Woodhouse, David @ 2021-02-10 21:20 UTC (permalink / raw)
To: seanjc; +Cc: jmattson, joro, kvm, vkuznets, pbonzini, wanpengli, linux-kernel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup
2021-02-10 18:26 [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Sean Christopherson
` (4 preceding siblings ...)
2021-02-10 18:26 ` [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes Sean Christopherson
@ 2021-02-10 18:37 ` Paolo Bonzini
2021-02-10 18:49 ` Sean Christopherson
5 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-02-10 18:37 UTC (permalink / raw)
To: Sean Christopherson
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel, David Woodhouse
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/20210129161821.74635-1-vkuznets@redhat.com
>
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup
2021-02-10 18:37 ` [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup Paolo Bonzini
@ 2021-02-10 18:49 ` Sean Christopherson
0 siblings, 0 replies; 15+ messages in thread
From: Sean Christopherson @ 2021-02-10 18:49 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel, David Woodhouse
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/20210129161821.74635-1-vkuznets@redhat.com
> >
> > 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
^ permalink raw reply [flat|nested] 15+ messages in thread