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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.09.14 at 12:49, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> +static unsigned int check_resource_access(struct xen_resource_access *ra)
> +{
> +    xenpf_resource_entry_t *entry;
> +    int ret = 0;
> +    unsigned int i;
> +
> +    for ( i = 0; i < ra->nr_entries; i++ )
> +    {
> +        entry = ra->entries + i;
> +
> +        if ( entry->rsvd )
> +        {
> +            entry->u.ret = -EINVAL;
> +            break;
> +        }
> +
> +        switch ( entry->u.cmd )
> +        {
> +        case XEN_RESOURCE_OP_MSR_READ:
> +        case XEN_RESOURCE_OP_MSR_WRITE:
> +            if ( entry->idx >> 32 )
> +                ret = -EINVAL;
> +            else if ( !allow_access_msr(entry->idx) )
> +                ret = -EACCES;
> +            break;
> +        default:
> +            ret = -EINVAL;

-EOPNOTSUPP or any other suitable but more specific error code
than -EINVAL.

> +            break;
> +        }
> +
> +        if ( ret )
> +        {
> +           entry->u.ret = ret;
> +           break;
> +        }

Other than Andrew said, this is okay (thus retaining the ->u.cmd
value for the success case).

> +    }
> +
> +    /* Return the number of successes. */
> +    return i;
> +}
> +
> +static void resource_access(void *info)
> +{
> +    struct xen_resource_access *ra = info;
> +    xenpf_resource_entry_t *entry;
> +    int ret;
> +    unsigned int i;
> +
> +    for ( i = 0; i < ra->nr_entries; i++ )
> +    {
> +        entry = ra->entries + i;
> +
> +        switch ( entry->u.cmd )
> +        {
> +        case XEN_RESOURCE_OP_MSR_READ:
> +            ret = rdmsr_safe(entry->idx, entry->val);
> +            break;
> +        case XEN_RESOURCE_OP_MSR_WRITE:
> +            ret = wrmsr_safe(entry->idx, entry->val);
> +            break;
> +        default:
> +            ret = -EINVAL;
> +            break;

BUG(). You checked invalid cmd-s already above.

> +        }
> +
> +        if ( ret )
> +        {
> +           entry->u.ret = ret;
> +           break;
> +        }

Here you indeed should update ->u.ret unconditionally if we really
want success to be indicated here too. I'm not sure this is needed
though, since the return value of the hypercall should indicate the
slot where to look for the op-specific error code.

> @@ -601,6 +689,75 @@ ret_t 
> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>      }
>      break;
>  
> +    case XENPF_resource_op:
> +    {
> +        struct xen_resource_access ra;
> +        uint32_t cpu;
> +        XEN_GUEST_HANDLE(xenpf_resource_entry_t) guest_entries;
> +
> +        ra.nr_entries = op->u.resource_op.nr_entries;
> +        if ( ra.nr_entries == 0 || ra.nr_entries > 
> RESOURCE_ACCESS_MAX_ENTRIES )
> +        {
> +            ret = -EINVAL;

I don't think ra.nr_entries == 0 is a reason to fail the hypercall.

> +            break;
> +        }
> +
> +        ra.entries = xmalloc_array(xenpf_resource_entry_t, ra.nr_entries);
> +        if ( !ra.entries )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        guest_from_compat_handle(guest_entries, op->u.resource_op.entries);
> +
> +        if ( copy_from_guest(ra.entries, guest_entries, ra.nr_entries) )
> +        {
> +            xfree(ra.entries);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        /* Do sanity check earlier to omit the potential IPI overhead. */
> +        if ( check_resource_access(&ra) < ra.nr_entries )
> +        {
> +            /* Copy the return value for failed entry. */
> +            if ( __copy_to_guest_offset(guest_entries, ret,
> +                                        ra.entries + ret, 1) )
> +                ret = -EFAULT;
> +            else
> +                ret = 0;

This should be the index of the failed entry. I guess it would be
easier and more consistent if check_resource_access() too used
ra.ret for passing back the failed index (which btw should be
renamed to e.g. "done" - "ret" is no longer a suitable name).

> +
> +            xfree(ra.entries);
> +            break;
> +        }
> +
> +        cpu = op->u.resource_op.cpu;
> +        if ( cpu == smp_processor_id() )
> +            resource_access(&ra);
> +        else if ( cpu_online(cpu) )

This continues to be wrong.

> +            on_selected_cpus(cpumask_of(cpu), resource_access, &ra, 1);
> +        else
> +        {
> +            xfree(ra.entries);
> +            ret = -ENODEV;
> +            break;
> +        }
> +
> +        /* Copy all if succeeded or up to the failed entry. */
> +        if ( __copy_to_guest_offset(guest_entries, 0, ra.entries,
> +                                    min(ra.nr_entries, ra.ret + 1)) )

I don't see a need for min() here - ra.ret mustn't be out of range.
If you're concerned, add an ASSERT().

Jan

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