Re: [Xen-devel] [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work

[George Dunlap <dunlapg@xxxxxxxxx>]

(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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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