Suraj Jitindar Singh <sjitindarsingh@xxxxxxxxx> writes:
> Hi,
>
> On Fri, 2019-10-18 at 18:53 +0200, Vitaly Kuznetsov wrote:
>> Suraj Jitindar Singh <sjitindarsingh@xxxxxxxxx> writes:
>>
>> > From: Suraj Jitindar Singh <surajjs@xxxxxxxxxx>
>> >
>> > 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@xxxxxxxxx>
>> > ---
>> > 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
