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

[Chong Li <lichong659@gmail.com>]

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:
>> Add XEN_DOMCTL_SCHEDOP_getvcpuinfo and _putvcpuinfo hypercalls
>> to independently get and set the scheduling parameters of each
>> vCPU of a domain
>>
>> Signed-off-by: Chong Li <chong.li@wustl.edu>
>> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
>> Signed-off-by: Sisu Xi <xisisu@gmail.com>
>>
>> ---
>> Changes on PATCH v7:
>> 1) A bug in case XEN_DOMCTL_SCHEDOP_getinfo (in Xen 4.6) is fixed:
>> The default PERIOD or BUDGET should be divided by MICROSECS(1),
>> before returned to upper caller.
>
> Seems like there's still some misunderstanding here: Anything
> past the first --- won't make it into the repo, yet the description
> of what bug you fix should end up there.

I see. Do I put the "bug fix" before "Signed-off-by ..." or after it?

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

> 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;
+            }

?

Thanks,
Chong


-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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