Re: [Xen-devel] [PATCH] xen/arm: Black list everything with a PPI

[Andre Przywara <andre.przywara@xxxxxxx>]

On Thu, 16 May 2019 17:15:36 +0530
Amit Tomer <amittomer25@xxxxxxxxx> wrote:

Hi,

> Thanks for having a look at it.
> 
> On Thu, May 16, 2019 at 12:25 AM Oleksandr <olekstysh@xxxxxxxxx> 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@xxxxxxx>
> > > Signed-off-by: Amit Singh Tomar <amittomer25@xxxxxxxxx>
> > > ---
> > >      * 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

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