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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Alloc, save, loop over params, load, free.

Jan

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