[PATCH v2 0/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[PATCH v2 0/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

I'm sending this as a v2 considering that I previously posted a diff
in email discussions.

This patch fixes an DT problem with the ZynqMP PCIe node.
Today, we try to remap IRQs regardless of if they are directly
connected to the primary controller or not. If they are indirectly
connected via secondary IRQ controllers, we abort the boot with
an error.

The ZynqMP PCIe DTS bindings were upstreamed to Linux after Xen 4.6, so
this issue is not a regression.
PCIe is also not generally a critical feature of current ZynqMP boards,
i.e the boards are functional without PCIe although for some users PCIe
may be more or less critical.

As I see it we have two options:

1. A safe option is to disable the PCIe node in the ZynqMP platform.
   We can then fix this with calm for 4.8.
   + It will fix the dom0 boot.
   + Very safe and unintrusive.
   - PCIe will not be functional.

2. Apply this patch to 4.7
   + It will fix the dom0 boot.
   + PCIe will be functional.
   - Intrusive fix. Although the fix really only affects a case that
     otherwise would have resulted in an aborted boot of dom0.

I'm happy to submit a patch for option nr 1 if you guys feel that's
safer/better at this point.

Best regards,
Edgar

ChangeLog:

v1 -> v2:
* Rephrased comment no longer mentioning bus-bridges.
* Fixed coding style issues.
* Added a dt_dprintk to log skipped IRQs.


Edgar E. Iglesias (1):
  xen/device-tree: Do not remap IRQs for secondary IRQ controllers

 xen/common/device_tree.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.5.0


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

[PATCH v2 1/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Do not remap IRQs connected to secondary interrupt controllers.
These IRQs have no meaning to us until they connect to the
primary controller.

Secondary IRQ controllers will at some point connect to the
primary controller (possibly via other IRQ controllers). We
map the IRQs at that last connection point.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/common/device_tree.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 0ed86a7..02a7ede 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1176,6 +1176,22 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
         for ( i = 0; i < pintsize; i++ )
             dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1);
 
+        if ( dt_raw_irq.controller != dt_interrupt_controller )
+        {
+            /* We don't map IRQs connected to secondary IRQ controllers as
+             * these IRQs have no meaning to us until they connect to the
+             * primary controller.
+             *
+             * Secondary IRQ controllers will at some point connect to
+             * the primary controller (possibly via other IRQ controllers).
+             * We map the IRQs at that last connection point.
+             */
+            imap += pintsize;
+            imaplen -= pintsize;
+            dt_dprintk(" -> Skipped IRQ for secondary IRQ controller\n");
+            continue;
+        }
+
         ret = dt_irq_translate(&dt_raw_irq, &dt_irq);
         if ( ret )
         {
-- 
2.5.0


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

Re: [PATCH v2 0/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[Wei Liu]

On Mon, May 16, 2016 at 05:03:54PM +0200, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> I'm sending this as a v2 considering that I previously posted a diff
> in email discussions.
> 
> This patch fixes an DT problem with the ZynqMP PCIe node.
> Today, we try to remap IRQs regardless of if they are directly
> connected to the primary controller or not. If they are indirectly
> connected via secondary IRQ controllers, we abort the boot with
> an error.
> 
> The ZynqMP PCIe DTS bindings were upstreamed to Linux after Xen 4.6, so
> this issue is not a regression.
> PCIe is also not generally a critical feature of current ZynqMP boards,
> i.e the boards are functional without PCIe although for some users PCIe
> may be more or less critical.
> 
> As I see it we have two options:
> 
> 1. A safe option is to disable the PCIe node in the ZynqMP platform.
>    We can then fix this with calm for 4.8.
>    + It will fix the dom0 boot.
>    + Very safe and unintrusive.
>    - PCIe will not be functional.
> 
> 2. Apply this patch to 4.7
>    + It will fix the dom0 boot.
>    + PCIe will be functional.
>    - Intrusive fix. Although the fix really only affects a case that
>      otherwise would have resulted in an aborted boot of dom0.
> 
> I'm happy to submit a patch for option nr 1 if you guys feel that's
> safer/better at this point.
> 

I'm a bit wary that this patch touches common code because the issue you
describe seems to be board specific. It's also unclear why this would
make PCIe function on ZynqMP from the commit message. 

I admit I don't know much about ARM so I will leave the further review
and judgement to ARM maintainers. I don't think OSSTest will pick up bug
in this patch FWIW.

Wei.

> Best regards,
> Edgar
> 
> ChangeLog:
> 
> v1 -> v2:
> * Rephrased comment no longer mentioning bus-bridges.
> * Fixed coding style issues.
> * Added a dt_dprintk to log skipped IRQs.
> 
> 
> Edgar E. Iglesias (1):
>   xen/device-tree: Do not remap IRQs for secondary IRQ controllers
> 
>  xen/common/device_tree.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> -- 
> 2.5.0
> 

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

Re: [PATCH v2 0/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[Julien Grall]

Hi Wei,

On 16/05/16 16:47, Wei Liu wrote:
> On Mon, May 16, 2016 at 05:03:54PM +0200, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>
>> I'm sending this as a v2 considering that I previously posted a diff
>> in email discussions.
>>
>> This patch fixes an DT problem with the ZynqMP PCIe node.
>> Today, we try to remap IRQs regardless of if they are directly
>> connected to the primary controller or not. If they are indirectly
>> connected via secondary IRQ controllers, we abort the boot with
>> an error.
>>
>> The ZynqMP PCIe DTS bindings were upstreamed to Linux after Xen 4.6, so
>> this issue is not a regression.
>> PCIe is also not generally a critical feature of current ZynqMP boards,
>> i.e the boards are functional without PCIe although for some users PCIe
>> may be more or less critical.
>>
>> As I see it we have two options:
>>
>> 1. A safe option is to disable the PCIe node in the ZynqMP platform.
>>     We can then fix this with calm for 4.8.
>>     + It will fix the dom0 boot.
>>     + Very safe and unintrusive.
>>     - PCIe will not be functional.
>>
>> 2. Apply this patch to 4.7
>>     + It will fix the dom0 boot.
>>     + PCIe will be functional.
>>     - Intrusive fix. Although the fix really only affects a case that
>>       otherwise would have resulted in an aborted boot of dom0.
>>
>> I'm happy to submit a patch for option nr 1 if you guys feel that's
>> safer/better at this point.
>>
>
> I'm a bit wary that this patch touches common code because the issue you
> describe seems to be board specific.

Quite a few platforms (e.g Exynos, Tegra, ZynqMP) provide multiple 
interrupt controllers in cascade, with a GIC has the main controller.
We solved the issue for device in handle_device, but not when mapping 
interrupts associated to a bus.

Even if this code touches common code, there is only one caller 
(map_device_children) and the call will only be done for PCI bus. So the 
patch is not as scary as it looks like :).

It's also unclear why this would
> make PCIe function on ZynqMP from the commit message.
>
> I admit I don't know much about ARM so I will leave the further review
> and judgement to ARM maintainers. I don't think OSSTest will pick up bug
> in this patch FWIW.

Correct, there are no ARM platforms with PCI in OSSTest.
Nonetheless, this patch is not too risky as the usage of the function is 
very limited.

IIRC, we currently support 4 platforms with PCI: X-Gene, 
Seattle/Overdrive, Juno, and ZynqMP.

I gave a try on Juno r2 and I have not noticed any regression. I can 
give a try on X-Gene and Seattle if necessary.

For me the change is good to go in Xen 4.7. Stefano do you have any opinion?

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 0/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[Wei Chen]

Hi Julien,

On 17 May 2016 at 00:30, Julien Grall <julien.grall@arm.com> wrote:
> Hi Wei,
>
>
> On 16/05/16 16:47, Wei Liu wrote:
>>
>> On Mon, May 16, 2016 at 05:03:54PM +0200, Edgar E. Iglesias wrote:
>>>
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> I'm sending this as a v2 considering that I previously posted a diff
>>> in email discussions.
>>>
>>> This patch fixes an DT problem with the ZynqMP PCIe node.
>>> Today, we try to remap IRQs regardless of if they are directly
>>> connected to the primary controller or not. If they are indirectly
>>> connected via secondary IRQ controllers, we abort the boot with
>>> an error.
>>>
>>> The ZynqMP PCIe DTS bindings were upstreamed to Linux after Xen 4.6, so
>>> this issue is not a regression.
>>> PCIe is also not generally a critical feature of current ZynqMP boards,
>>> i.e the boards are functional without PCIe although for some users PCIe
>>> may be more or less critical.
>>>
>>> As I see it we have two options:
>>>
>>> 1. A safe option is to disable the PCIe node in the ZynqMP platform.
>>>     We can then fix this with calm for 4.8.
>>>     + It will fix the dom0 boot.
>>>     + Very safe and unintrusive.
>>>     - PCIe will not be functional.
>>>
>>> 2. Apply this patch to 4.7
>>>     + It will fix the dom0 boot.
>>>     + PCIe will be functional.
>>>     - Intrusive fix. Although the fix really only affects a case that
>>>       otherwise would have resulted in an aborted boot of dom0.
>>>
>>> I'm happy to submit a patch for option nr 1 if you guys feel that's
>>> safer/better at this point.
>>>
>>
>> I'm a bit wary that this patch touches common code because the issue you
>> describe seems to be board specific.
>
>
> Quite a few platforms (e.g Exynos, Tegra, ZynqMP) provide multiple interrupt
> controllers in cascade, with a GIC has the main controller.
> We solved the issue for device in handle_device, but not when mapping
> interrupts associated to a bus.
>
> Even if this code touches common code, there is only one caller
> (map_device_children) and the call will only be done for PCI bus. So the
> patch is not as scary as it looks like :).
>
> It's also unclear why this would
>>
>> make PCIe function on ZynqMP from the commit message.
>>
>> I admit I don't know much about ARM so I will leave the further review
>> and judgement to ARM maintainers. I don't think OSSTest will pick up bug
>> in this patch FWIW.
>
>
> Correct, there are no ARM platforms with PCI in OSSTest.
> Nonetheless, this patch is not too risky as the usage of the function is
> very limited.
>
> IIRC, we currently support 4 platforms with PCI: X-Gene, Seattle/Overdrive,
> Juno, and ZynqMP.
>

I have tested on Seattle/Overdrive and I have not noticed any regression too.
I also created a dummy secondary interrupt controller in DTS to test this patch.
It works, Xen is doing the right things in both way.

> I gave a try on Juno r2 and I have not noticed any regression. I can give a
> try on X-Gene and Seattle if necessary.
>
> For me the change is good to go in Xen 4.7. Stefano do you have any opinion?
>
> Regards,
>
> --
> Julien Grall
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH v2 1/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[Julien Grall]

Hi Edgar,

On 16/05/16 16:03, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Do not remap IRQs connected to secondary interrupt controllers.
> These IRQs have no meaning to us until they connect to the
> primary controller.
>
> Secondary IRQ controllers will at some point connect to the
> primary controller (possibly via other IRQ controllers). We
> map the IRQs at that last connection point.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

With the change mention below:

Reviewed-by:  Julien Grall <julien.grall@linaro.org>

> ---
>   xen/common/device_tree.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 0ed86a7..02a7ede 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1176,6 +1176,22 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
>           for ( i = 0; i < pintsize; i++ )
>               dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1);
>
> +        if ( dt_raw_irq.controller != dt_interrupt_controller )
> +        {
> +            /* We don't map IRQs connected to secondary IRQ controllers as

The comments in Xen looks like:

/*
  * Foo
  * Bart
  */

> +             * these IRQs have no meaning to us until they connect to the
> +             * primary controller.
> +             *
> +             * Secondary IRQ controllers will at some point connect to
> +             * the primary controller (possibly via other IRQ controllers).
> +             * We map the IRQs at that last connection point.
> +             */
> +            imap += pintsize;
> +            imaplen -= pintsize;
> +            dt_dprintk(" -> Skipped IRQ for secondary IRQ controller\n");
> +            continue;
> +        }
> +
>           ret = dt_irq_translate(&dt_raw_irq, &dt_irq);
>           if ( ret )
>           {
>

Regards,

-- 
Julien Grall

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

Re: [PATCH v2 1/1] xen/device-tree: Do not remap IRQs for secondary IRQ controllers

[Edgar E. Iglesias]

On Tue, May 17, 2016 at 12:20:43PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 16/05/16 16:03, Edgar E. Iglesias wrote:
> >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> >Do not remap IRQs connected to secondary interrupt controllers.
> >These IRQs have no meaning to us until they connect to the
> >primary controller.
> >
> >Secondary IRQ controllers will at some point connect to the
> >primary controller (possibly via other IRQ controllers). We
> >map the IRQs at that last connection point.
> >
> >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> With the change mention below:
> 
> Reviewed-by:  Julien Grall <julien.grall@linaro.org>

Thanks Julien,

I've fixed the style and posted a v3.

Cheers,
Edgar


> 
> >---
> >  xen/common/device_tree.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> >diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >index 0ed86a7..02a7ede 100644
> >--- a/xen/common/device_tree.c
> >+++ b/xen/common/device_tree.c
> >@@ -1176,6 +1176,22 @@ int dt_for_each_irq_map(const struct dt_device_node *dev,
> >          for ( i = 0; i < pintsize; i++ )
> >              dt_raw_irq.specifier[i] = dt_read_number(imap + i, 1);
> >
> >+        if ( dt_raw_irq.controller != dt_interrupt_controller )
> >+        {
> >+            /* We don't map IRQs connected to secondary IRQ controllers as
> 
> The comments in Xen looks like:
> 
> /*
>  * Foo
>  * Bart
>  */
> 
> >+             * these IRQs have no meaning to us until they connect to the
> >+             * primary controller.
> >+             *
> >+             * Secondary IRQ controllers will at some point connect to
> >+             * the primary controller (possibly via other IRQ controllers).
> >+             * We map the IRQs at that last connection point.
> >+             */
> >+            imap += pintsize;
> >+            imaplen -= pintsize;
> >+            dt_dprintk(" -> Skipped IRQ for secondary IRQ controller\n");
> >+            continue;
> >+        }
> >+
> >          ret = dt_irq_translate(&dt_raw_irq, &dt_irq);
> >          if ( ret )
> >          {
> >
> 
> Regards,
> 
> -- 
> Julien Grall

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