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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


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


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


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