| Message ID | 1473285866-6868-3-git-send-email-paul.c.lai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.09.16 at 00:04, <paul.c.lai@intel.com> wrote: > Indent goto labels by one space > Inline (header) altp2m functions > Define default behavior in switch > Define max and min for range of altp2m macroed values > > Signed-off-by: Paul Lai <paul.c.lai@intel.com> > --- Missing a brief summary of changes from previous version here. > @@ -5413,6 +5426,8 @@ static int do_altp2m_op( > rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view, > _gfn(a.u.change_gfn.old_gfn), > _gfn(a.u.change_gfn.new_gfn)); > + default: > + ASSERT_UNREACHABLE(); > } Did you test anything using HVMOP_altp2m_change_gfn with this change, on a debug build? There's obviously an unintended fall- through right now. > /* emulates #VE */ > -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v); > +static inline bool_t altp2m_vcpu_emulate_ve(struct vcpu *v) Already on v3 I had asked to switch to plain bool (and true/false as appropriate). Jan
[Paul] in-line -----Original Message----- From: Jan Beulich [mailto:JBeulich@suse.com] Sent: Thursday, September 8, 2016 7:47 AM To: Lai, Paul C <paul.c.lai@intel.com> Cc: george.dunlap@citrix.com; Sahita, Ravi <ravi.sahita@intel.com>; xen-devel <xen-devel@lists.xenproject.org> Subject: Re: [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work >>> On 08.09.16 at 00:04, <paul.c.lai@intel.com> wrote: > Indent goto labels by one space > Inline (header) altp2m functions > Define default behavior in switch > Define max and min for range of altp2m macroed values > > Signed-off-by: Paul Lai <paul.c.lai@intel.com> > --- Missing a brief summary of changes from previous version here. > @@ -5413,6 +5426,8 @@ static int do_altp2m_op( > rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view, > _gfn(a.u.change_gfn.old_gfn), > _gfn(a.u.change_gfn.new_gfn)); > + default: > + ASSERT_UNREACHABLE(); > } Did you test anything using HVMOP_altp2m_change_gfn with this change, on a debug build? There's obviously an unintended fall- through right now. [Paul] - Yes, this patch series was tested with Tamas's HVMOP_altp2m_change_gfn in a debug build. > /* emulates #VE */ > -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v); > +static inline bool_t altp2m_vcpu_emulate_ve(struct vcpu *v) Already on v3 I had asked to switch to plain bool (and true/false as appropriate). [Paul] - Maybe I misunderstood something here. I fixed the return value to false in the patch. ... Ah, it's the non-false case that's returning void. Will fix. Jan
On Thu, Sep 8, 2016 at 9:50 AM, Lai, Paul C <paul.c.lai@intel.com> wrote: > [Paul] in-line > > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, September 8, 2016 7:47 AM > To: Lai, Paul C <paul.c.lai@intel.com> > Cc: george.dunlap@citrix.com; Sahita, Ravi <ravi.sahita@intel.com>; xen-devel <xen-devel@lists.xenproject.org> > Subject: Re: [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work > >>>> On 08.09.16 at 00:04, <paul.c.lai@intel.com> wrote: >> Indent goto labels by one space >> Inline (header) altp2m functions >> Define default behavior in switch >> Define max and min for range of altp2m macroed values >> >> Signed-off-by: Paul Lai <paul.c.lai@intel.com> >> --- > > Missing a brief summary of changes from previous version here. > >> @@ -5413,6 +5426,8 @@ static int do_altp2m_op( >> rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view, >> _gfn(a.u.change_gfn.old_gfn), >> _gfn(a.u.change_gfn.new_gfn)); >> + default: >> + ASSERT_UNREACHABLE(); >> } > > Did you test anything using HVMOP_altp2m_change_gfn with this change, on a debug build? There's obviously an unintended fall- through right now. > > [Paul] - Yes, this patch series was tested with Tamas's HVMOP_altp2m_change_gfn in a debug build. Not sure what you used here, the xen-access tool right now in Xen doesn't (yet) exercise the gfn remapping, only the mem-access parts. Sergej has a patch for this in the arm-altp2m series though. Tamas
[Paul2] in-line -----Original Message----- From: Tamas K Lengyel [mailto:tamas.k.lengyel@gmail.com] Sent: Thursday, September 8, 2016 9:07 AM To: Lai, Paul C <paul.c.lai@intel.com> Cc: Jan Beulich <JBeulich@suse.com>; xen-devel <xen-devel@lists.xenproject.org>; Sahita, Ravi <ravi.sahita@intel.com>; george.dunlap@citrix.com Subject: Re: [Xen-devel] [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work On Thu, Sep 8, 2016 at 9:50 AM, Lai, Paul C <paul.c.lai@intel.com> wrote: > [Paul] in-line > > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, September 8, 2016 7:47 AM > To: Lai, Paul C <paul.c.lai@intel.com> > Cc: george.dunlap@citrix.com; Sahita, Ravi <ravi.sahita@intel.com>; > xen-devel <xen-devel@lists.xenproject.org> > Subject: Re: [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work > >>>> On 08.09.16 at 00:04, <paul.c.lai@intel.com> wrote: >> Indent goto labels by one space >> Inline (header) altp2m functions >> Define default behavior in switch >> Define max and min for range of altp2m macroed values >> >> Signed-off-by: Paul Lai <paul.c.lai@intel.com> >> --- > > Missing a brief summary of changes from previous version here. > >> @@ -5413,6 +5426,8 @@ static int do_altp2m_op( >> rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view, >> _gfn(a.u.change_gfn.old_gfn), >> _gfn(a.u.change_gfn.new_gfn)); >> + default: >> + ASSERT_UNREACHABLE(); >> } > > Did you test anything using HVMOP_altp2m_change_gfn with this change, on a debug build? There's obviously an unintended fall- through right now. > > [Paul] - Yes, this patch series was tested with Tamas's HVMOP_altp2m_change_gfn in a debug build. Not sure what you used here, the xen-access tool right now in Xen doesn't (yet) exercise the gfn remapping, only the mem-access parts. Sergej has a patch for this in the arm-altp2m series though. Tamas [Paul2] All I was testing was if xen-access behaved as before the gfn remapping patch was introduced. After the gfn remapping patch was introduced, 'xen-acess -m 1 altp2m_write' hanged the system. Now, with your fix, it doesn't and the output of xen-access appears as before. Thanks, -Paul
On 08/09/16 17:45, Lai, Paul C wrote:
> [Paul2] in-line
If you're going to engage in discussions on xen-devel it would really be
worth your time to find a mail setup that allows you to actually quote
properly such that you can reply in-line without these manual mark-ups.
If you can't configure your mail reader to do proper nested quoting, and
you can't / don't want to change your mail reader, then you could
consider doing what I do and signing up for a gmail account to get all
the xen-devel mail and replying from there.
Thanks,
-George
On Mon, Sep 12, 2016 at 11:47:35AM +0100, George Dunlap wrote: > On 08/09/16 17:45, Lai, Paul C wrote: > > [Paul2] in-line > > If you're going to engage in discussions on xen-devel it would really be > worth your time to find a mail setup that allows you to actually quote > properly such that you can reply in-line without these manual mark-ups. > > If you can't configure your mail reader to do proper nested quoting, and > you can't / don't want to change your mail reader, then you could > consider doing what I do and signing up for a gmail account to get all > the xen-devel mail and replying from there. > > Thanks, > -George > George, Thanks for the suggestions. Hopefully this reply illustrates that I can learn. -Paul
On 13/09/16 18:38, Lai, Paul wrote: > On Mon, Sep 12, 2016 at 11:47:35AM +0100, George Dunlap wrote: >> On 08/09/16 17:45, Lai, Paul C wrote: >>> [Paul2] in-line >> >> If you're going to engage in discussions on xen-devel it would really be >> worth your time to find a mail setup that allows you to actually quote >> properly such that you can reply in-line without these manual mark-ups. >> >> If you can't configure your mail reader to do proper nested quoting, and >> you can't / don't want to change your mail reader, then you could >> consider doing what I do and signing up for a gmail account to get all >> the xen-devel mail and replying from there. >> >> Thanks, >> -George >> > > George, > Thanks for the suggestions. No problem -- and I hope the tone of my message didn't come off too wrong. I was trying to find solutions that work for everyone, not criticize. :-) -George
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 787f055..e63ef0b 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1942,11 +1942,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, * Otherwise, this is an error condition. */ rc = fall_through; -out_put_gfn: + out_put_gfn: __put_gfn(p2m, gfn); if ( ap2m_active ) __put_gfn(hostp2m, gfn); -out: + out: /* All of these are delayed until we exit, since we might * sleep on event ring wait queues, and we must not hold * locks in such circumstance */ @@ -5291,12 +5291,25 @@ static int do_altp2m_op( return -EFAULT; if ( a.pad1 || a.pad2 || - (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) || - (a.cmd < HVMOP_altp2m_get_domain_state) || - (a.cmd > HVMOP_altp2m_change_gfn) ) + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ) return -EINVAL; - d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? + switch ( a.cmd ) + { + case HVMOP_altp2m_get_domain_state: + case HVMOP_altp2m_set_domain_state: + case HVMOP_altp2m_vcpu_enable_notify: + case HVMOP_altp2m_create_p2m: + case HVMOP_altp2m_destroy_p2m: + case HVMOP_altp2m_switch_p2m: + case HVMOP_altp2m_set_mem_access: + case HVMOP_altp2m_change_gfn: + break; + default: + return -ENOSYS; + } + + d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ? rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain(); if ( d == NULL ) @@ -5413,6 +5426,8 @@ static int do_altp2m_op( rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view, _gfn(a.u.change_gfn.old_gfn), _gfn(a.u.change_gfn.new_gfn)); + default: + ASSERT_UNREACHABLE(); } out: @@ -5937,25 +5952,6 @@ void hvm_toggle_singlestep(struct vcpu *v) v->arch.hvm_vcpu.single_step = !v->arch.hvm_vcpu.single_step; } -void altp2m_vcpu_update_p2m(struct vcpu *v) -{ - if ( hvm_funcs.altp2m_vcpu_update_p2m ) - hvm_funcs.altp2m_vcpu_update_p2m(v); -} - -void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v) -{ - if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve ) - hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v); -} - -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v) -{ - if ( hvm_funcs.altp2m_vcpu_emulate_ve ) - return hvm_funcs.altp2m_vcpu_emulate_ve(v); - return 0; -} - int hvm_set_mode(struct vcpu *v, int mode) { diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 81b60d5..ed043b2 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -599,13 +599,26 @@ static inline bool_t hvm_altp2m_supported(void) } /* updates the current hardware p2m */ -void altp2m_vcpu_update_p2m(struct vcpu *v); +static inline void altp2m_vcpu_update_p2m(struct vcpu *v) +{ + if ( hvm_funcs.altp2m_vcpu_update_p2m ) + hvm_funcs.altp2m_vcpu_update_p2m(v); +} /* updates VMCS fields related to VMFUNC and #VE */ -void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v); +static inline void altp2m_vcpu_update_vmfunc_ve(struct vcpu *v) +{ + if ( hvm_funcs.altp2m_vcpu_update_vmfunc_ve ) + hvm_funcs.altp2m_vcpu_update_vmfunc_ve(v); +} /* emulates #VE */ -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v); +static inline bool_t altp2m_vcpu_emulate_ve(struct vcpu *v) +{ + if ( hvm_funcs.altp2m_vcpu_emulate_ve ) + return hvm_funcs.altp2m_vcpu_emulate_ve(v); + return false; +} /* Check CR4/EFER values */ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value,
Indent goto labels by one space Inline (header) altp2m functions Define default behavior in switch Define max and min for range of altp2m macroed values Signed-off-by: Paul Lai <paul.c.lai@intel.com> --- xen/arch/x86/hvm/hvm.c | 46 ++++++++++++++++++++----------------------- xen/include/asm-x86/hvm/hvm.h | 19 +++++++++++++++--- 2 files changed, 37 insertions(+), 28 deletions(-)