[PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup

[PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup

[Sean Christopherson]

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(-)

-- 
2.30.0.478.g8a0d178c01-goog

[PATCH 1/5] KVM: selftests: Ignore recently added Xen tests' build output

[Sean Christopherson]

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

[PATCH 2/5] KVM: selftests: Fix size of memslots created by Xen tests

[Sean Christopherson]

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

[PATCH 3/5] KVM: selftests: Fix hex vs. decimal snafu in Xen test

[Sean Christopherson]

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

[PATCH 4/5] KVM: sefltests: Don't bother mapping GVA for Xen shinfo test

[Sean Christopherson]

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

[PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes

[Sean Christopherson]

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

Re: [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup

[Paolo Bonzini]

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

Re: [PATCH 0/5] KVM: x86/xen: Selftest fixes and a cleanup

[Sean Christopherson]

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

Re: [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes

[Woodhouse, David]

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.

Re: [EXTERNAL] [PATCH 1/5] KVM: selftests: Ignore recently added Xen tests' build output

[David Woodhouse]

[-- 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 --]

Re: [EXTERNAL] [PATCH 2/5] KVM: selftests: Fix size of memslots created by Xen tests

[David Woodhouse]

[-- 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 --]

Re: [PATCH 3/5] KVM: selftests: Fix hex vs. decimal snafu in Xen test

[Woodhouse, David]

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.

Re: [EXTERNAL] [PATCH 4/5] KVM: sefltests: Don't bother mapping GVA for Xen shinfo test

[David Woodhouse]

[-- 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 --]

Re: [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes

[Sean Christopherson]

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.

Re: [PATCH 5/5] KVM: x86/xen: Explicitly pad struct compat_vcpu_info to 64 bytes

[Woodhouse, David]

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.