[PATCH for-4.9] Restore HVM_OP hypercall continuation

[PATCH for-4.9] Restore HVM_OP hypercall continuation

[George Dunlap]

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@intel.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 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:
  */
-
-- 
2.1.4


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

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

[George Dunlap]

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@intel.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

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@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[George Dunlap]

On Mon, Jun 5, 2017 at 12:20 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 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@intel.com>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>
> 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.

Either that, or I'll discover that it's already been checked in and I
didn't notice because I failed to merge origin/staging into staging.

Sorry for the noise everyone.

 -George

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