Re: [Xen-devel] [PATCH v6 2/9] x86/hvm: introduce hvm_copy_context_and_params

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2020 19:06, Tamas K Lengyel wrote:
> @@ -4139,49 +4140,32 @@ static int hvm_allow_set_param(struct domain *d,
>      return rc;
>  }
>  
> -static int hvmop_set_param(
> -    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> +static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
>  {
>      struct domain *curr_d = current->domain;
> -    struct xen_hvm_param a;
> -    struct domain *d;
>      struct vcpu *v;
>      int rc;
>  
> -    if ( copy_from_guest(&a, arg, 1) )
> -        return -EFAULT;
> -
> -    if ( a.index >= HVM_NR_PARAMS )
> +    if ( index >= HVM_NR_PARAMS )
>          return -EINVAL;

The equivalent of this on the "get" path now seems to be gone. Is
there any reason the one here is still needed?

> +int hvmop_set_param(
> +    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> +{
> +    struct xen_hvm_param a;
> +    struct domain *d;
> +    int rc;
> +
> +    if ( copy_from_guest(&a, arg, 1) )
> +        return -EFAULT;
> +
> +    if ( a.index >= HVM_NR_PARAMS )
> +        return -EINVAL;
> +
> +    /* Make sure the above bound check is not bypassed during speculation. */
> +    block_speculation();
> +
> +    d = rcu_lock_domain_by_any_id(a.domid);
> +    if ( d == NULL )
> +        return -ESRCH;
> +
> +    rc = -EINVAL;
> +    if ( !is_hvm_domain(d) )
> +        goto out;

Despite your claim to have addressed my remaining comment from v4,
you still use goto here when there's an easy alternative.

> @@ -5297,6 +5322,37 @@ void hvm_set_segment_register(struct vcpu *v, enum 
> x86_segment seg,
>      alternative_vcall(hvm_funcs.set_segment_register, v, seg, reg);
>  }
>  
> +int hvm_copy_context_and_params(struct domain *dst, struct domain *src)
> +{
> +    int rc;
> +    unsigned int i;
> +    struct hvm_domain_context c = { };
> +
> +    for ( i = 0; i < HVM_NR_PARAMS; i++ )
> +    {
> +        uint64_t value = 0;
> +
> +        if ( hvm_get_param(src, i, &value) || !value )
> +            continue;
> +
> +        if ( (rc = hvm_set_param(dst, i, value)) )
> +            return rc;
> +    }
> +
> +    c.size = hvm_save_size(src);
> +    if ( (c.data = vmalloc(c.size)) == NULL )
> +        return -ENOMEM;
> +
> +    if ( !(rc = hvm_save(src, &c)) )

Also contrary to your claim you still do allocation and save
after the loop, leaving dst in a partly modified state in more
cases than necessary. May I ask that you go back to the v4
comments one more time?

Jan

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