Re: [RFC PATCH 03/31] pmstat: move pmstat.c file to the xen/drivers/pm/stat.c location

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

Hi, Jan.

Sorry for the late response.

On Mon, May 7, 2018 at 6:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 09.11.17 at 18:09, <olekstysh@gmail.com> wrote:
>> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>>
>> Cpufreq driver should be more generalizable (not ACPI-specific).
>> Thus this file should be placed to more convenient location.
>>
>> This is a rebased version of the original patch:
>> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00935.html
>>
>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@linaro.org>
>> ---
>>  MAINTAINERS               |   1 +
>>  xen/arch/x86/Kconfig      |   1 +
>>  xen/common/sysctl.c       |   2 +-
>>  xen/drivers/Kconfig       |   2 +
>>  xen/drivers/Makefile      |   1 +
>>  xen/drivers/acpi/Makefile |   1 -
>>  xen/drivers/acpi/pmstat.c | 526 ----------------------------------------------
>>  xen/drivers/pm/Kconfig    |   3 +
>>  xen/drivers/pm/Makefile   |   1 +
>>  xen/drivers/pm/stat.c     | 526 ++++++++++++++++++++++++++++++++++++++++++++++
>
> I think I'd prefer drivers/power/*, and please try present movement of files as
> renames instead of as delete+create.
Will do.

>
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -23,6 +23,7 @@ config X86
>>       select HAS_PDX
>>       select NUMA
>>       select VGA
>> +     select HAS_PM
>
> Please insert at the right spot.
ok.

>
>> +int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
>> +{
>> +    u32 bits[3];
>> +    int ret;
>> +
>> +    if ( copy_from_guest(bits, pdc, 2) )
>> +        ret = -EFAULT;
>> +    else if ( bits[0] != ACPI_PDC_REVISION_ID || !bits[1] )
>> +        ret = -EINVAL;
>> +    else if ( copy_from_guest_offset(bits + 2, pdc, 2, 1) )
>> +        ret = -EFAULT;
>> +    else
>> +    {
>> +        u32 mask = 0;
>> +
>> +        if ( xen_processor_pmbits & XEN_PROCESSOR_PM_CX )
>> +            mask |= ACPI_PDC_C_MASK | ACPI_PDC_SMP_C1PT;
>> +        if ( xen_processor_pmbits & XEN_PROCESSOR_PM_PX )
>> +            mask |= ACPI_PDC_P_MASK | ACPI_PDC_SMP_C1PT;
>> +        if ( xen_processor_pmbits & XEN_PROCESSOR_PM_TX )
>> +            mask |= ACPI_PDC_T_MASK | ACPI_PDC_SMP_C1PT;
>> +        bits[2] &= (ACPI_PDC_C_MASK | ACPI_PDC_P_MASK | ACPI_PDC_T_MASK |
>> +                    ACPI_PDC_SMP_C1PT) & ~mask;
>> +        ret = arch_acpi_set_pdc_bits(acpi_id, bits, mask);
>> +    }
>> +    if ( !ret && __copy_to_guest_offset(pdc, 2, bits + 2, 1) )
>> +        ret = -EFAULT;
>> +
>> +    return ret;
>> +}
>
> Looks quite ACPI-specific.
Yes, current patch does just a movement.

Next patch [1] wraps it in #ifdef CONFIG_ACPI.

However during patch discussion we decided to move this function to arch/x86.
It is called from arch/x86/platform_hypercall.c and pulls a bunch of
#define-s from pdc_intel.h

Sounds ok?

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-11/msg00651.html

>
> Jan
>
>

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel