On Thu, Jan 16, 2020 at 5:28 AM Jan Beulich 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(, 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 )
> > +continue;
> > +
> > +if ( (rc = hvm_set_param(dst, i, value)) )
> > +goto out;
> > +}
> > +
> > +if ( (rc = hvm_save(src, )) )
> > +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
