[kvm-unit-tests PATCH] x86/apic: Skip pv ipi test if hcall not available

[kvm-unit-tests PATCH] x86/apic: Skip pv ipi test if hcall not available

[Suraj Jitindar Singh]

From: Suraj Jitindar Singh <surajjs@amazon.com>

The test in x86/apic.c named test_pv_ipi is used to test for a kernel
bug where a guest making the hcall KVM_HC_SEND_IPI can trigger an out of
bounds access.

If the host doesn't implement this hcall then the out of bounds access
cannot be triggered.

Detect the case where the host doesn't implement the KVM_HC_SEND_IPI
hcall and skip the test when this is the case, as the test doesn't
apply.

Output without patch:
FAIL: PV IPIs testing

With patch:
SKIP: PV IPIs testing: h-call not available

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 x86/apic.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/x86/apic.c b/x86/apic.c
index eb785c4..bd44b54 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -8,6 +8,8 @@
 #include "atomic.h"
 #include "fwcfg.h"
 
+#include <linux/kvm_para.h>
+
 #define MAX_TPR			0xf
 
 static void test_lapic_existence(void)
@@ -638,6 +640,15 @@ static void test_pv_ipi(void)
     unsigned long a0 = 0xFFFFFFFF, a1 = 0, a2 = 0xFFFFFFFF, a3 = 0x0;
 
     asm volatile("vmcall" : "=a"(ret) :"a"(KVM_HC_SEND_IPI), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
+    /*
+     * Detect the case where the hcall is not implemented by the hypervisor and
+     * skip this test if this is the case. Is the hcall isn't implemented then
+     * the bug that this test is trying to catch can't be triggered.
+     */
+    if (ret == -KVM_ENOSYS) {
+	    report_skip("PV IPIs testing: h-call not available");
+	    return;
+    }
     report("PV IPIs testing", !ret);
 }
 
-- 
2.15.3.AMZN

Re: [kvm-unit-tests PATCH] x86/apic: Skip pv ipi test if hcall not available

[Vitaly Kuznetsov]

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> From: Suraj Jitindar Singh <surajjs@amazon.com>
>
> The test in x86/apic.c named test_pv_ipi is used to test for a kernel
> bug where a guest making the hcall KVM_HC_SEND_IPI can trigger an out of
> bounds access.
>
> If the host doesn't implement this hcall then the out of bounds access
> cannot be triggered.
>
> Detect the case where the host doesn't implement the KVM_HC_SEND_IPI
> hcall and skip the test when this is the case, as the test doesn't
> apply.
>
> Output without patch:
> FAIL: PV IPIs testing
>
> With patch:
> SKIP: PV IPIs testing: h-call not available
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  x86/apic.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/x86/apic.c b/x86/apic.c
> index eb785c4..bd44b54 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -8,6 +8,8 @@
>  #include "atomic.h"
>  #include "fwcfg.h"
>  
> +#include <linux/kvm_para.h>
> +
>  #define MAX_TPR			0xf
>  
>  static void test_lapic_existence(void)
> @@ -638,6 +640,15 @@ static void test_pv_ipi(void)
>      unsigned long a0 = 0xFFFFFFFF, a1 = 0, a2 = 0xFFFFFFFF, a3 = 0x0;
>  
>      asm volatile("vmcall" : "=a"(ret) :"a"(KVM_HC_SEND_IPI), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
> +    /*
> +     * Detect the case where the hcall is not implemented by the hypervisor and
> +     * skip this test if this is the case. Is the hcall isn't implemented then
> +     * the bug that this test is trying to catch can't be triggered.
> +     */
> +    if (ret == -KVM_ENOSYS) {
> +	    report_skip("PV IPIs testing: h-call not available");
> +	    return;
> +    }
>      report("PV IPIs testing", !ret);
>  }

Should we be checking CPUID bit (KVM_FEATURE_PV_SEND_IPI) instead?

-- 
Vitaly

Re: [kvm-unit-tests PATCH] x86/apic: Skip pv ipi test if hcall not available

[Suraj Jitindar Singh]

Hi,

On Fri, 2019-10-18 at 18:53 +0200, Vitaly Kuznetsov wrote:
> Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
> 
> > From: Suraj Jitindar Singh <surajjs@amazon.com>
> > 
> > The test in x86/apic.c named test_pv_ipi is used to test for a
> > kernel
> > bug where a guest making the hcall KVM_HC_SEND_IPI can trigger an
> > out of
> > bounds access.
> > 
> > If the host doesn't implement this hcall then the out of bounds
> > access
> > cannot be triggered.
> > 
> > Detect the case where the host doesn't implement the
> > KVM_HC_SEND_IPI
> > hcall and skip the test when this is the case, as the test doesn't
> > apply.
> > 
> > Output without patch:
> > FAIL: PV IPIs testing
> > 
> > With patch:
> > SKIP: PV IPIs testing: h-call not available
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  x86/apic.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/x86/apic.c b/x86/apic.c
> > index eb785c4..bd44b54 100644
> > --- a/x86/apic.c
> > +++ b/x86/apic.c
> > @@ -8,6 +8,8 @@
> >  #include "atomic.h"
> >  #include "fwcfg.h"
> >  
> > +#include <linux/kvm_para.h>
> > +
> >  #define MAX_TPR			0xf
> >  
> >  static void test_lapic_existence(void)
> > @@ -638,6 +640,15 @@ static void test_pv_ipi(void)
> >      unsigned long a0 = 0xFFFFFFFF, a1 = 0, a2 = 0xFFFFFFFF, a3 =
> > 0x0;
> >  
> >      asm volatile("vmcall" : "=a"(ret) :"a"(KVM_HC_SEND_IPI),
> > "b"(a0), "c"(a1), "d"(a2), "S"(a3));
> > +    /*
> > +     * Detect the case where the hcall is not implemented by the
> > hypervisor and
> > +     * skip this test if this is the case. Is the hcall isn't
> > implemented then
> > +     * the bug that this test is trying to catch can't be
> > triggered.
> > +     */
> > +    if (ret == -KVM_ENOSYS) {
> > +	    report_skip("PV IPIs testing: h-call not available");
> > +	    return;
> > +    }
> >      report("PV IPIs testing", !ret);
> >  }
> 
> Should we be checking CPUID bit (KVM_FEATURE_PV_SEND_IPI) instead?
> 

That's also an option. It will produce the same result.

Would that be the preferred approach or is the method used in the
current patch ok?

Re: [kvm-unit-tests PATCH] x86/apic: Skip pv ipi test if hcall not available

[Vitaly Kuznetsov]

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> Hi,
>
> On Fri, 2019-10-18 at 18:53 +0200, Vitaly Kuznetsov wrote:
>> Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
>> 
>> > From: Suraj Jitindar Singh <surajjs@amazon.com>
>> > 
>> > The test in x86/apic.c named test_pv_ipi is used to test for a
>> > kernel
>> > bug where a guest making the hcall KVM_HC_SEND_IPI can trigger an
>> > out of
>> > bounds access.
>> > 
>> > If the host doesn't implement this hcall then the out of bounds
>> > access
>> > cannot be triggered.
>> > 
>> > Detect the case where the host doesn't implement the
>> > KVM_HC_SEND_IPI
>> > hcall and skip the test when this is the case, as the test doesn't
>> > apply.
>> > 
>> > Output without patch:
>> > FAIL: PV IPIs testing
>> > 
>> > With patch:
>> > SKIP: PV IPIs testing: h-call not available
>> > 
>> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>> > ---
>> >  x86/apic.c | 11 +++++++++++
>> >  1 file changed, 11 insertions(+)
>> > 
>> > diff --git a/x86/apic.c b/x86/apic.c
>> > index eb785c4..bd44b54 100644
>> > --- a/x86/apic.c
>> > +++ b/x86/apic.c
>> > @@ -8,6 +8,8 @@
>> >  #include "atomic.h"
>> >  #include "fwcfg.h"
>> >  
>> > +#include <linux/kvm_para.h>
>> > +
>> >  #define MAX_TPR			0xf
>> >  
>> >  static void test_lapic_existence(void)
>> > @@ -638,6 +640,15 @@ static void test_pv_ipi(void)
>> >      unsigned long a0 = 0xFFFFFFFF, a1 = 0, a2 = 0xFFFFFFFF, a3 =
>> > 0x0;
>> >  
>> >      asm volatile("vmcall" : "=a"(ret) :"a"(KVM_HC_SEND_IPI),
>> > "b"(a0), "c"(a1), "d"(a2), "S"(a3));
>> > +    /*
>> > +     * Detect the case where the hcall is not implemented by the
>> > hypervisor and
>> > +     * skip this test if this is the case. Is the hcall isn't
>> > implemented then
>> > +     * the bug that this test is trying to catch can't be
>> > triggered.
>> > +     */
>> > +    if (ret == -KVM_ENOSYS) {
>> > +	    report_skip("PV IPIs testing: h-call not available");
>> > +	    return;
>> > +    }
>> >      report("PV IPIs testing", !ret);
>> >  }
>> 
>> Should we be checking CPUID bit (KVM_FEATURE_PV_SEND_IPI) instead?
>> 
>
> That's also an option. It will produce the same result.
>

Generally yes, but CPUID announcement has its own advantages: when a
feature bit is set we know the hypercall *must* exist so -KVM_ENOSYS
would be a bug (think of a theoretical situation when the hypercall
starts to return -KVM_ENOSYS erroneously - how do we catch this?)

> Would that be the preferred approach or is the method used in the
> current patch ok?

I'm not insisting, let's leave it up to Paolo :-)

-- 
Vitaly

Re: [kvm-unit-tests PATCH] x86/apic: Skip pv ipi test if hcall not available

[Wanpeng Li]

On Tue, 22 Oct 2019 at 06:51, Suraj Jitindar Singh
<sjitindarsingh@gmail.com> wrote:
>
> Hi,
>
> On Fri, 2019-10-18 at 18:53 +0200, Vitaly Kuznetsov wrote:
> > Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
> >
> > > From: Suraj Jitindar Singh <surajjs@amazon.com>
> > >
> > > The test in x86/apic.c named test_pv_ipi is used to test for a
> > > kernel
> > > bug where a guest making the hcall KVM_HC_SEND_IPI can trigger an
> > > out of
> > > bounds access.
> > >
> > > If the host doesn't implement this hcall then the out of bounds
> > > access
> > > cannot be triggered.
> > >
> > > Detect the case where the host doesn't implement the
> > > KVM_HC_SEND_IPI
> > > hcall and skip the test when this is the case, as the test doesn't
> > > apply.
> > >
> > > Output without patch:
> > > FAIL: PV IPIs testing
> > >
> > > With patch:
> > > SKIP: PV IPIs testing: h-call not available
> > >
> > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > > ---
> > >  x86/apic.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/x86/apic.c b/x86/apic.c
> > > index eb785c4..bd44b54 100644
> > > --- a/x86/apic.c
> > > +++ b/x86/apic.c
> > > @@ -8,6 +8,8 @@
> > >  #include "atomic.h"
> > >  #include "fwcfg.h"
> > >
> > > +#include <linux/kvm_para.h>
> > > +
> > >  #define MAX_TPR                    0xf
> > >
> > >  static void test_lapic_existence(void)
> > > @@ -638,6 +640,15 @@ static void test_pv_ipi(void)
> > >      unsigned long a0 = 0xFFFFFFFF, a1 = 0, a2 = 0xFFFFFFFF, a3 =
> > > 0x0;
> > >
> > >      asm volatile("vmcall" : "=a"(ret) :"a"(KVM_HC_SEND_IPI),
> > > "b"(a0), "c"(a1), "d"(a2), "S"(a3));
> > > +    /*
> > > +     * Detect the case where the hcall is not implemented by the
> > > hypervisor and
> > > +     * skip this test if this is the case. Is the hcall isn't
> > > implemented then
> > > +     * the bug that this test is trying to catch can't be
> > > triggered.
> > > +     */
> > > +    if (ret == -KVM_ENOSYS) {
> > > +       report_skip("PV IPIs testing: h-call not available");
> > > +       return;
> > > +    }
> > >      report("PV IPIs testing", !ret);
> > >  }
> >
> > Should we be checking CPUID bit (KVM_FEATURE_PV_SEND_IPI) instead?
> >
>
> That's also an option. It will produce the same result.
>
> Would that be the preferred approach or is the method used in the
> current patch ok?

Btw, is it amazon using pv ipis? I don't think. I suspect there is an
extra hardware assistant to benefit broadcast ipis in amazon.

    Wanpeng