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

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

On Tue, Jan 28, 2020 at 9:48 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

I didn't write this code. This is preexisting code that I'm just
moving. I don't want to rewrite preexisting code here.

>
> > @@ -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?

I guess I'll do that cause I thought you asked for the allocation to
be moved at the end. It was the other way around before, so I guess I
don't know what you are asking for.

Tamas

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