Re: [Xen-devel] [PATCH v11] x86/emulate: Send vm_event from emulate

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 19.09.2019 16:59, Jan Beulich wrote:
> On 19.09.2019 15:03, Alexandru Stefan ISAILA wrote:
>> @@ -601,6 +602,7 @@ static void *hvmemul_map_linear_addr(
>>   
>>           case HVMTRANS_gfn_paged_out:
>>           case HVMTRANS_gfn_shared:
>> +        case HVMTRANS_bad_gfn_access:
>>               err = ERR_PTR(~X86EMUL_RETRY);
>>               goto out;
> 
> This looks pretty suspicious now - why would (without knowing all
> the background) "bad access" translate into "retry". While you did
> post the suggested name before, it's nevertheless pretty clear now
> that it needs changing. Perhaps HVMTRANS_need_retry or some such?

It's fine by me, I will change the name to HVMTRANS_need_retry.

> 
>> @@ -1852,6 +1864,8 @@ static int hvmemul_rep_movs(
>>   
>>       xfree(buf);
>>   
>> +    ASSERT(rc != HVMTRANS_bad_gfn_access);
>> +
>>       if ( rc == HVMTRANS_gfn_paged_out )
>>           return X86EMUL_RETRY;
>>       if ( rc == HVMTRANS_gfn_shared )
>> @@ -1964,6 +1978,8 @@ static int hvmemul_rep_stos(
>>           if ( buf != p_data )
>>               xfree(buf);
>>   
>> +        ASSERT(rc != HVMTRANS_bad_gfn_access);
>> +
>>           switch ( rc )
>>           {
>>           case HVMTRANS_gfn_paged_out:
> 
> These are changes to places that were pointed out before do consume
> HVMTRANS_* return values. Did you go through and check nothing else
> needs adjustment? You don't say anything in this regard in the
> description. For example, if shadow's hvm_read() would get to see
> the new value, it would fall out of its switch() into a BUG().
> 

Yes, you are right, the only thing that saves shadow from not raising a 
BUG is the send_event flag. For safety reasons I will have a complete 
check of all the places that can fail from adding the new return value.

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3236,6 +3236,19 @@ static enum hvm_translation_result __hvm_copy(
>>               return HVMTRANS_bad_gfn_to_mfn;
>>           }
>>   
>> +        /*
>> +         * In case a vm event was sent return paged_out so the emulation 
>> will
>> +         * stop with no side effect
>> +         */
>> +        if ( (flags & HVMCOPY_linear) &&
>> +             unlikely(v->arch.vm_event) &&
>> +             v->arch.vm_event->send_event &&
>> +             hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) )
> 
> In such a sequence of checks with _some_ part using unlikely() I
> think it would be better to have the unlikely() one first (unless
> it's a relatively expensive check, which isn't the case here), to
> have as little as possible unnecessary computations / branches in
> the common (fast path) case.

I will change the order in the next version.

> 
> Furthermore while you now restrict the check to linear address
> based accesses, other than the description says (or at least
> implies) you do not restrict it to read and exec accesses. It's
> not clear to me whether that's intentional, yet it affects which
> hvm_copy_*_linear() callers need auditing.

The pfec var is checked in hvm_monitor_check_p2m(). If you think it is 
necessary I can add one more check here for (pfec & (PFEC_insn_fetch | 
PFEC_write_access)).

> 
> Finally, what about ->arch.vm_event->send_event remaining set
> after hvm_emulate_one_vm_event(), because hvm_monitor_check_p2m()
> (the only place where the flag would get cleared) was never hit
> in the process?
Thanks for pointing this out, indeed it's a problem here. A solution can 
be to move send_event = false; after hvm_emulate_one_vm_event() is 
finished. And state in the comment that the user is in charge of 
enabling and disabling the flag.
Or just have it in both places.

> And what about an instruction accessing two (or
> more) distinct addresses? The flag would be clear after the first
> one was checked afaict.

There is no problem here because emulation will stop after the first bad 
access so there is no need to continue.

Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel