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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.09.2019 14:16, Alexandru Stefan ISAILA wrote:
> In order to have __hvm_copy() issue ~X86EMUL_RETRY a new return type,
> HVMTRANS_need_retry, was added and all the places that consume HVMTRANS*
> and needed adjustment where changed accordingly.

This is wrong and hence confusing: __hvm_copy() would never return
~X86EMUL_RETRY. In fact I think you've confused yourself enough to
make a questionable (possibly resulting) change:

> @@ -582,7 +583,7 @@ static void *hvmemul_map_linear_addr(
>          ASSERT(mfn_x(*mfn) == 0);
>  
>          res = hvm_translate_get_page(curr, addr, true, pfec,
> -                                     &pfinfo, &page, NULL, &p2mt);
> +                                     &pfinfo, &page, &gfn, &p2mt);

This function ...

>          switch ( res )
>          {
> @@ -601,6 +602,7 @@ static void *hvmemul_map_linear_addr(
>  
>          case HVMTRANS_gfn_paged_out:
>          case HVMTRANS_gfn_shared:
> +        case HVMTRANS_need_retry:

... can't return this value, so you should omit this addition,
letting the new return value go through "default:".

> @@ -1852,6 +1864,8 @@ static int hvmemul_rep_movs(
>  
>      xfree(buf);
>  
> +    ASSERT(rc != HVMTRANS_need_retry);
> +
>      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_need_retry);
> +
>          switch ( rc )
>          {
>          case HVMTRANS_gfn_paged_out:

Looking at this again, I think it would better be an addition to
the switch() (using ASSERT_UNREACHABLE()). Generally this is
true for the rep_movs case as well, but that one would first
need converting to switch(), which I agree is beyond the scope
of this change. In both cases a brief comment would seem
worthwhile adding, clarifying that the new return value can
result from hvm_copy_*_guest_linear() only. This might become
relevant in particular if, down the road, we invent more cases
where HVMTRANS_need_retry is produced.

Then again maybe switching rep_movs to switch() would still be
a good thing to do here: Don't you agree that from an abstract
pov in both cases above X86EMUL_RETRY should be produced, if at
a future point physical accesses could also produce
HVMTRANS_need_retry? With this retaining the assertions is
certainly an option, but I think the fallback return value for
this case should still be X86EMUL_RETRY.

Jan

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