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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -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().

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

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.

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

Jan

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