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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

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