Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device tree binding

[Wei Chen <Wei.Chen@xxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> Sent: 2017年7月6日 2:08
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>;
> Julien Grall <Julien.Grall@xxxxxxx>; Steve Capper <Steve.Capper@xxxxxxx>; nd
> <nd@xxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device
> tree binding
> 
> On Tue, 4 Jul 2017, Wei Chen wrote:
> > Hi Stefano,
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> > > Sent: 2017年7月4日 6:30
> > > To: Wei Chen <Wei.Chen@xxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; Steve Capper
> > > <Steve.Capper@xxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; Julien Grall
> > > <Julien.Grall@xxxxxxx>; nd <nd@xxxxxxx>
> > > Subject: Re: [Xen-devel] [PATCH 4/7] xen/arm: SMMU: Detect types of device
> > > tree binding
> > >
> > > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > > The device tree provides two types of IOMMU bindings, one is legacy
> > > > another is generic. The legacy bindings will be depercated in favour
> > >                                                   ^ deprecated
> > >
> > > > of the generic bindings. But in the transitional period, we have to
> > > > support both of them.
> > > >
> > > > The codes to handle these two types of bindings are very differnet,
> > >       ^ code                                               ^ different
> > >
> > > Please use a spellchecker.
> >
> > Thanks, I will fix them.
> >
> > >
> > > > so we have to detect the binding types while doing SMMU probing.
> > > >
> > > > This detect code is based on Linux ARM SMMUv2 driver:
> > > > https://github.com/torvalds/linux/blob/master/drivers/iommu/arm-smmu.c
> > > >
> > > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > > ---
> > > >  xen/drivers/passthrough/arm/smmu.c | 23 +++++++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > > b/xen/drivers/passthrough/arm/smmu.c
> > > > index 2efa52d..441c296 100644
> > > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > > @@ -143,6 +143,8 @@ typedef enum irqreturn irqreturn_t;
> > > >
> > > >  #define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
> > > >
> > > > +#define pr_notice(fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> > > > +
> > > >  /* Alias to Xen allocation helpers */
> > > >  #define kfree xfree
> > > >  #define kmalloc(size, flags)           _xmalloc(size, sizeof(void *))
> > > > @@ -681,6 +683,8 @@ struct arm_smmu_option_prop {
> > > >         const char *prop;
> > > >  };
> > > >
> > > > +static bool using_legacy_binding, using_generic_binding;
> > >
> > > __initdata?
> > >
> >
> > I think these two variables are not only used in initialization. They also
> > have been used in ops->add_device. Althrough the add_device is only be
> > invoked while construct_dom0.
> 
> I don't think that add_device is supposed to be limited at boot. It's
> best to avoid __initdata then.
> 

Yes. So let keep these two variables without __initdata here.

> 
> > > But why do these two variables need to be static? Can't they just be
> > > local variables in arm_smmu_device_dt_probe?
> > >
> > > Is it to enforce that all smmus on a given platform are either using the
> > > legacy or the generic bindings, but not a mix of the two? If so, it
> > > should be clearly written. Also, I am not sure we should really be
> >
> > Yes, this checking will enforce all smmus are using the same bindings.
> >
> > > checking for that. It seems to me that one smmu could be using generic
> > > bindings and another could be using legacy bindings. Is it specified
> > > anywhere that it cannot be the case?
> > >
> >
> > In theory, different SMMUs can use different bindings. About this concern,
> > I have discussed with Robin Murphy and Julien. We have three reasons:
> >
> > The first is that, we ported this checking from Linux, we are trying to keep
> > the code very close to the Linux driver. To ease backporting changes.
> >
> > The second is that, we think it is a good change to try to phase out the
> > legacy binding and request to use generic bindings everywhere if you
> > start to use in one SMMU.
> >
> > The other less technical reason for not supporting both at once is that
> anyone
> > who can update their DT to add or update SMMUs with the new binding has no
> good
> > excuse for not updating the whole lot. It's the likes of Seattle and
> ThunderX
> > boxes with firmware that won't get updated for which we have to preserve
> "mmu-masters"
> > support.
> 
> I would like these reasons to be written in the commit message. I would
> also like to detect and print a clear warning when SMMUs are using
> mismatched bindings.
> 

Ok.

> 
> > >
> > > >  static struct arm_smmu_option_prop arm_smmu_options[] = {
> > > >         { ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-
> access" },
> > > >         { 0, NULL},
> > > > @@ -2289,6 +2293,25 @@ static int arm_smmu_device_dt_probe(struct
> > > platform_device *pdev)
> > > >         struct rb_node *node;
> > > >         struct of_phandle_args masterspec;
> > > >         int num_irqs, i, err;
> > > > +       bool legacy_binding;
> > > > +
> > > > +       /*
> > > > +        * Xen: Do the same check as Linux. Checking the SMMU device 
> > > > tree
> > > bindings
> > >             ^ do                        ^ Check that
> > >
> > >
> > > > +        * are either using generic or legacy one.
> > >                                           ^ bindings
> > >
> > > > +        *
> > > > +        * The "mmu-masters" property is only existed in legacy 
> > > > bindings.
> > >                                   ^ only exists in the legacy bindings
> > >
> >
> > Thanks, I will fix above typos.
> >
> > > > +        */
> > > > +       legacy_binding = dt_find_property(dev->of_node, "mmu-masters",
> NULL);
> > > > +       if (legacy_binding && !using_generic_binding) {
> > > > +               if (!using_legacy_binding)
> > > > +                       pr_notice("deprecated \"mmu-masters\" DT 
> > > > property in
> use\n");
> > > > +               using_legacy_binding = true;
> > > > +       } else if (!legacy_binding && !using_legacy_binding) {
> > > > +               using_generic_binding = true;
> > >
> > > Please simplify this series of if/else.
> > >
> >
> > This code is the same as Linux SMMU driver. If we agree on enforcing all
> smmus
> > are using the same binding, I prefer to keep the code the same.
> 
> Is it?! Wow... All right then, but I would still like a warning to be
> printed when we find out that an SMMU is using legacy bindings and
> others are using generic bindings.
> 

Ok, I will add this warning message.

> 
> > >
> > > > +       } else {
> > > > +               dev_err(dev, "not probing due to mismatched DT
> properties\n");
> > > > +               return -ENODEV;
> > > > +       }
> > >
> > >
> > >
> > >
> > > >         smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> > > >         if (!smmu) {
> > > > --
> > > > 2.7.4
> > > >
> > > >
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@xxxxxxxxxxxxx
> > > > https://lists.xen.org/xen-devel
> > > >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > https://lists.xen.org/xen-devel
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel