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

[Tamas K Lengyel <tamas.k.lengyel@gmail.com>]

On Thu, Jan 16, 2020 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> 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)?

I'm not sure what the size is normally, never checked.

>
> > +        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.

Thanks for the review, I don't have any objections to the things you
pointed out.

Tamas

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