Re: [Xen-devel] [PATCH v2 for Xen 4.6 1/4] xen: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.05.15 at 02:05, <lichong659@xxxxxxxxx> wrote:
> Add two hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to get/set a 
> domain's
> per-VCPU parameters. Hypercalls are handled by newly added hook 
> (.adjust_vcpu) in the
> scheduler interface.
> 
> Add a new data structure (struct xen_domctl_scheduler_vcpu_op) for 
> transferring data
> between tool and hypervisor.
> 
> Signed-off-by: Chong Li <chong.li@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
> ---

Missing brief description of changes in v2 here.

> +static int
> +rt_vcpu_cntl(
> +    const struct scheduler *ops,
> +    struct domain *d,
> +    struct xen_domctl_scheduler_vcpu_op *op)
> +{
> +    struct rt_private *prv = rt_priv(ops);
> +    struct rt_dom * const sdom = rt_dom(d);
> +    struct rt_vcpu *svc;
> +    struct list_head *iter;
> +    unsigned long flags;
> +    int rc = 0;
> +    xen_domctl_sched_rtds_params_t local_sched;
> +    unsigned int vcpuid;
> +    unsigned int i;
> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        list_for_each( iter, &sdom->vcpu )
> +        {
> +            svc = list_entry(iter, struct rt_vcpu, sdom_elem);
> +            vcpuid = svc->vcpu->vcpu_id;
> +
> +            local_sched.budget = svc->budget / MICROSECS(1);
> +            local_sched.period = svc->period / MICROSECS(1);
> +            if ( copy_to_guest_offset(op->u.rtds.vcpus, vcpuid,

What makes you think that the caller has provided enough slots?

> +                    &local_sched, 1) )

You're leaking hypervisor stack contents here, but by reading
the structure from guest memory first to honor its vcpuid field
(this making "get" match "put") you'd avoid this anyway.

Also - indentation.

> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return  -EFAULT;

Setting rc to -EFAULT and break-ing would seem better for these
error paths (avoiding the repeated spin_unlock_irqrestore()-s).

> +            }
> +            hypercall_preempt_check();

???

> +        }
> +        spin_unlock_irqrestore(&prv->lock, flags);
> +        break;
> +    case XEN_DOMCTL_SCHEDOP_putvcpuinfo:
> +        spin_lock_irqsave(&prv->lock, flags);
> +        for( i = 0; i < op->u.rtds.nr_vcpus; i++ )
> +        {
> +            if ( copy_from_guest_offset(&local_sched,
> +                    op->u.rtds.vcpus, i, 1) )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return -EFAULT;
> +            }
> +            if ( local_sched.period <= 0 || local_sched.budget <= 0 )
> +            {
> +                spin_unlock_irqrestore(&prv->lock, flags);
> +                return -EINVAL;
> +            }
> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);

You mustn't assume local_sched.vcpuid to represent a valid array
index.

> +            svc->period = MICROSECS(local_sched.period);
> +            svc->budget = MICROSECS(local_sched.budget);
> +            hypercall_preempt_check();

Again - ???

> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1104,6 +1104,30 @@ long sched_adjust(struct domain *d, struct 
> xen_domctl_scheduler_op *op)
>      return ret;
>  }
>  
> +/* Adjust scheduling parameter for the vcpus of a given domain. */
> +long sched_adjust_vcpu(
> +    struct domain *d,
> +    struct xen_domctl_scheduler_vcpu_op *op)
> +{
> +    long ret;

I see no reason for this and the function return type to be "long".

Jan


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