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

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

>>> On 04.08.14 at 04:16, <dongxiao.xu@intel.com> wrote:
> +static unsigned int allow_access_msr(unsigned int msr)

bool_t.

> +static void resource_access_one(void *info)
> +{
> +    struct xen_resource_access *ra = info;
> +    int ret = 0;
> +
> +    switch ( ra->type )
> +    {
> +    case XEN_RESOURCE_TYPE_MSR:
> +        if ( !allow_access_msr((unsigned int)ra->data.idx) )

I think I said this before: Please _do not_ discard input data, but
instead check the full value the guest supplied.

> +            ret = -EACCES;
> +
> +        if ( ra->data.cmd == XEN_RESOURCE_OP_READ )
> +            ret = rdmsr_safe((unsigned int)ra->data.idx, ra->data.val);
> +        else if ( ra->data.cmd == XEN_RESOURCE_OP_WRITE )
> +            ret = wrmsr_safe((unsigned int)ra->data.idx, ra->data.val);

The casts here are unnecessary in any case.

> +        break;

Furthermore I'm also sure I already asked you to add another "else"
setting rc to -EINVAL here.

> +
> +    default:
> +        gdprintk(XENLOG_WARNING, "unsupported resource type: %d\n", ra->type);

And I'm similarly certain I previously pointed out that messages like
this may be okay in RFC submissions, but are rarely of any use in
what is intended to get committed. Please either explain why you
need a debugging message like this, or drop it.

> +        ret = -ENOSYS;

Let's try to avoid further abuses of -ENOSYS. That error value
should be used only for unimplemented hypercalls, not sub-ops
of implemented ones. If you don't want to use -EINVAL, which I
appreciate due to it being used in some many other places, use
-EOPNOTSUPP unless you can find an even better fit.

> +    case XENPF_resource_op:
> +    {
> +        struct xen_resource_access ra;
> +        struct xenpf_resource_op *rsc_op = &op->u.resource_op;
> +        unsigned int i, cpu = smp_processor_id();
> +
> +        ra.type = rsc_op->type;
> +        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 ( copy_to_guest_offset(rsc_op->data, i, &ra.data, 1) )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            if ( hypercall_preempt_check() )
> +            {
> +                ret = hypercall_create_continuation(
> +                    __HYPERVISOR_platform_op, "h", u_xenpf_op);

You're losing the value of "i" here, making it impossible to resume at
the right array slot. Furthermore I'm _still_ missing the flag suppressing
preemption between two particular iterations. Finally you're also
dropping ra.ret on the floor.

As to you avoiding continue_hypercall_on_cpu(): The way it's being
done is perhaps indeed fine for a first cut, provided Andrew isn't
concerned anymore about the extra IPIs resulting from you doing
each iteration with one.

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -527,6 +527,29 @@ struct xenpf_core_parking {
>  typedef struct xenpf_core_parking xenpf_core_parking_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
>  
> +#define XENPF_resource_op   61
> +
> +#define XEN_RESOURCE_OP_READ  0
> +#define XEN_RESOURCE_OP_WRITE 1
> +
> +#define XEN_RESOURCE_TYPE_MSR  0

I think this should just be XEN_RESOURCE_OP_MSR_{READ,WRITE}...

> +struct xenpf_resource_data {
> +    uint32_t cmd;       /* XEN_RESOURCE_OP_* */
> +    uint32_t cpu;
> +    uint64_t idx;
> +    uint64_t val;
> +};
> +typedef struct xenpf_resource_data xenpf_resource_data_t;
> +DEFINE_XEN_GUEST_HANDLE(xenpf_resource_data_t);
> +struct xenpf_resource_op {

[Blank line above this one.]

> +    uint32_t nr;
> +    uint32_t type;      /* XEN_RESOURCE_TYPE_* */

... at once eliminating this field and allowing to mix different resource
types in a single invocation.

> +    XEN_GUEST_HANDLE(xenpf_resource_data_t) data;
> +};

I guess for ease of implementation you'll want to violate the rule
of not modifying the interface structures, and update nr and data.
This violation then needs to be clearly documented in a comment
here.

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -88,6 +88,7 @@
>  ?	xenpf_enter_acpi_sleep		platform.h
>  ?	xenpf_pcpuinfo			platform.h
>  ?	xenpf_pcpu_version		platform.h
> +?	xenpf_resource_op		platform.h

This is pointless without invoking the resulting CHECK_* macro. At
which point you'd learn that the two structures aren't identical
(structures containing XEN_GUEST_HANDLE() instances never will
be). But you only need to check struct xenpf_resource_data
anyway, as the body of do_platform_op() gets built twice (native
and compat) already.

Jan