Re: [PATCH v8 for Xen 4.7 1/4] xen: enable per-VCPU parameter for RTDS

["Jan Beulich" <JBeulich@suse.com>]

>>> On 21.03.16 at 16:18, <lichong659@gmail.com> wrote:
> On Mon, Mar 21, 2016 at 8:35 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 18.03.16 at 22:26, <lichong659@gmail.com> wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -338,24 +338,64 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_max_vcpus_t);
>>>  #define XEN_SCHEDULER_ARINC653 7
>>>  #define XEN_SCHEDULER_RTDS     8
>>>
>>> -/* Set or get info? */
>>> +typedef struct xen_domctl_sched_credit {
>>> +    uint16_t weight;
>>> +    uint16_t cap;
>>> +} xen_domctl_sched_credit_t;
>>> +
>>> +typedef struct xen_domctl_sched_credit2 {
>>> +    uint16_t weight;
>>> +} xen_domctl_sched_credit2_t;
>>> +
>>> +typedef struct xen_domctl_sched_rtds {
>>> +    uint32_t period;
>>> +    uint32_t budget;
>>> +} xen_domctl_sched_rtds_t;
>>> +
>>> +typedef struct xen_domctl_schedparam_vcpu {
>>> +    union {
>>> +        xen_domctl_sched_credit_t credit;
>>> +        xen_domctl_sched_credit2_t credit2;
>>> +        xen_domctl_sched_rtds_t rtds;
>>> +    } u;
>>> +    uint32_t vcpuid;
>>> +    uint16_t padding[2];
>>
>> So why uint16_t[2] instead of just uint32_t? And what's the
>> padding needed for in the first place?
> 
> You're right. It's better to use uint32_t, which pads (the struct) to
> the 64-bit boundary.

Which doesn't answer the "why in the first place" part - I don't
see why structure size needs to be a multiple of 64 bits.

>> Also, while for a domctl it's
>> not as strictly needed as for other hypercalls, checking that all
>> padding fields are zero would still seem to be rather desirable.
>>
> 
> Do you mean:
> 
> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> +        if ( op->u.v.padding !=0 )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        while ( index < op->u.v.nr_vcpus )
> +        {
> +            if ( copy_from_guest_offset(&local_sched,
> +                                        op->u.v.vcpus, index, 1) )
> +            {
> +                rc = -EFAULT;
> +                break;
> +            }
> +            if ( local_sched.padding != 0 )
> +            {
> +                rc = -EINVAL;
> +                break;
> +            }
> +            if ( local_sched.vcpuid >= d->max_vcpus ||
> +                 d->vcpu[local_sched.vcpuid] == NULL )
> +            {
> +                rc = -EINVAL;
> +                break;
> +            }
> 
> ?

Yes, something along those lines.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel