[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 09/12] xen: arm: implement platform hypercall
On Fri, Oct 17, 2014 at 3:44 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote: >>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx> wrote: >> +long arch_do_platform_op(struct xen_platform_op *platform_op, >> + XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) >> u_xenpf_op) >> +{ >> + long ret = 0; >> + struct xen_platform_op *op = platform_op; >> + >> + switch ( op->cmd ) >> + { >> + case XENPF_set_processor_pminfo: >> + switch ( op->u.set_pminfo.type ) >> + { >> + case XEN_PM_PX: >> + if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) ) >> + { >> + ret = -ENOSYS; >> + break; >> + } >> +#ifdef HAS_CPUFREQ >> + ret = set_px_pminfo(op->u.set_pminfo.id, >> &op->u.set_pminfo.u.perf); > > This cannot possibly be HAS_CPUFREQ, since ->u.set_pminfo > specifically uses ACPI notation for e.g. the CPU numbering. And > with that it becomes questionable whether the whole series is > doing right in abstracting away ACPI. set_px_pminfo() function is implemented in the cpufreq.c file and this file will be compiled only if HAS_CPUFREQ is defined. Thus I've used HAS_CPUFREQ in this case. If HAS_CPUFREQ is not defined then set_px_pminfo() will not be compiled and there will be errors on compilation without this definition. ->u.set_pminfo uses ACPI notation for the CPU numbering but patch "cpufreq: make cpufreq driver more generalizable" skips ACPI notation for the CPU numbering if CONFIG_ACPI is not defined. >> @@ -37,6 +38,7 @@ CHECK_pf_enter_acpi_sleep; >> #define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t) >> typedef int ret_t; >> >> +#include "../../../common/platform_hypercall.c" >> #include "../platform_hypercall.c" > > This needs to be done properly in common/. > >> +extern ret_t >> +arch_do_platform_op( >> + struct xen_platform_op *platform_op, >> + XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op); > > Declarations belong in header files except in very special cases > (which this is none of). > >> +ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op) >> +{ >> + ret_t ret = 0; >> + struct xen_platform_op curop, *op = &curop; >> + >> + if ( copy_from_guest(op, u_xenpf_op, 1) ) >> + return -EFAULT; >> + >> + if ( op->interface_version != XENPF_INTERFACE_VERSION ) >> + return -EACCES; >> + >> + ret = xsm_platform_op(XSM_PRIV, op->cmd); >> + if ( ret ) >> + return ret; >> + >> + /* >> + * Trylock here avoids deadlock with an existing platform critical >> section >> + * which might (for some current or future reason) want to synchronise >> + * with this vcpu. >> + */ >> + while ( !spin_trylock(&xenpf_lock) ) >> + if ( hypercall_preempt_check() ) >> + return hypercall_create_continuation( >> + __HYPERVISOR_platform_op, "h", u_xenpf_op); >> + >> + ret = arch_do_platform_op(op, u_xenpf_op); >> + >> + spin_unlock(&xenpf_lock); > > And seeing this I really wonder whether making this common is > really worthwhile. I'll create separate file for platform_hypercall without common code (as it was done for physdev_op) > Jan > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |