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

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

>>> On 02.10.14 at 13:35, <chao.p.peng@linux.intel.com> wrote:
> +static void 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 = -EOPNOTSUPP;
> +            break;
> +        }
> +
> +        if ( ret )
> +        {
> +           entry->u.ret = ret;
> +           break;
> +        }
> +    }
> +
> +    ra->idx_done = i;

This is slightly abusing the field (considering its name) when all
entries succeeded, but I guess we can live with that. Beyond
that I have only cosmetic remarks to make, which I could equally
well address when committing (unless other issue get noticed by
someone else):

> +static void resource_access(void *info)
> +{
> +    struct xen_resource_access *ra = info;
> +    xenpf_resource_entry_t *entry;
> +    int ret;

I'd prefer these two to be inside the loop (also again in the earlier
function).

> +    unsigned int i;
> +
> +    for ( i = 0; i < ra->idx_done; 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:
> +            BUG();
> +            break;
> +        }
> +
> +        entry->u.ret = ret;
> +        if ( ret )
> +           break;

I continue to wonder whether we wouldn't be better off not
overwriting the command in the success case.

> @@ -601,6 +685,80 @@ 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;

unsigned int

> +        XEN_GUEST_HANDLE(xenpf_resource_entry_t) guest_entries;
> +
> +        ra.nr_entries = op->u.resource_op.nr_entries;
> +        if ( ra.nr_entries == 0 )
> +        {
> +            ret = 0;
> +            break;
> +        }
> +        else if ( ra.nr_entries > RESOURCE_ACCESS_MAX_ENTRIES )

Pointless "else".

> +        {
> +            ret = -EINVAL;
> +            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. */
> +        check_resource_access(&ra);
> +        if ( ra.idx_done == 0 )
> +        {
> +            /* Copy the return value for entry 0 if it failed. */
> +            if ( __copy_to_guest_offset(guest_entries, 0, ra.entries, 1) )

__copy_to_guest()

> +                ret = -EFAULT;
> +            else
> +                ret = 0;
> +
> +            xfree(ra.entries);
> +            break;
> +        }
> +
> +        cpu = op->u.resource_op.cpu;
> +        if ( (cpu >= nr_cpu_ids) || !cpu_online(cpu) )
> +        {
> +            xfree(ra.entries);
> +            ret = -ENODEV;
> +            break;
> +        }
> +        else if ( cpu == smp_processor_id() )

Pointless "else" again.

> +            resource_access(&ra);
> +        else
> +            on_selected_cpus(cpumask_of(cpu), resource_access, &ra, 1);
> +
> +        /* Copy all if succeeded or up to the failed entry. */
> +        if ( __copy_to_guest_offset(guest_entries, 0, ra.entries,

__copy_to_guest()

> +                                    min(ra.nr_entries, ra.idx_done + 1)) )

I still think it would be more natural to code

ra.idx_done < ra.nr_entries ? ra.idx_done + 1 : ra.nr_entries

This has the same effect, but avoids the slight abuse of min() here
(and would look even more natural if idx_done was renamed to
nr_done or nr_valid).

> +        {
> +            xfree(ra.entries);
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        ret = ra.idx_done;

And this again is a slight abuse. Also, handling this as an "else" to the
preceding "if" would save you an xfree() invocation and a "break".

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -528,6 +528,40 @@ typedef struct xenpf_core_parking xenpf_core_parking_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>  
>  /*
> + * Access generic platform resources(e.g., accessing MSR, port I/O, etc)
> + * in unified way. Batch resource operations in one call are supported and
> + * thay are always non-preemptible and executed in their original order.

they

> + * The batch itself returns a negative integer for general errors, or a
> + * non-negative integer for the number of successful operations. For latter

For the latter

> + * case, the @ret in the failed entry(if have) indicates the exact error and

entry (if any)

> + * it's meaningful only when it has a negative value.

???

> + */
> +#define XENPF_resource_op   61
> +
> +#define XEN_RESOURCE_OP_MSR_READ  0
> +#define XEN_RESOURCE_OP_MSR_WRITE 1
> +
> +struct xenpf_resource_entry {
> +    union {
> +        uint32_t cmd;   /* IN: XEN_RESOURCE_OP_* */
> +        int32_t  ret;   /* OUT: return value for this entry */

Imprecise comment (not all entries have this set).

Jan