| Message ID | 1464907946-19242-1-git-send-email-tamas@tklengyel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/03/16 01:52, Tamas K Lengyel wrote: > The return value has not been clearly defined, with the function > never returning 0 which seemingly indicated a condition where the > guest should crash. In this patch we define -rc as error condition > where the notification was not sent, 0 where the notification was sent > and the vCPU is not paused and 1 that the notification was sent and > that the vCPU is paused. > > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> > --- > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > --- > xen/arch/x86/hvm/event.c | 4 ++-- > xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- > xen/common/vm_event.c | 5 +++-- > 3 files changed, 8 insertions(+), 7 deletions(-) Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3392,11 +3392,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > break; > } > else { > - int handled = > + int rc = > hvm_event_breakpoint(regs->eip, > HVM_EVENT_SOFTWARE_BREAKPOINT); > > - if ( handled < 0 ) > + if ( !rc ) > { > struct hvm_trap trap = { > .vector = TRAP_int3, > @@ -3410,7 +3410,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) > hvm_inject_trap(&trap); > break; > } > - else if ( handled ) > + else if ( rc > 0 ) > break; > } > Ah, I guess that's what you were referring to on the other thread. There's indeed quite a bit of cleanup potential here. The minimal thing I'd like to ask for is to drop the pointless "else", as you're touching that line anyway. With that (also doable upon commit of course) Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Fri, Jun 3, 2016 at 9:54 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 03.06.16 at 00:52, <tamas@tklengyel.com> wrote: >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -3392,11 +3392,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >> break; >> } >> else { >> - int handled = >> + int rc = >> hvm_event_breakpoint(regs->eip, >> HVM_EVENT_SOFTWARE_BREAKPOINT); >> >> - if ( handled < 0 ) >> + if ( !rc ) >> { >> struct hvm_trap trap = { >> .vector = TRAP_int3, >> @@ -3410,7 +3410,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) >> hvm_inject_trap(&trap); >> break; >> } >> - else if ( handled ) >> + else if ( rc > 0 ) >> break; >> } >> > > Ah, I guess that's what you were referring to on the other thread. > There's indeed quite a bit of cleanup potential here. The minimal > thing I'd like to ask for is to drop the pointless "else", as you're > touching that line anyway. Sounds good. > > With that (also doable upon commit of course) > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Jan Thanks, Tamas
On Thu, Jun 2, 2016 at 4:52 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: > The return value has not been clearly defined, with the function > never returning 0 which seemingly indicated a condition where the > guest should crash. In this patch we define -rc as error condition > where the notification was not sent, 0 where the notification was sent > and the vCPU is not paused and 1 that the notification was sent and > that the vCPU is paused. > > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> > --- > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> Pinging the rest of the maintainers to get an update on this patch. Current status is: Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, Tamas
> From: Tamas K Lengyel [mailto:tamas@tklengyel.com] > Sent: Saturday, June 18, 2016 3:09 AM > > On Thu, Jun 2, 2016 at 4:52 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote: > > The return value has not been clearly defined, with the function > > never returning 0 which seemingly indicated a condition where the > > guest should crash. In this patch we define -rc as error condition > > where the notification was not sent, 0 where the notification was sent > > and the vCPU is not paused and 1 that the notification was sent and > > that the vCPU is paused. > > > > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> > > --- > > Cc: Jun Nakajima <jun.nakajima@intel.com> > > Cc: Kevin Tian <kevin.tian@intel.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Pinging the rest of the maintainers to get an update on this patch. > Current status is: > > Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Acked-by: Kevin Tian <kevin.tian@intel.com>
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index 56c5514..5772c6b 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -47,8 +47,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old) .u.write_ctrlreg.old_value = old }; - vm_event_monitor_traps(curr, sync, &req); - return 1; + if ( vm_event_monitor_traps(curr, sync, &req) >= 0 ) + return 1; } return 0; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 3acf1ab..097d97c 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3392,11 +3392,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) break; } else { - int handled = + int rc = hvm_event_breakpoint(regs->eip, HVM_EVENT_SOFTWARE_BREAKPOINT); - if ( handled < 0 ) + if ( !rc ) { struct hvm_trap trap = { .vector = TRAP_int3, @@ -3410,7 +3410,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) hvm_inject_trap(&trap); break; } - else if ( handled ) + else if ( rc > 0 ) break; } diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 2906407..fe86fb9 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -801,7 +801,7 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync, * If there was no ring to handle the event, then * simply continue executing normally. */ - return 1; + return 0; default: return rc; }; @@ -810,6 +810,7 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync, { req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; vm_event_vcpu_pause(v); + rc = 1; } if ( altp2m_active(d) ) @@ -821,7 +822,7 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync, vm_event_fill_regs(req); vm_event_put_request(d, &d->vm_event->monitor, req); - return 1; + return rc; } void vm_event_monitor_guest_request(void)
The return value has not been clearly defined, with the function never returning 0 which seemingly indicated a condition where the guest should crash. In this patch we define -rc as error condition where the notification was not sent, 0 where the notification was sent and the vCPU is not paused and 1 that the notification was sent and that the vCPU is paused. Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com> --- Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/hvm/event.c | 4 ++-- xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- xen/common/vm_event.c | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-)