| Message ID | 1556902928-18682-1-git-send-email-amittomer25@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Black list everything with a PPI | expand |
On 03.05.19 20:02, Amit Singh Tomar wrote: Hi, Amit > XEN should not forward PPIs to Dom0 as it only support SPIs. > One of solution to this problem is to skip any device that > uses PPI source completely while building domain itself. > > This patch goes through all the interrupt sources of device and skip it > if one of interrupt source is PPI. It fixes XEN boot on i.MX8MQ by > skipping PMU node. > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> > --- > * This replaces following patch. > https://patchwork.kernel.org/patch/10899881/ > --- > xen/arch/arm/domain_build.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index d983677..8f54472 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, i, nirq, irq_id; > const char *name; > const char *path; > > @@ -1399,6 +1399,20 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > return 0; > } > > + /* Skip the node, using PPI source */ > + nirq = dt_number_of_irq(node); > + > + for ( i = 0 ; i < nirq ; i++ ) > + { > + irq_id = platform_get_irq(node, i); Do we need to do something here if platform_get_irq() returns -1? > + > + if ( irq_id >= 16 && irq_id < 32 ) > + { > + dt_dprintk(" Skip node with (PPI source)\n"); > + return 0; > + } > + } > + > /* > * Xen is using some path for its own purpose. Warn if a node > * already exists with the same path. Patch looks good. Although R-Car H3 seems to not use PPIs for PMU, I have tested your patch just to be sure it wouldn't skip anything by a mistake)
Hello, Thanks for having a look at it. On Thu, May 16, 2019 at 12:25 AM Oleksandr <olekstysh@gmail.com> wrote: > > > On 03.05.19 20:02, Amit Singh Tomar wrote: > > Hi, Amit > > > XEN should not forward PPIs to Dom0 as it only support SPIs. > > One of solution to this problem is to skip any device that > > uses PPI source completely while building domain itself. > > > > This patch goes through all the interrupt sources of device and skip it > > if one of interrupt source is PPI. It fixes XEN boot on i.MX8MQ by > > skipping PMU node. > > > > Suggested-by: Julien Grall <julien.grall@arm.com> > > Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> > > --- > > * This replaces following patch. > > https://patchwork.kernel.org/patch/10899881/ > > --- > > xen/arch/arm/domain_build.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index d983677..8f54472 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, i, nirq, irq_id; > > const char *name; > > const char *path; > > > > @@ -1399,6 +1399,20 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > > return 0; > > } > > > > + /* Skip the node, using PPI source */ > > + nirq = dt_number_of_irq(node); > > + > > + for ( i = 0 ; i < nirq ; i++ ) > > + { > > + irq_id = platform_get_irq(node, i); > > Do we need to do something here if platform_get_irq() returns -1? Yeah, I should have done it. some think like following: https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/domain_build.c;h=d9836779d17c90e84b94ba32e4a20f028189fc5b;hb=HEAD#l1284 > > + > > + if ( irq_id >= 16 && irq_id < 32 ) > > + { > > + dt_dprintk(" Skip node with (PPI source)\n"); > > + return 0; > > + } > > + } > > + > > /* > > * Xen is using some path for its own purpose. Warn if a node > > * already exists with the same path. > > Patch looks good. Although R-Car H3 seems to not use PPIs for PMU, I > have tested your patch just to be sure it wouldn't skip anything by a > mistake) Ok, Thanks for testing it. I would resend it with error condition check. -Thanks Amit.
On Thu, 16 May 2019 17:15:36 +0530 Amit Tomer <amittomer25@gmail.com> wrote: Hi, > Thanks for having a look at it. > > On Thu, May 16, 2019 at 12:25 AM Oleksandr <olekstysh@gmail.com> wrote: > > > > > > On 03.05.19 20:02, Amit Singh Tomar wrote: > > > > Hi, Amit > > > > > XEN should not forward PPIs to Dom0 as it only support SPIs. > > > One of solution to this problem is to skip any device that > > > uses PPI source completely while building domain itself. > > > > > > This patch goes through all the interrupt sources of device and skip it > > > if one of interrupt source is PPI. It fixes XEN boot on i.MX8MQ by > > > skipping PMU node. > > > > > > Suggested-by: Julien Grall <julien.grall@arm.com> > > > Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> > > > --- > > > * This replaces following patch. > > > https://patchwork.kernel.org/patch/10899881/ > > > --- > > > xen/arch/arm/domain_build.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > > index d983677..8f54472 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, i, nirq, irq_id; > > > const char *name; > > > const char *path; > > > > > > @@ -1399,6 +1399,20 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > > > return 0; > > > } > > > > > > + /* Skip the node, using PPI source */ > > > + nirq = dt_number_of_irq(node); > > > + > > > + for ( i = 0 ; i < nirq ; i++ ) > > > + { > > > + irq_id = platform_get_irq(node, i); > > > > Do we need to do something here if platform_get_irq() returns -1? > > Yeah, I should have done it. some think like following: > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/domain_build.c;h=d9836779d17c90e84b94ba32e4a20f028189fc5b;hb=HEAD#l1284 Why would that (or any actual check against -1) be necessary? If irq_id is < 0, then it would surely not match the condition below and *nothing* would happen. So I'd say: Keep it like it is, no action necessary. > > > + > > > + if ( irq_id >= 16 && irq_id < 32 ) Any chance you can put names there? Or at least add a comment that PPIs range from 16 to 31? > > > + { > > > + dt_dprintk(" Skip node with (PPI source)\n"); > > > + return 0; > > > + } > > > + } > > > + > > > /* > > > * Xen is using some path for its own purpose. Warn if a node > > > * already exists with the same path. > > > > Patch looks good. Although R-Car H3 seems to not use PPIs for PMU, I > > have tested your patch just to be sure it wouldn't skip anything by a > > mistake) > > Ok, Thanks for testing it. I would resend it with error condition check. Please don't ;-) Cheers, Andre
On 16/05/2019 14:17, Andre Przywara wrote: > On Thu, 16 May 2019 17:15:36 +0530 > Amit Tomer <amittomer25@gmail.com> wrote: >> On Thu, May 16, 2019 at 12:25 AM Oleksandr <olekstysh@gmail.com> wrote: >>> On 03.05.19 20:02, Amit Singh Tomar wrote: >>>> Suggested-by: Julien Grall <julien.grall@arm.com> >>>> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> >>>> --- >>>> * This replaces following patch. >>>> https://patchwork.kernel.org/patch/10899881/ >>>> --- >>>> xen/arch/arm/domain_build.c | 16 +++++++++++++++- >>>> 1 file changed, 15 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index d983677..8f54472 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, i, nirq, irq_id; >>>> const char *name; >>>> const char *path; >>>> >>>> @@ -1399,6 +1399,20 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, >>>> return 0; >>>> } >>>> >>>> + /* Skip the node, using PPI source */ >>>> + nirq = dt_number_of_irq(node); >>>> + >>>> + for ( i = 0 ; i < nirq ; i++ ) >>>> + { >>>> + irq_id = platform_get_irq(node, i); >>> >>> Do we need to do something here if platform_get_irq() returns -1? >> >> Yeah, I should have done it. some think like following: >> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/domain_build.c;h=d9836779d17c90e84b94ba32e4a20f028189fc5b;hb=HEAD#l1284 > > Why would that (or any actual check against -1) be necessary? > If irq_id is < 0, then it would surely not match the condition below and > *nothing* would happen. So I'd say: Keep it like it is, no action necessary. Worst, depending on the action done with check, you could actively break support for platform with multiple interrupt controllers. That's why in handle_device(), the interrupt controller is checked before calling platform_get_irq(). Cheers,
Hi Amit, Just a quick follow-up. Is there any plan to send a new version of this patch? Cheers, On 03/05/2019 18:02, Amit Singh Tomar wrote: > XEN should not forward PPIs to Dom0 as it only support SPIs. > One of solution to this problem is to skip any device that > uses PPI source completely while building domain itself. > > This patch goes through all the interrupt sources of device and skip it > if one of interrupt source is PPI. It fixes XEN boot on i.MX8MQ by > skipping PMU node. > > Suggested-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> > --- > * This replaces following patch. > https://patchwork.kernel.org/patch/10899881/ > --- > xen/arch/arm/domain_build.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index d983677..8f54472 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, i, nirq, irq_id; > const char *name; > const char *path; > > @@ -1399,6 +1399,20 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > return 0; > } > > + /* Skip the node, using PPI source */ > + nirq = dt_number_of_irq(node); > + > + for ( i = 0 ; i < nirq ; i++ ) > + { > + irq_id = platform_get_irq(node, i); > + > + if ( irq_id >= 16 && irq_id < 32 ) > + { > + dt_dprintk(" Skip node with (PPI source)\n"); > + return 0; > + } > + } > + > /* > * Xen is using some path for its own purpose. Warn if a node > * already exists with the same path. >
Hi, On Wed, May 29, 2019 at 11:17 PM Julien Grall <julien.grall@arm.com> wrote: > > Hi Amit, > > Just a quick follow-up. Is there any plan to send a new version of this patch? > Sorry for late on this, I would be able to send it(with a comment for PPI's range) on coming weekend. Thanks, -Amit
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d983677..8f54472 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, i, nirq, irq_id; const char *name; const char *path; @@ -1399,6 +1399,20 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return 0; } + /* Skip the node, using PPI source */ + nirq = dt_number_of_irq(node); + + for ( i = 0 ; i < nirq ; i++ ) + { + irq_id = platform_get_irq(node, i); + + if ( irq_id >= 16 && irq_id < 32 ) + { + dt_dprintk(" Skip node with (PPI source)\n"); + return 0; + } + } + /* * Xen is using some path for its own purpose. Warn if a node * already exists with the same path.
XEN should not forward PPIs to Dom0 as it only support SPIs. One of solution to this problem is to skip any device that uses PPI source completely while building domain itself. This patch goes through all the interrupt sources of device and skip it if one of interrupt source is PPI. It fixes XEN boot on i.MX8MQ by skipping PMU node. Suggested-by: Julien Grall <julien.grall@arm.com> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com> --- * This replaces following patch. https://patchwork.kernel.org/patch/10899881/ --- xen/arch/arm/domain_build.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)