Re: [Xen-devel] [PATCH v4 01/18] x86/hvm: introduce hvm_copy_context_and_params

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2020 18:13, Tamas K Lengyel wrote:
> @@ -4129,49 +4130,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;
> +    struct vcpu *v;

Nit: Personally I'd prefer if "rc" remained last.

> +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;
> +
> +    rc = hvm_set_param(d, a.index, a.value);

With

    rc = -EINVAL;
    if ( is_hvm_domain(d) )
        rc = hvm_set_param(d, a.index, a.value);

the function wouldn't need an "out" label (and hence any goto)
anymore. I know others are less picky about goto-s than me, but
I think in cases where it's easy to avoid them they would better
be avoided.

> @@ -4400,6 +4414,43 @@ static int hvm_allow_get_param(struct domain *d,
>      return rc;
>  }
>  
> +static int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value)
> +{
> +    int rc;
> +
> +    if ( index >= HVM_NR_PARAMS || !value )
> +        return -EINVAL;

I don't think the range check is needed here: It's redundant with
that in hvmop_get_param() and pointless for the new function you
add. (Same for "set" then, but I noticed it here first.) I also
don't think value needs checking against NULL in a case like this
one (we don't typically do so elsewhere in similar situations).

> @@ -5266,6 +5294,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 *src, struct domain *dst)

Following memcpy() and alike, perhaps better to have dst first and
src second?

> +{
> +    int rc, i;

unsigned int for i please.

> +    struct hvm_domain_context c = { };
> +
> +    c.size = hvm_save_size(src);

Put in the variable's initializer?

> +    if ( (c.data = xmalloc_bytes(c.size)) == NULL )

How likely is it for this to be more than a page's worth of space?
IOW wouldn't it be better to use vmalloc() here right away, even if
right now this may still fit in a page (which I'm not sure it does)?

> +        return -ENOMEM;
> +
> +    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)) )
> +            goto out;
> +    }
> +
> +    if ( (rc = hvm_save(src, &c)) )
> +        goto out;

Better do this ahead of the loop? There's no point in fiddling with
dst if this fails, I would think.

Jan

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