Re: [Xen-devel] [PATCH XTF v2] Functional: Add a UMIP test

[Boqun Feng <boqun.feng@xxxxxxxxx>]

On Mon, Aug 14, 2017 at 11:35:47AM +0100, Andrew Cooper wrote:
> On 14/08/17 06:08, Boqun Feng (Intel) wrote:
> > Add a "umip" test for the User-Model Instruction Prevention. The test
> > simply tries to run sgdt/sidt/sldt/str/smsw in guest user-mode with
> > CR4_UMIP = 1.
> >
> > Signed-off-by: Boqun Feng (Intel) <boqun.feng@xxxxxxxxx>
> 
> Thankyou for this.  Its looking much better.  Just a few comments.
> 

Apologies for being so late, got interrupted by something more urgent..

> > ---
> > v1 --> v2:
> >     * add a new write_cr4_safe()
> >     * use %pe for exception print
> >     * refactor the code based on Andrew's guide and advice
> >
> > Test results:
> >
> > * With UMIP patch:
> > ** boot with hvm_fep: SUCCESS
> > ** boot without hvm_fep: SKIP, due to "FEP support not detected.."
> >
> > * Without UMIP patch:
> > ** boot with hvm_fep: SKIP, due to "UMIP is not supported.."
> > ** boot without hvm_fep: SKIP, due to "UMIP is not supported.."
> >
> > * With UMIP cpuid exposed but CR4 invalid:
> > ** boot with hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
> > ** boot without hvm_fep: FAILURE, due to "Fail: Unable to activate UMIP.."
> 
> What do you mean by CR4 invalid here?
> 

I mean hvm_cr4_guest_valid_bits() doesn't return with X86_CR4_UMIP set,
and I manually modified my "Expose UMIP" patch to test this.

> > diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h
> > index f608af9996f0..4f0d85290cf0 100644
> > --- a/arch/x86/include/arch/lib.h
> > +++ b/arch/x86/include/arch/lib.h
> > @@ -340,6 +340,19 @@ static inline void write_cr4(unsigned long cr4)
> >      asm volatile ("mov %0, %%cr4" :: "r" (cr4));
> >  }
> >  
> > +static inline bool write_cr4_safe(unsigned long cr4)
> > +{
> > +    exinfo_t fault = 0;
> > +
> > +    asm volatile ("1: mov %1, %%cr4; 2:"
> > +                  _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_edi)
> > +                  : "+D" (fault)
> > +                  : "r" (cr4),
> > +                    "X" (ex_record_fault_edi));
> > +
> > +    return !fault;
> 
> To match the existing {rd,wr}msr_safe() implementation in XTF and the
> common usage in Linux and Xen, the return value should be 0 for success
> and nonzero for fault.
> 
> i.e. you should "return fault;"
> 

Got it.

> > diff --git a/tests/umip/main.c b/tests/umip/main.c
> > new file mode 100644
> > index 000000000000..fcaba4e34570
> > --- /dev/null
> > +++ b/tests/umip/main.c
> >
> > +
> > +static const struct stub {
> > +    unsigned long (*fn)(unsigned long);
> > +    const char *name;
> > +} stubs[] = {
> > +    { stub_sgdt, "SGDT" },
> > +    { stub_sidt, "SIDT" },
> > +    { stub_sldt, "SLDT" },
> > +    { stub_str,  "STR" },
> > +    { stub_smsw, "SMSW" },
> > +};
> > +
> > +void test_umip(bool umip_active, bool force)
> 
> (For reasons explained below,) I'd rename this to test_insns(), and...
> 
> > +{
> > +    unsigned int i;
> > +    bool user;
> > +
> > +    for ( user = false; ; user = true )
> > +    {
> > +        exinfo_t exp = user && umip_active ? EXINFO_SYM(GP, 0) : 0;
> > +
> > +        for ( i = 0; i < ARRAY_SIZE(stubs); i++)
> > +        {
> > +            const struct stub *s = &stubs[i];
> > +            exinfo_t ret;
> > +
> > +            ret = user ? exec_user_param(s->fn, force) : s->fn(force);
> > +
> > +            /*
> > +             * Tolerate the instruction emulator not understanding these
> > +             * instructions in older releases of Xen.
> > +             */
> > +
> > +            if ( force && ret == EXINFO_SYM(UD, 0) )
> > +            {
> > +                static bool once;
> > +
> > +                if ( !once )
> > +                {
> > +                    xtf_skip("Skip: Emulator doesn't implement %s\n", 
> > s->name);
> > +                    once = true;
> > +                }
> > +
> > +                continue;
> > +            }
> > +
> > +            if ( ret != exp )
> > +                xtf_failure("Fail: %s %s\n"
> > +                            "  expected %pe\n"
> > +                            "       got %pe\n",
> > +                            user ? "user" : "supervisor", s->name,
> > +                            _p(exp), _p(ret));
> > +        }
> > +
> > +        if ( user )
> > +            break;
> > +    }
> > +}
> 
> ... have this helper, which simplifies the test_main() logic.
> 
> static void test_umip(bool umip_active)
> {
>     test_insns(umip_active, false);
> 
>     if ( xtf_has_fep )
>         test_insns(umip_active, true);
> }
> 

Good point, will do this in V3.

> > +
> > +void test_main(void)
> > +{
> > +    unsigned long cr4;
> > +
> > +    /* run with UMIP inactive */
> > +    test_umip(false, false);
> > +
> > +    if ( !xtf_has_fep )
> > +        xtf_skip("FEP support not detected - some tests will be 
> > skipped\n");
> 
> This text only needs to be shown once.  I'd move it ahead of the first
> test_umip() call, and you can drop the else clauses given the new
> test_umip() implementation.
> 

Agreed.

> > +    else
> > +        test_umip(false, true);
> > +
> > +    if ( !cpu_has_umip )
> > +    {
> > +        xtf_skip("UMIP is not supported, skip the rest of test\n");
> 
> Since I pointed you at the cpuid-faulting test case, I've had my code
> reviewed by someone in our testing team, and a logical issue was found. 
> (As it turns out, when CPUID faulting is not available, writes
> attempting to enable it are squashed and appear to have succeeded.  I'll
> fix that bug in due course.)
> 
> At this point, we should check that attempting to set CR4.UMIP is
> prohibited.
> 

Ok, I will add something like:

        if ( !cpu_has_umip )
        {
             if (!write_cr4_safe(cr4 | X86_CR4_UMIP))
                 xtf_fail("UMIP unsupported, but setting CR4 bit succeeds");
             else
                 xtf_skip("UMIP is not supported, skip the rest of test\n");
        }

Thoughts?

Regards,
Boqun

> Otherwise, everything else looks fine.
> 
> ~Andrew

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel