| Message ID | 1466525090-1692-2-git-send-email-paul.c.lai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 21.06.16 at 18:04, <paul.c.lai@intel.com> wrote: > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op { > #define HVMOP_altp2m_set_mem_access 7 > /* Change a p2m entry to have a different gfn->mfn mapping */ > #define HVMOP_altp2m_change_gfn 8 > +#define HVMOP_cmd_min HVMOP_altp2m_get_domain_state > +#define HVMOP_cmd_max HVMOP_altp2m_change_gfn I don't see why these would need to be in the public interface. Nor were their names chosen to properly describe their purpose. Jan
I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else. That would make reading/understanding of the macros more difficult. This practice is common. Also, If min & max are defined elsewhere, it will be more likely to lead to mistakes/bugs. The use of "_min" and "_max" should be quite clear and is common use in linux code; Yes, I know this is xen code and I see it here too. If there's a better way, please propose the better. Maybe you're suggesting the macro names should be all caps: HVMOP_CMD_MIN, HVMOP_CMD_MAX ? I was following the coding style within the file itself. I'm open to suggestions, -Paul -----Original Message----- From: Jan Beulich [mailto:JBeulich@suse.com] Sent: Thursday, June 23, 2016 8:45 AM To: Lai, Paul C <paul.c.lai@intel.com> Cc: Sahita, Ravi <ravi.sahita@intel.com>; xen-devel <xen-devel@lists.xenproject.org> Subject: Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work >>> On 21.06.16 at 18:04, <paul.c.lai@intel.com> wrote: > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op { > #define HVMOP_altp2m_set_mem_access 7 > /* Change a p2m entry to have a different gfn->mfn mapping */ > #define HVMOP_altp2m_change_gfn 8 > +#define HVMOP_cmd_min HVMOP_altp2m_get_domain_state > +#define HVMOP_cmd_max HVMOP_altp2m_change_gfn I don't see why these would need to be in the public interface. Nor were their names chosen to properly describe their purpose. Jan
>>> On 23.06.16 at 20:23, <paul.c.lai@intel.com> wrote: First of all - please don't top post. > I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else. That > would make reading/understanding of the macros more difficult. This practice > is common. Also, If min & max are defined elsewhere, it will be more likely > to lead to mistakes/bugs. I understand that, but I'm going to nak any patch that introduces clutter (which then will need to be retained forever) to the public interface. A possible compromise might be to frame these in __XEN__ conditionals, but this would need to be outweighed by the resulting benefits, which I'm not convinced of. > The use of "_min" and "_max" should be quite clear and is common use in > linux code; Yes, I know this is xen code and I see it here too. If there's a > better way, please propose the better. Maybe you're suggesting the macro > names should be all caps: > HVMOP_CMD_MIN, HVMOP_CMD_MAX > ? I was following the coding style within the file itself. The problem isn't the use of min and max, but the selected names suggesting these are the minimum/maximum HVMOPs, whereas - afaict - you really mean to frame the altp2m subset. Which btw, together with the above, points at another issue here: What if later another altp2m subop gets added, and especially when its new value ends up not being adjacent to the existing range? Jan
[Paul] inlined.... -----Original Message----- From: Jan Beulich [mailto:JBeulich@suse.com] Sent: Thursday, June 23, 2016 11:19 PM To: Lai, Paul C <paul.c.lai@intel.com> Cc: Sahita, Ravi <ravi.sahita@intel.com>; xen-devel <xen-devel@lists.xenproject.org> Subject: RE: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work >>> On 23.06.16 at 20:23, <paul.c.lai@intel.com> wrote: First of all - please don't top post. [Paul] Understood > I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else. > That would make reading/understanding of the macros more difficult. > This practice is common. Also, If min & max are defined elsewhere, it > will be more likely to lead to mistakes/bugs. I understand that, but I'm going to nak any patch that introduces clutter (which then will need to be retained forever) to the public interface. A possible compromise might be to frame these in __XEN__ conditionals, but this would need to be outweighed by the resulting benefits, which I'm not convinced of. [Paul] Still waiting for a better idea. > The use of "_min" and "_max" should be quite clear and is common use > in linux code; Yes, I know this is xen code and I see it here too. If > there's a better way, please propose the better. Maybe you're > suggesting the macro names should be all caps: > HVMOP_CMD_MIN, HVMOP_CMD_MAX > ? I was following the coding style within the file itself. The problem isn't the use of min and max, but the selected names suggesting these are the minimum/maximum HVMOPs, whereas - afaict - you really mean to frame the altp2m subset. Which btw, together with the above, points at another issue here: What if later another altp2m subop gets added, and especially when its new value ends up not being adjacent to the existing range? [Paul] Again, waiting for a better idea. Jan
On Thu, Jun 23, 2016 at 7:23 PM, Lai, Paul C <paul.c.lai@intel.com> wrote: > I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else. That would make reading/understanding of the macros more difficult. This practice is common. Also, If min & max are defined elsewhere, it will be more likely to lead to mistakes/bugs. > > The use of "_min" and "_max" should be quite clear and is common use in linux code; Yes, I know this is xen code and I see it here too. If there's a better way, please propose the better. Maybe you're suggesting the macro names should be all caps: > HVMOP_CMD_MIN, HVMOP_CMD_MAX > ? I was following the coding style within the file itself. Jan was suggesting that these should be called HVMOP_altp2m_{max,min} (or perhaps HVMOP_altp2m_cmd_{max,min}). But in any case, the most robust thing to do is not to check the values here at all -- just add a default: clause to the switch() statement which returns -ENOSYS. -George
(Removing Paul, adding Lars) Ravi, I just got to this thread, and I was quite disappointed with both the code and the interaction here. In patch 1, the naming of the min/max is obviously inappropriate, and a.cmd is compared to HVMOP_cmd_max twice in a row. In patch 3, unused elements of the struct are commented out rather than deleted. These aren't "new to the Xen way of doing things" mistakes, these are basic programming errors. Did anyone review this series internally? -George On Tue, Jun 21, 2016 at 5:04 PM, Paul Lai <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> > --- > xen/arch/x86/hvm/hvm.c | 33 +++++++++------------------------ > xen/include/asm-x86/hvm/hvm.h | 19 ++++++++++++++++--- > xen/include/public/hvm/hvm_op.h | 2 ++ > 3 files changed, 27 insertions(+), 27 deletions(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 22f045e..1595b3e 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1926,11 +1926,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 */ > @@ -5207,9 +5207,11 @@ 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) || > + (a.cmd < HVMOP_cmd_min) || (a.cmd > HVMOP_cmd_max)) > + return -EINVAL; > + > + if (a.cmd > HVMOP_cmd_max) > return -EINVAL; > > d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? > @@ -5329,6 +5331,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: > + return -EINVAL; > } > > out: > @@ -5816,25 +5820,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 f486ee9..231c921 100644 > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -589,13 +589,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 0; > +} > > /* Check CR4/EFER values */ > const char *hvm_efer_valid(const struct vcpu *v, uint64_t value, > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > index ebb907a..3350492 100644 > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op { > #define HVMOP_altp2m_set_mem_access 7 > /* Change a p2m entry to have a different gfn->mfn mapping */ > #define HVMOP_altp2m_change_gfn 8 > +#define HVMOP_cmd_min HVMOP_altp2m_get_domain_state > +#define HVMOP_cmd_max HVMOP_altp2m_change_gfn > domid_t domain; > uint16_t pad1; > uint32_t pad2; > -- > 1.9.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 22f045e..1595b3e 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1926,11 +1926,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 */ @@ -5207,9 +5207,11 @@ 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) || + (a.cmd < HVMOP_cmd_min) || (a.cmd > HVMOP_cmd_max)) + return -EINVAL; + + if (a.cmd > HVMOP_cmd_max) return -EINVAL; d = (a.cmd != HVMOP_altp2m_vcpu_enable_notify) ? @@ -5329,6 +5331,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: + return -EINVAL; } out: @@ -5816,25 +5820,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 f486ee9..231c921 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -589,13 +589,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 0; +} /* Check CR4/EFER values */ const char *hvm_efer_valid(const struct vcpu *v, uint64_t value, diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index ebb907a..3350492 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op { #define HVMOP_altp2m_set_mem_access 7 /* Change a p2m entry to have a different gfn->mfn mapping */ #define HVMOP_altp2m_change_gfn 8 +#define HVMOP_cmd_min HVMOP_altp2m_get_domain_state +#define HVMOP_cmd_max HVMOP_altp2m_change_gfn domid_t domain; uint16_t pad1; uint32_t pad2;
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 | 33 +++++++++------------------------ xen/include/asm-x86/hvm/hvm.h | 19 ++++++++++++++++--- xen/include/public/hvm/hvm_op.h | 2 ++ 3 files changed, 27 insertions(+), 27 deletions(-)