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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Wed, Oct 22, 2014 at 12:24 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 22.10.14 at 10:40, <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx> wrote:
>> 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.
>
> This all looks very kludgy and hence is likely going to become a
> maintenance nightmare. Please separate things properly that are
> separate by design.
Yes, It all looks very kludgy.
But now ->u.set_pminfo can be reused for non-ACPI cases.
set_px_pminfo() function can be reused for non-ACPI cases too.
Thus this function is not only ACPI-specific. This function can be
CPUFREQ-specific in this context.

> Jan
>

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