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

[Wei Chen <Wei.Chen@xxxxxxx>]

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.

> 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.

> 
> >  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.

> 
> > +   } 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