Re: [Xen-devel] [RFC PATCH v2 09/12] xen: arm: implement platform hypercall

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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.

Jan


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