| Message ID | 1555252419-17121-1-git-send-email-amittomer25@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Blacklist PMU with "arm, cortex-a53-pmu" | expand |
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,
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, >
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
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.
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,
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
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,
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
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,
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. */
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(+)