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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/08/17 13:43, Boqun Feng wrote:
> 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..

No worries.  I get exactly the same kind of interruptions as well.

>
>>> ---
>>> 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.

Ah ok.

>>> +    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?

I'd personally go with this, because I think it is slightly clearer
logic to follow.

if ( !cpu_has_umip )
{
    xtf_skip("UMIP is not supported, skip the rest of test\n");

    if ( !write_cr4_safe(cr4 | X86_CR4_UMIP) )
        xtf_fail("UMIP unsupported, but setting CR4 bit succeeded\n");
}

~Andrew

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