[Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Amit Singh Tomar posted 1 patch 8 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/1555252419-17121-1-git-send-email-amittomer25@gmail.com
xen/arch/arm/domain_build.c | 1 +
1 file changed, 1 insertion(+)

[Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Amit Singh Tomar 8 weeks ago
At the moment, we hide PMU's from domain 0 and XEN boot fails on platform[1]
where DTS contains "arm,cortex-a53-pmu" as compatible string for PMU node.

This patch simply adds "arm,cortex-a53-pmu" to list of blacklisted PMUs.

[1]: https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mq.dtsi#L124

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 xen/arch/arm/domain_build.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d983677..b54592a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1334,6 +1334,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
         DT_MATCH_COMPATIBLE("arm,cortex-a15-pmu"),
         DT_MATCH_COMPATIBLE("arm,cortex-a53-edac"),
         DT_MATCH_COMPATIBLE("arm,armv8-pmuv3"),
+        DT_MATCH_COMPATIBLE("arm,cortex-a53-pmu"),
         DT_MATCH_PATH("/cpus"),
         DT_MATCH_TYPE("memory"),
         /* The memory mapped timer is not supported by Xen. */
-- 
2.7.4


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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Julien Grall 8 weeks ago
Hi,

On 4/14/19 3:33 PM, Amit Singh Tomar wrote:
> At the moment, we hide PMU's from domain 0 and XEN boot fails on platform[1]
> where DTS contains "arm,cortex-a53-pmu" as compatible string for PMU node.

"Domain 0 and Xen boot fails" is pretty vague. How does it fail?

In general, a commit message should give enough to the reviewer to 
understand what is the issue and be able to decide whether your patch is 
the correct fix.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by André Przywara 8 weeks ago
On 14/04/2019 17:07, Julien Grall wrote:
> Hi,
> 
> On 4/14/19 3:33 PM, Amit Singh Tomar wrote:
>> At the moment, we hide PMU's from domain 0 and XEN boot fails on
>> platform[1]
>> where DTS contains "arm,cortex-a53-pmu" as compatible string for PMU
>> node.
> 
> "Domain 0 and Xen boot fails" is pretty vague. How does it fail?

After talking via IRC, the problem is PPIs, that this platform uses for
PMU interrupts. When Xen tries to setup the IRQ forwarding for Dom0 for
this device, it fails because it only supports forwarding SPIs.
So interestingly we erroneously forwarded A53 PMUs (those that are
described by that particular compatible string only) to Dom0 for quite a
while, but nobody noticed (or cared).

Amit, please adjust the commit message accordingly.

Cheers,
Andre.


> 
> In general, a commit message should give enough to the reviewer to
> understand what is the issue and be able to decide whether your patch is
> the correct fix.
> 
> Cheers,
> 


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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Amit Tomer 8 weeks ago
Hello,

> After talking via IRC, the problem is PPIs, that this platform uses for
> PMU interrupts. When Xen tries to setup the IRQ forwarding for Dom0 for
> this device, it fails because it only supports forwarding SPIs.
> So interestingly we erroneously forwarded A53 PMUs (those that are
> described by that particular compatible string only) to Dom0 for quite a
> while, but nobody noticed (or cared).
>
> Amit, please adjust the commit message accordingly.
>
Ok but original commit "d45e9b7c53428a2aa4d067927e7ef5e30783fb8b" that blacklist
PMU's clearly states that issue is due to PPI and that can not be
routed to guest.

I thought, This patch is just continuation of same.

Thanks
-Amit

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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Andre Przywara 8 weeks ago
On Mon, 15 Apr 2019 13:41:41 +0530
Amit Tomer <amittomer25@gmail.com> wrote:

> Hello,
> 
> > After talking via IRC, the problem is PPIs, that this platform uses for
> > PMU interrupts. When Xen tries to setup the IRQ forwarding for Dom0 for
> > this device, it fails because it only supports forwarding SPIs.
> > So interestingly we erroneously forwarded A53 PMUs (those that are
> > described by that particular compatible string only) to Dom0 for quite a
> > while, but nobody noticed (or cared).
> >
> > Amit, please adjust the commit message accordingly.
> >  
> Ok but original commit "d45e9b7c53428a2aa4d067927e7ef5e30783fb8b" that blacklist
> PMU's clearly states that issue is due to PPI and that can not be
> routed to guest.
> 
> I thought, This patch is just continuation of same.

Sure, but Julien was asking for a reason for this patch in the commit
message. There are quite some platforms which use this compatible string
already, and none of them failed so far. This is because they use SPIs,
but as the original commit message mentions there are more issues than just
the IRQs.

All this should be in the commit message. As you have seen yourself in Ian's
commit, it's quite helpful later on.

So you should mention:
a) the problem you encountered: PPI forwarding denied on iMX8
b) the reason for the problem: A53 PMU not blacklisted
c) the fix: adding the A53 compatible string to the blacklist

Cheers,
Andre.

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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Julien Grall 7 weeks ago
Hi,

On 15/04/2019 09:41, Andre Przywara wrote:
> On Mon, 15 Apr 2019 13:41:41 +0530
> Amit Tomer <amittomer25@gmail.com> wrote:
> 
>> Hello,
>>
>>> After talking via IRC, the problem is PPIs, that this platform uses for
>>> PMU interrupts. When Xen tries to setup the IRQ forwarding for Dom0 for
>>> this device, it fails because it only supports forwarding SPIs.
>>> So interestingly we erroneously forwarded A53 PMUs (those that are
>>> described by that particular compatible string only) to Dom0 for quite a
>>> while, but nobody noticed (or cared).
>>>
>>> Amit, please adjust the commit message accordingly.
>>>   
>> Ok but original commit "d45e9b7c53428a2aa4d067927e7ef5e30783fb8b" that blacklist
>> PMU's clearly states that issue is due to PPI and that can not be
>> routed to guest.
>>
>> I thought, This patch is just continuation of same.
> 
> Sure, but Julien was asking for a reason for this patch in the commit
> message. There are quite some platforms which use this compatible string
> already, and none of them failed so far. This is because they use SPIs,
> but as the original commit message mentions there are more issues than just
> the IRQs.
> 
> All this should be in the commit message. As you have seen yourself in Ian's
> commit, it's quite helpful later on.
> 
> So you should mention:
> a) the problem you encountered: PPI forwarding denied on iMX8
> b) the reason for the problem: A53 PMU not blacklisted
> c) the fix: adding the A53 compatible string to the blacklist

There are ~25 compatible string for the PMUs and I would rather not want to see 
all of them added in Xen.

 From the test on Juno (it is using arm,cortex-a53-pmu with SPIs), Linux will 
happily give up because we craft the PMU registers to expose nothing.

Could we instead try to automatically blacklist any device using PPI?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Amit Tomer 7 weeks ago
Hello,

> Could we instead try to automatically blacklist any device using PPI?

Just tested following change:

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d983677..0c82976 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1289,6 +1289,10 @@ static int __init handle_device(struct domain
*d, struct dt_device_node *dev,
             return res;
         }

+        /* Don't process device using PPI source */
+        if ( res > 16 && res < 32)
+            return 0;
+
         res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
         if ( res )
             return res;

Would it be Ok ?

Thanks,
-Amit

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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Julien Grall 7 weeks ago

On 26/04/2019 12:58, Amit Tomer wrote:
> Hello,

Hi,

> 
>> Could we instead try to automatically blacklist any device using PPI?
> 
> Just tested following change:
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d983677..0c82976 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1289,6 +1289,10 @@ static int __init handle_device(struct domain
> *d, struct dt_device_node *dev,
>               return res;
>           }
> 
> +        /* Don't process device using PPI source */
> +        if ( res > 16 && res < 32)
> +            return 0;
> +
>           res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
>           if ( res )
>               return res;
> 
> Would it be Ok ?

Well, this does not work properly. The DT node will still be written in the 
domain because handle_device() return 0. However, the device will be half mapped 
resulting to crash later on.

The proper way is to detect PPI before hand and completely skip the node if any.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Amit Tomer 6 weeks ago
Hello,
>
> The proper way is to detect PPI before hand and completely skip the node if any.

I tried the following change:

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d983677..a9ecfed 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1353,7 +1353,7 @@ static int __init handle_node(struct domain *d,
struct kernel_info *kinfo,
         { /* sentinel */ },
     };
     struct dt_device_node *child;
-    int res;
+    int res, irq_id;
     const char *name;
     const char *path;

@@ -1399,6 +1399,16 @@ static int __init handle_node(struct domain *d,
struct kernel_info *kinfo,
         return 0;
     }

+    /*Skip the node, using PPI source */
+    irq_id = platform_get_irq(node, 0);
+
+    if ( irq_id > 16 && irq_id < 32 )
+    {
+        dt_dprintk(" Skip node with (PPI source)\n");
+        return 0;
+    }
+
+

After booting dom0, don't see PMU node is getting generated(its
skipped completely now).

Thanks,
-Amit.



> Julien Grall

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

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

Posted by Julien Grall 6 weeks ago
On 29/04/2019 11:52, Amit Tomer wrote:
> Hello,

Hi,

>>
>> The proper way is to detect PPI before hand and completely skip the node if any.
> 
> I tried the following change:
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d983677..a9ecfed 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1353,7 +1353,7 @@ static int __init handle_node(struct domain *d,
> struct kernel_info *kinfo,
>           { /* sentinel */ },
>       };
>       struct dt_device_node *child;
> -    int res;
> +    int res, irq_id;
>       const char *name;
>       const char *path;
> 
> @@ -1399,6 +1399,16 @@ static int __init handle_node(struct domain *d,
> struct kernel_info *kinfo,
>           return 0;
>       }
> 
> +    /*Skip the node, using PPI source */
> +    irq_id = platform_get_irq(node, 0);

That's only cover the first interrupt of a device. What if the first interrupt 
is an SPI but all the other are actually PPIs?

In order to black all devices using PPI, we need to go through *all* the 
interrupts of the device. Otherwise you will miss some.

Cheers,

-- 
Julien Grall

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