| Message ID | 20191017235036.25624-1-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86/apic: Skip pv ipi test if hcall not available | expand |
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?
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?
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 :-)
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
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); }