Re: [Xen-devel] [RFC PATCH v3 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Thu, Oct 23, 2014 at 7:27 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Oleksandr,
>
> On 10/23/2014 04:07 PM, Oleksandr Dmytryshyn wrote:
>> Kernel uses this op to get some parameters for the
>> xen-cpufreq driver to change CPUs frequency.
>
> The XSM bit is missing for this new sysctl (see flask_sysctl).
I'll introduce the XSM bit for this new sysctl. But I see that file
xen/xsm/flask/hooks.c is not compiled for x86 and ARM architectures.
It uses bits (for example XEN__PHYSINFO) which are not defined anywhere.

>> diff --git a/xen/drivers/cpufreq/hwdom-cpufreq.c 
>> b/xen/drivers/cpufreq/hwdom-cpufreq.c
>> index 67c9e1d..cc97f37 100644
>> --- a/xen/drivers/cpufreq/hwdom-cpufreq.c
>> +++ b/xen/drivers/cpufreq/hwdom-cpufreq.c
>> @@ -34,12 +34,53 @@ struct hwdom_cpufreq_data {
>>  };
>>
>>  static struct hwdom_cpufreq_data *hwdom_cpufreq_drv_data[NR_CPUS];
>> +static DEFINE_SPINLOCK(sysctl_cpufreq_lock);
>> +
>> +struct sysctl_cpufreq_data {
>> +    uint32_t cpu;
>> +    uint32_t freq;
>> +    uint32_t relation;
>> +    int32_t result;
>> +};
>> +
>> +static struct sysctl_cpufreq_data sysctl_cpufreq_data;
>>
>>  int cpufreq_cpu_init(unsigned int cpuid)
>>  {
>>      return cpufreq_add_cpu(cpuid);
>>  }
>>
>> +static void notify_cpufreq_domains(void)
>> +{
>> +    send_global_virq(VIRQ_CPUFREQ);
>> +}
>> +
>> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op)
>> +{
>> +    int ret = 0;
>
> AFAIU, the cpufreq will always run in the hardware domain. Therefore,
> shouldn't you check that the sysctl is effectively executed by this domain?
I'll try to do this in the next patch set.

>>  static int hwdom_cpufreq_verify(struct cpufreq_policy *policy)
>>  {
>>      struct hwdom_cpufreq_data *data;
>> @@ -97,6 +138,17 @@ static int hwdom_cpufreq_target(struct cpufreq_policy 
>> *policy,
>>      freqs.old = perf->states[perf->state].core_frequency * 1000;
>>      freqs.new = data->freq_table[next_state].frequency;
>>
>> +    /* Do send cmd for Dom0 */
>
> s/Dom0/Hardware domain/
I'll fix this in the next patch set.

>> +    spin_lock(&sysctl_cpufreq_lock);
>> +    /* return previous result */
>> +    ret = sysctl_cpufreq_data.result;
>> +
>> +    sysctl_cpufreq_data.cpu = policy->cpu;
>> +    sysctl_cpufreq_data.freq = freqs.new;
>> +    sysctl_cpufreq_data.relation = (uint32_t)relation;
>> +    spin_unlock(&sysctl_cpufreq_lock);
>> +    notify_cpufreq_domains();
>> +
>>      for_each_cpu( j, &online_policy_cpus )
>>          cpufreq_statistic_update(j, perf->state, next_perf_state);
>>
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index 8437d31..ecd4674 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -632,6 +632,23 @@ struct xen_sysctl_coverage_op {
>>  typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
>>
>> +#define XEN_SYSCTL_CPUFREQ_get_target      0
>> +#define XEN_SYSCTL_CPUFREQ_set_result      1
>> +
>> +struct xen_sysctl_cpufreq_op {
>> +    uint32_t cmd;
>> +    union {
>> +        struct {
>> +            uint32_t cpu;
>> +            uint32_t freq;
>> +            uint32_t relation;
>> +        } target;
>> +        uint32_t result;
>> +    } u;
>> +};
>
> Can you document this structure?
I'll do this in the next patch set.

> Regards,
>
> --
> Julien Grall

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