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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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