ARM Xen Bug #45: Is there a solution?

Commit Message

Dirk Behme May 27, 2016, 12:34 p.m. UTC

On 26.05.2016 11:00, Julien Grall wrote:
> On 25/05/2016 16:10, Dirk Behme wrote:
>> On 24.05.2016 22:05, Julien Grall wrote:
>>> On 24/05/2016 14:39, Dirk Behme wrote:
>>>> On 23.05.2016 22:15, Julien Grall wrote:
>>>>> Hello Dirk,
>>>>>
>>>>>> is there a solution for
>>>>>>
>>>>>> arm: domain 0 disables clocks which are in fact being used
>>>>>> http://bugs.xenproject.org/xen/bug/45
>>>>>>
>>>>>> ?
>>>>>>
>>>>>> On an ARM based board I have to use 'clk_ignore_unused' preventing
>>>>>> that
>>>>>> Dom0 disables the UART clock for the console UART configured with
>>>>>> console=hvc0.
>>>>>
>>>>> There is no better solution than passing "clk_ignore_unused" on the
>>>>> kernel command line so far.
>>>>
>>>>
>>>> What would be the solution for this issue? The
>>>>
>>>> "propagate any clock related properties from the UART
>>>> node into the Xen hypervisor node"
>>>>
>>>> mentioned in the ticket?
>>>
>>> That is correct. Xen would copy the property "clocks" of the UART into
>>> the hypervisor node.
>>>
>>> DOM0 would then parse the clocks associated to this node and mark them
>>> as used by Xen (I think CLK_IGNORE_UNUSED could do the job for us).
>>
>>
>> I've started to look into this:
>>
>> I'd think in arm_uart.c in dt_uart_init() after
>>
>> if ( !dev )
>>
>> we know the UART node we are looking for. From this we have to read the
>> clock configuration.
>>
>> To be clarified: How to read the clock configuration? I couldn't find
>> any convenient function dt_device_get_clock() or similar for that.
>
> Xen does not need to parse the content of the property "clocks" but
> copy the raw value to the DOM0 DT.
>
> You cand find the value of a property with dt_get_property.
>
>>
>> Now, we have the clocks we are looking for.
>>
>> These are needed in domain_build.c in make_hypervisor_node(), then.
>>
>> To be clarified: How to pass the clock configuration from arm_uart.c to
>> domain_build.c (and not break the non-dt / non-ARM platforms)?
>>
>> Any ideas or comments?
>
> All the devices (UART included) used by Xen will return DOMID_XEN when
> dt_device_used_by is called to the node.
>
> You could use it to collect the clocks of all those devices and gather
> the value in a single property to be created in the hypervisor node.


Anything like below (untested) [1]?

I'm unhappy about the global variables and the max clocks, though.

Best regards

Dirk

[1]

---
  xen/arch/arm/domain_build.c |   24 ++++++++++++++++++++++++
  1 file changed, 24 insertions(+)

Comments

Julien Grall May 31, 2016, 10:44 a.m. UTC | #1

Hello Dirk,

On 27/05/16 13:34, Dirk Behme wrote:
> On 26.05.2016 11:00, Julien Grall wrote:
>> On 25/05/2016 16:10, Dirk Behme wrote:
>>> On 24.05.2016 22:05, Julien Grall wrote:
>>>> On 24/05/2016 14:39, Dirk Behme wrote:
>>>>> On 23.05.2016 22:15, Julien Grall wrote:
>> All the devices (UART included) used by Xen will return DOMID_XEN when
>> dt_device_used_by is called to the node.
>>
>> You could use it to collect the clocks of all those devices and gather
>> the value in a single property to be created in the hypervisor node.
>
>
> Anything like below (untested) [1]?

Yes.

> I'm unhappy about the global variables and the max clocks, though.

How about moving those variables in the structure kernel_info?

>
> Best regards
>
> Dirk
>
> [1]
>
> ---
>   xen/arch/arm/domain_build.c |   24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> Index: xen.git/xen/arch/arm/domain_build.c
> ===================================================================
> --- xen.git.orig/xen/arch/arm/domain_build.c
> +++ xen.git/xen/arch/arm/domain_build.c
> @@ -42,6 +42,10 @@ static void __init parse_dom0_mem(const
>   }
>   custom_param("dom0_mem", parse_dom0_mem);
>
> +#define MAX_DT_CLOCKS 256
> +static unsigned char dt_clocks[MAX_DT_CLOCKS];
> +static unsigned int clk_cnt;
> +
>   //#define DEBUG_DT
>
>   #ifdef DEBUG_DT
> @@ -657,6 +661,10 @@ static int make_hypervisor_node(const st
>       if ( res )
>           return res;
>
> +    res = fdt_property(fdt, "clocks", dt_clocks, clk_cnt);
> +    if ( res )
> +        return res;
> +
>       res = fdt_end_node(fdt);
>
>       return res;
> @@ -1213,9 +1221,11 @@ static int handle_node(struct domain *d,
>           { /* sentinel */ },
>       };
>       struct dt_device_node *child;
> +    unsigned int len;
>       int res;
>       const char *name;
>       const char *path;
> +    const char *clocks;
>
>       path = dt_node_full_name(node);
>
> @@ -1246,6 +1256,20 @@ static int handle_node(struct domain *d,
>       if ( dt_device_used_by(node) == DOMID_XEN )
>       {
>           DPRINT("  Skip it (used by Xen)\n");
> +
> +        /*
> +         * Remember the clock used by the skipped node
> +         * We add it later to the hypervisor node to make the
> +         * Linux kernel aware of its usage
> +         */
> +        clocks = dt_get_property(node, "clocks", &len);
> +        if ( clk_cnt + len >= MAX_DT_CLOCKS ) {
> +            printk("Failed to remember the clock node of %s\n", path);
> +            printk("Use the Linux kernel command 'clk_ignore_unused'\n");
> +            return 0;
> +        }
> +        memcpy(&dt_clocks[clk_cnt], clocks, len);
> +        clk_cnt += len;
>           return 0;
>       }
>

Dirk Behme June 21, 2016, 10:27 a.m. UTC | #2

> Subject: Re: ARM Xen Bug #45: Is there a solution?
> Date: Tue, 31 May 2016 11:44:23 +0100
> From: Julien Grall <[e-mail]>
> To: Dirk Behme <[e-mail]>, [e-mail]
> <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>, Ian Jackson
> <[e-mail]>
>
> Hello Dirk,
>
> On 27/05/16 13:34, Dirk Behme wrote:
>> On 26.05.2016 11:00, Julien Grall wrote:
>>> On 25/05/2016 16:10, Dirk Behme wrote:
>>>> On 24.05.2016 22:05, Julien Grall wrote:
>>>>> On 24/05/2016 14:39, Dirk Behme wrote:
>>>>>> On 23.05.2016 22:15, Julien Grall wrote:
>>> All the devices (UART included) used by Xen will return DOMID_XEN when
>>> dt_device_used_by is called to the node.
>>>
>>> You could use it to collect the clocks of all those devices and gather
>>> the value in a single property to be created in the hypervisor node.
>>
>>
>> Anything like below (untested) [1]?
>
> Yes.
>
>> I'm unhappy about the global variables and the max clocks, though.
>
> How about moving those variables in the structure kernel_info?


Just for the logs to finish this thread:

https://lists.xen.org/archives/html/xen-devel/2016-06/msg02607.html

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438067.html


Best regards

Dirk

Patch

Index: xen.git/xen/arch/arm/domain_build.c
===================================================================
--- xen.git.orig/xen/arch/arm/domain_build.c
+++ xen.git/xen/arch/arm/domain_build.c
@@ -42,6 +42,10 @@  static void __init parse_dom0_mem(const
  }
  custom_param("dom0_mem", parse_dom0_mem);

+#define MAX_DT_CLOCKS 256
+static unsigned char dt_clocks[MAX_DT_CLOCKS];
+static unsigned int clk_cnt;
+
  //#define DEBUG_DT

  #ifdef DEBUG_DT
@@ -657,6 +661,10 @@  static int make_hypervisor_node(const st
      if ( res )
          return res;

+    res = fdt_property(fdt, "clocks", dt_clocks, clk_cnt);
+    if ( res )
+        return res;
+
      res = fdt_end_node(fdt);

      return res;
@@ -1213,9 +1221,11 @@  static int handle_node(struct domain *d,
          { /* sentinel */ },
      };
      struct dt_device_node *child;
+    unsigned int len;
      int res;
      const char *name;
      const char *path;
+    const char *clocks;

      path = dt_node_full_name(node);

@@ -1246,6 +1256,20 @@  static int handle_node(struct domain *d,
      if ( dt_device_used_by(node) == DOMID_XEN )
      {
          DPRINT("  Skip it (used by Xen)\n");
+
+        /*
+         * Remember the clock used by the skipped node
+         * We add it later to the hypervisor node to make the
+         * Linux kernel aware of its usage
+         */
+        clocks = dt_get_property(node, "clocks", &len);
+        if ( clk_cnt + len >= MAX_DT_CLOCKS ) {
+            printk("Failed to remember the clock node of %s\n", path);
+            printk("Use the Linux kernel command 'clk_ignore_unused'\n");
+            return 0;
+        }
+        memcpy(&dt_clocks[clk_cnt], clocks, len);
+        clk_cnt += len;
          return 0;
      }