Re: [PATCH for-4.7 3/4] x86/hvm: Correct the emulated interaction of invlpg with segments

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/05/16 14:42, Jan Beulich wrote:
>>>> On 09.05.16 at 15:15, <andrew.cooper3@citrix.com> wrote:
>> The `invlpg` instruction is documented to take a memory address, and is not
>> documented to suffer faults from segmentation violations.
>>
>> Experimentally, and subsequently confirmed by both Intel and AMD, the
>> instruction does take into account segment bases, but will happily invalidate
>> a TLB entry for a mapping beyond the segment limit.
> How about non-canonical addresses? If those don't #GP (which is
> what I assume), is such an INVLPG a NOP

They are explicitly documented by both Intel and AMD as being a NOP when
passed a non-canonical address.  Experimentation confirms this.

(I am just putting the finishing touches on an XTF test for all of this).

> , or does it invalidate
> something (e.g. the translation for the address truncated to 48
> bits)? In that latter case we might need to also make our code
> behave that way...
>
>> The emulation logic will currently raise #GP/#SS faults for segment limit
>> violations, or non-canonical addresses, which doesn't match hardware's
>> behaviour.  Instead, squash exceptions generated by
>> hvmemul_virtual_to_linear() and proceed with invalidation.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> albeit I think ...
>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1608,7 +1608,22 @@ static int hvmemul_invlpg(
>>      rc = hvmemul_virtual_to_linear(
>>          seg, offset, 1, &reps, hvm_access_none, hvmemul_ctxt, &addr);
>>  
>> -    if ( rc == X86EMUL_OKAY )
>> +    if ( rc == X86EMUL_EXCEPTION )
>> +    {
>> +        /*
>> +         * `invlpg` takes segment bases into account, but is not subject to
>> +         * faults from segment type/limit checks, and is specified as a NOP
>> +         * when issued on non-canonical addresses.
>> +         *
>> +         * hvmemul_virtual_to_linear() raises exceptions for type/limit
>> +         * violations, so squash them.
>> +         */
>> +        hvmemul_ctxt->exn_pending = 0;
>> +        hvmemul_ctxt->trap = (struct hvm_trap){};
> ... this does more work than is really needed.

For sanity sake, I would prefer not to leave stale information in the
emulation context.  This path is definitely not a hotpath.

~Andrew

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