Re: [Xen-devel] [PATCH v14 01/10] x86: add generic resource (e.g. MSR) access hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.08.14 at 09:43, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> +static void resource_access_one(void *info)
> +{
> +    struct xen_resource_access *ra = info;
> +    int ret = 0;
> +
> +    switch ( ra->data.cmd )
> +    {
> +    case XEN_RESOURCE_OP_MSR_READ:
> +    case XEN_RESOURCE_OP_MSR_WRITE:
> +        if ( ra->data.idx >> 32 )
> +            ret = -EINVAL;
> +        if ( !allow_access_msr(ra->data.idx) )
> +            ret = -EACCES;
> +        if ( ra->data.cmd == XEN_RESOURCE_OP_MSR_READ )
> +            ret = rdmsr_safe(ra->data.idx, ra->data.val);
> +        else
> +            ret = wrmsr_safe(ra->data.idx, ra->data.val);
> +        break;

Did you mean these latter tow if()-s perhaps be "else if"?

> +    case XENPF_resource_op:
> +    {
> +        struct xen_resource_access ra;
> +        struct xenpf_resource_op *rsc_op = &op->u.resource_op;
> +        unsigned int i, j = 0, cpu = smp_processor_id();
> +
> +        for ( i = 0; i < rsc_op->nr; i++ )
> +        {
> +            if ( copy_from_guest_offset(&ra.data, rsc_op->data, i, 1) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            if ( ra.data.cpu == cpu )
> +                resource_access_one(&ra);
> +            else if ( cpu_online(ra.data.cpu) )
> +                on_selected_cpus(cpumask_of(ra.data.cpu),
> +                                 resource_access_one, &ra, 1);
> +            else
> +            {
> +                ret = -ENODEV;
> +                break;
> +            }
> +
> +            if ( ra.ret )
> +            {
> +                ret = ra.ret;
> +                break;
> +            }
> +
> +            if ( copy_to_guest_offset(rsc_op->data, i, &ra.data, 1) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            /* Find the start point that requires no preemption */
> +            if ( ra.data.flag && j == 0 )
> +                j = i;
> +            /* Set j = 0 when walking out of the non-preemption area */
> +            if ( ra.data.flag == 0 )
> +                j = 0;
> +            if ( hypercall_preempt_check() )
> +            {
> +                ret = hypercall_create_continuation(
> +                    __HYPERVISOR_platform_op, "ih",
> +                    ra.data.flag ? j : i, u_xenpf_op);

Which means everything starting from j will be re-executed
another time when continuing. That creates three problems: You
can't guarantee forwards progress, you may do something
having side effects more than once, and you break the operation
in a place that was requested to not be preemptible.

> +struct xenpf_resource_data {
> +    uint16_t cmd;       /* XEN_RESOURCE_OP_* */
> +    uint16_t flag;      /* avoid preemption between certain iterations */

You want to use just one bit out of this field (and verify all other bits
are zero, so they can get assigned a meaning later).

Jan

> +    uint32_t cpu;
> +    uint64_t idx;
> +    uint64_t val;
> +};



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