Re: [Xen-devel] [PATCH] EPT: utilize GLA->GPA translation known for certain faults

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.08.14 at 21:20, <tim@xxxxxxx> wrote:
> At 15:40 +0100 on 28 Aug (1409236857), Jan Beulich wrote:
>> Rather than doing the translation ourselves in __hvmemul_{read,write}()
>> leverage that we know the association for faults other than such having
>> occurred when translating addresses of page tables.
>> 
>> There is one intentional but not necessarily obvious (and possibly
>> subtle) adjustment to behavior: __hvmemul_read() no longer blindly
>> bails on instruction fetches matching the MMIO GVA (the callers of
>> handle_mmio_with_translation() now control the behavior via the struct
>> npfec they pass, and it didn't seem right to bail here rather than just
>> falling through to the unaccelerated path)
> 
> Understood.  I think the test you have is not quite right:
> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -481,10 +481,11 @@ static int __hvmemul_read(
>>          while ( off & (chunk - 1) )
>>              chunk >>= 1;
>>  
>> -    if ( unlikely(vio->mmio_gva == (addr & PAGE_MASK)) && vio->mmio_gva )
>> +    if ( ((access_type != hvm_access_insn_fetch
>> +           ? vio->mmio_access.read_access
>> +           : vio->mmio_access.insn_fetch)) &&
>> +         (vio->mmio_gva == (addr & PAGE_MASK)) )
> 
> This seems like it will follow the fast path for some instruction
> fetches, which might be OK, but doesn't quite match your description.

To me it does.

> If that's the behaviour you want then I think we can simplify the test
> a lot because we don't care whether this is an ifetch or not; only
> whether the mapping is valid (assuming we model a coherent
> icache/dcache and itlb/dtlb here).

Perhaps, but I wanted to be as restrictive as possible (and the hit
rate of the accelerated case of over 99.9% tells me that this extra
[possibly paranoid] care doesn't really hurt). Otherwise I can't see
why we couldn't drop the read_access check too, and similarly the
write_access one in the write path.

> But IIRC we don't really support fetching instructions from MMIO
> addresses anyway (raising questions about, e.g. operation sizes,
> idempotence and readahead) so we probably want:
> 
>        if ( access_type != hvm_access_insn_fetch &&
>             vio->mmio_access.read_access &&
>             vio->mmio_gva == (addr & PAGE_MASK) )

Getting us back to the previous behavior. While fetching
instructions from MMIO is suspicious in some cases, I think whoever
does so has to be prepared for side effects to potentially occur
more than once. My perspective is more that on side effect free
memory (e.g. VRAM), fetching instructions is quite fine.

Jan


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