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:59 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 28.01.2020 17:54, Tamas K Lengyel wrote:
> > 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.
>
> Well, with the code movement you could (and imo should) _move_
> the "out" label instead of duplicating it.

I really rather not change preexisting code when possible.

Tamas

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