[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v13 01/10] x86: add generic resource (e.g. MSR) access hypercall
>>> On 04.08.14 at 04:16, <dongxiao.xu@xxxxxxxxx> 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |