Re: [Xen-devel] [PATCH for-4.9] Restore HVM_OP hypercall continuation

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 05/06/17 12:18, George Dunlap wrote:
> Commit ae20ccf removed the hypercall continuation logic from the end
> of do_hvm_op(), claiming:
> 
> "This patch removes the need for handling HVMOP restarts, so that
> infrastructure is removed."
> 
> That turns out to be only half true.  The removal of
> HVMOP_set_mem_type removed the need to store a start iteration value
> in the hypercall continuation, but a grep through hvm.c for ERESTART
> turns up at least two places where do_hvm_op() may still need a
> hypercall continuation:
> 
>  * HVMOP_set_hvm_param can return -ERESTART when setting
> HVM_PARAM_IDENT_PT in the event that it fails to acquire the domctl
> lock
> 
>  * HVMOP_flush_tlbs can return -ERESTART if several vcpus call it at
>    the same time
> 
> In both cases, a simple restart (with no stored iteration information)
> is necessary.
> 
> Add a check for -ERESTART again, along with a comment at the top of
> the function regarding the lack of decoding any information from the
> op value.
> 
> Remove a stray blank line at the end of the file while we're here.
> 
> Reported-by: Xudong Hao <xudong.hao@xxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Oh, actually Andy and Julien both already acked this.  I'll check it in
on staging and cherry-pick it to staging-4.9 unless I hear otherwise soon.

 -George

> ---
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 81691e2..e3e817d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4544,6 +4544,13 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      long rc = 0;
>  
> +    /* 
> +     * NB: hvm_op can be part of a restarted hypercall; but at the
> +     * moment the only hypercalls which do continuations don't need to
> +     * store any iteration information (since they're just re-trying
> +     * the acquisition of a lock).
> +     */
> +    
>      switch ( op )
>      {
>      case HVMOP_set_evtchn_upcall_vector:
> @@ -4636,6 +4643,10 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      }
>      }
>  
> +    if ( rc == -ERESTART )
> +        rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
> +                                           op, arg);
> +
>      return rc;
>  }
>  
> @@ -4869,4 +4880,3 @@ void hvm_set_segment_register(struct vcpu *v, enum 
> x86_segment seg,
>   * indent-tabs-mode: nil
>   * End:
>   */
> -
> 


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