Message ID | 1496661535-10846-1-git-send-email-george.dunlap@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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: > */ > - >
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
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: */ -
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(-)