Re: [PATCH 4/5] x86/hvm: Misc non-functional cleanup to the HVM_PARAM infrastructure

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Sep 05, 2018 at 07:12:03PM +0100, Andrew Cooper wrote:
> The parameter marshalling and xsm checks are common to both the set and get
> paths.  Lift all of the common code out into do_hvm_op() and let
> hvmop_{get,set}_param() operate on struct xen_hvm_param directly.
> 
> This is somewhat noisy in the functions as each a. reference has to change to
> a-> instead.
> 
> In addition, drop an empty default statement, insert newlines as appropriate
> between cases, and there is no need to update the IDENT_PT on the fastpath,
> because the common path after the switch will make the update.
> 
> No functional change, but a net shrink of -328 to do_hvm_op().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just two suggestions below.

> @@ -4322,41 +4308,34 @@ static int hvmop_set_param(
>           * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap
>           * plus one padding byte).
>           */
> -        if ( (a.value >> 32) > sizeof(struct tss32) +
> -                               (0x100 / 8) + (0x10000 / 8) + 1 )
> -            a.value = (uint32_t)a.value |
> -                      ((sizeof(struct tss32) + (0x100 / 8) +
> -                                               (0x10000 / 8) + 1) << 32);
> -        a.value |= VM86_TSS_UPDATED;
> +        if ( (a->value >> 32) > sizeof(struct tss32) +
> +                                (0x100 / 8) + (0x10000 / 8) + 1 )
> +            a->value = (uint32_t)a->value |
> +                       ((sizeof(struct tss32) + (0x100 / 8) +
> +                                                (0x10000 / 8) + 1) << 32);
> +        a->value |= VM86_TSS_UPDATED;
>          break;
>  
>      case HVM_PARAM_MCA_CAP:
> -        rc = vmce_enable_mca_cap(d, a.value);
> +        rc = vmce_enable_mca_cap(d, a->value);
>          break;
>      }
>  
>      if ( rc != 0 )
>          goto out;
>  
> -    d->arch.hvm.params[a.index] = a.value;
> +    d->arch.hvm.params[a->index] = a->value;
>  
>      HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64,
> -                a.index, a.value);
> +                a->index, a->value);
>  
>   out:
> -    rcu_unlock_domain(d);
>      return rc;

If the out label is just return rc, and unless there's no further
patches adding code here I would consider removing it.

> -static int hvmop_get_param(
> -    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> +static int hvmop_get_param(struct domain *d, struct xen_hvm_param *a)
>  {
> -    struct xen_hvm_param a;
> -    struct domain *d;
>      int rc;
>  
> -    if ( copy_from_guest(&a, arg, 1) )
> -        return -EFAULT;
> -
> -    d = rcu_lock_domain_by_any_id(a.domid);
> -    if ( d == NULL )
> -        return -ESRCH;
> -
> -    rc = -EINVAL;
> -    if ( !is_hvm_domain(d) )
> -        goto out;
> -
> -    rc = hvm_allow_get_param(d, &a);
> +    rc = hvm_allow_get_param(d, a);

You could move this initialization at declaration time.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel