Re: [Xen-devel] [PATCH v11 1/9] x86: add generic MSR access hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.14 at 16:31, <dongxiao.xu@xxxxxxxxx> wrote:
> Add a generic MSR access hypercall for tool stack or other components to
> read or write certain system MSRs.

If you want this to be usable by components other than the tool stack
then this can't be a sysctl (would e.g. need to be a platform-op then).

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -66,10 +66,27 @@ void arch_do_physinfo(xen_sysctl_physinfo_t *pi)
>          pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm_directio;
>  }
>  
> +static void msr_access_helper(void *data)
> +{
> +    struct msr_access_info *info = data;
> +    xen_sysctl_msr_data_t *msr_data;
> +    unsigned int i;
> +
> +    for ( i = 0; i < info->nr_ops; i++ )
> +    {
> +        msr_data = &info->msr_data[i];
> +        if ( msr_data->cmd == XEN_SYSCTL_MSR_read )
> +            rdmsrl(msr_data->msr, msr_data->val);
> +        else if ( msr_data->cmd == XEN_SYSCTL_MSR_write )
> +            wrmsrl(msr_data->msr, msr_data->val);
> +    }

Missing preemption check.

> @@ -101,11 +118,48 @@ long arch_do_sysctl(
>      }
>      break;
>  
> +    case XEN_SYSCTL_msr_op:
> +    {
> +        unsigned int cpu = sysctl->u.msr_op.cpu;
> +        struct msr_access_info info;
> +
> +        info.nr_ops = sysctl->u.msr_op.nr_ops;
> +        info.msr_data = xzalloc_array(xen_sysctl_msr_data_t, info.nr_ops);
> +        if ( !info.msr_data )
> +            return -ENOMEM;
> +
> +        if ( copy_from_guest(info.msr_data, sysctl->u.msr_op.msr_data,
> +                             info.nr_ops) )
> +        {
> +            xfree(info.msr_data);
> +            return -EFAULT;
> +        }
> +
> +        if ( cpu == smp_processor_id() )
> +            msr_access_helper(&info);
> +        else
> +            on_selected_cpus(cpumask_of(cpu), msr_access_helper, &info, 1);
> +
> +        if ( copy_to_guest(sysctl->u.msr_op.msr_data, info.msr_data,
> +                           info.nr_ops ) )
> +        {
> +            xfree(info.msr_data);
> +            return -EFAULT;
> +        }
> +
> +        xfree(info.msr_data);
> +        copyback = 1;

I don't see what you're meaning to copy back here.

Anyway, together with the comments on the interface itself (further
down) I think you'd be much better off basing this on
continue_hypercall_on_cpu().

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -636,6 +636,29 @@ struct xen_sysctl_coverage_op {
>  typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
>  
> +#define XEN_SYSCTL_MSR_read                0
> +#define XEN_SYSCTL_MSR_write               1
> +
> +struct xen_sysctl_msr_data {
> +    uint32_t cmd;       /* XEN_SYSCTL_MSR_* */
> +    uint32_t msr;
> +    uint64_t val;
> +};
> +typedef struct xen_sysctl_msr_data xen_sysctl_msr_data_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_msr_data_t);
> +struct xen_sysctl_msr_op {
> +    uint32_t nr_ops;

Jusr "nr" would suffice.

> +    uint32_t cpu;

This is rather limiting - you should be able to specify a CPU for
each operation. That way you'll have another 32-bit field available
for flags: You indicated earlier on that you'd want certain
operations (like a write followed by a read) pairable without
potentially getting preempted in between. The field then freed up
here should then be reserved for use as flags field too (i.e. you'd
need to check that it's zero).

> +    XEN_GUEST_HANDLE_64(xen_sysctl_msr_data_t) msr_data;
> +};
> +typedef struct xen_sysctl_msr_op xen_sysctl_msr_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_msr_op_t);

I think there shouldn't be any mention of "MSR" in other than the
specific sub-op names. As already said earlier, I see this also as
a vehicle to potentially to port accesses (namely when two of them
need to be done in close succession, or one needs to be carried
out on a particular CPU).

> +struct msr_access_info {
> +    uint32_t nr_ops;
> +    xen_sysctl_msr_data_t *msr_data;
> +};

This can only be misplaced - there are no pointers in public headers.

Jan

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