Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 07/04/2017 07:27 AM, Wei Chen wrote:
> Hi Stefano,
>
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> > Sent: 2017年7月4日 7:00
> > 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 6/7] xen/arm: SMMU: Support generic IOMMU
> > bindings
> >
> > On Fri, 30 Jun 2017, Wei Chen wrote:
> > > The SMMU MasterIDs are placed at the master devices' DT node while
> > > using the generic bindings. In this case, it's very hard for us to
> > > register SMMU masters while probing SMMU as we had done for legacy
> > > bindings. Because we have to go through whole device tree for all
> > > SMMU devices to find their master devices.
> > >
> > > It's better to register SMMU master for generic bindings in add_device
> > > callback. This callback will only be called while constructing Dom0.
> > >
> > > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > > ---
> > >  xen/drivers/passthrough/arm/smmu.c | 144
> > ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 143 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > > index 895024c..25f2207 100644
> > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > @@ -2621,8 +2621,150 @@ static void arm_smmu_destroy_iommu_domain(struct
> > iommu_domain *domain)
> > >      xfree(domain);
> > >  }
> > >
> > > +static int arm_smmu_add_generic_master_id(struct arm_smmu_device *smmu,
> > > +                           struct device *master_dev, u16 fwid)
> > > +{
> > > +   struct arm_smmu_master *master;
> > > +   struct device_node *master_np = master_dev->of_node;
> > > +
> > > +   master = find_smmu_master(smmu, master_np);
> > > +   if (!master) {
> > > +           dev_notice(smmu->dev,
> > > +                   "This smmu master [%s] hasn't been registered, creating
> > now!\n",
> > > +                   master_np->full_name);
> > > +           master = devm_kzalloc(smmu->dev, sizeof(*master), GFP_KERNEL);
> > > +           if (!master)
> > > +                   return -ENOMEM;
> > > +
> > > +           master->of_node = master_np;
> > > +           master->cfg.num_streamids = 0;
> > > +
> > > +           /*
> > > +            * Xen: Let Xen know that the device is protected by a SMMU.
> > > +            * Only do while registering the master.
> > > +            */
> > > +           dt_device_set_protected(master_np);
> > > +   }
> > > +
> > > +   /*
> > > +    * If the smmu is using the stream index mode, check whether
> > > +    * the streamid exceeds the max allowed id,
> > > +    */
> > > +   if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) &&
> > > +           (fwid >= smmu->num_mapping_groups)) {
> > > +           dev_err(smmu->dev,
> > > +                   "Stream ID for master device %s greater than maximum 
> > allowed
> > (%d)\n",t
> > > +                   master_np->name, smmu->num_mapping_groups);
You allocate memory that will be lost forever if it fails for the first ID.

> > > +           return -ERANGE;
> > > +   }
> > > +
> > > +   if (master->cfg.num_streamids >= MAX_MASTER_STREAMIDS) {
> > > +           dev_err(smmu->dev,
> > > +                   "Reached maximum number (%d) of stream IDs for master
> > device %s\n",
> > > +                   MAX_MASTER_STREAMIDS, master_np->name);
> > > +           return -ENOSPC;
Ditto.

> > > +   }
> > > +
> > > +   /*
> > > +    * If this is the first time we add id to this master,
> > > +    * we have to register this master to rb tree.
> > > +    */
> > > +   if (!master->cfg.num_streamids) {
> > > +           int ret;
> > > +           ret = insert_smmu_master(smmu, master);
> > > +           if ( ret && ret != -EEXIST ) {
> > > +                   dev_err(smmu->dev,
> > > +                           "Insert %s to smmu's master rb tree failed\n",
> > master_np->name);
> > > +                   return ret;
> > > +           }
> > > +   }
> > > +
> > > +   master->cfg.streamids[master->cfg.num_streamids] = fwid;
> > > +   master->cfg.num_streamids++;
> > > +   dev_dbg(smmu->dev,
> > > +           "Add new streamid [%d] to smmu [%s] for master [%s]!\n",
> > > +           fwid, smmu->dev->of_node->name, master_np->name);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static struct arm_smmu_device *find_smmu(const struct device *dev);
> > > +
> > > +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args
> > *args)
> > > +{
> > > +   struct arm_smmu_device *smmu;
> > > +   u32 mask = 0, fwid = 0;
> > > +
> > > +   smmu = find_smmu(dt_to_dev(args->np));
> > > +   if (!smmu) {
> > > +           dev_err(dev, "Could not find smmu device!\n");
> > > +           return -ENODEV;
> > > +   }
> > > +
> > > +   if (args->args_count > 0)
> > > +           fwid |= (u16)args->args[0];
> > > +
> > > +   if (args->args_count > 1)
> > > +           fwid |= (u16)args->args[1] << SMR_MASK_SHIFT;
> > > +   else if (!of_property_read_u32(args->np, "stream-match-mask", &mask))
> > > +           fwid |= (u16)mask << SMR_MASK_SHIFT;
> > > +   dev_dbg(dev, "%s fwid:%08x mask:%08x args_count:%d\n",
> > > +                      args->np->full_name, fwid,
> > > +                      mask, args->args_count);
> >
> > I don't understand why fwid is declared as u32 but used as a u16 below.
> > Shouldn't it be declared as u16 in the first place?
> Oh, it's my mistake. In Linux, the fwid will be passed to 
> iommu_fwspec_add_ids,
> it requires u32 parameter. But after I ported this code to Xen, we passed
> the fwid to arm_smmu_add_generic_master_id, a u16 parameter is enough
u16 is not enough. If you look at the code you ported:

if (args->args_count > 0)
  fwid |= (u16)args->args[0];

if (!of_property_read_u32(args->np, "stream-match-mask", &mask)
  fwid |= mask << SMR_MASK_SHIFT;

SMR_MASK_SHIFT is 16, so the top 16-bit will be set to the mask. With  
your u16 cast you loose those bits and therefore will not support  
properly SMMU when the property "stream-match-mask" has been set.
Looking at our SMMU driver, I think we don't yet support sharing SMRS  
(you would need to double check). I would be ok if you don't support  
them, but at least you need to avoid blindly skipping the property  
because this is going to be difficult to debug. This means we need to  
print an error message and bail out if someone set that.
This kind of porting error could have been mitigated if this series was  
rebased as suggested multiple time on top of the fwspec work from QC  
(see [1]).
Regardless that, I would much prefer to rebase this work on top of the  
fwspec series. This is going to simplify a lot the logic and avoid code  
duplication, arm_smmu_add_generic_master_id is very similar to  
register_smmu_master.
Lastly, as I mentioned to you, any code not present in the Linux SMMU  
driver should be commented with /* Xen: ... */. This is helping us to  
know what has changed. For instance, I cannot find  
arm_smmu_add_generic_master_id in Linux code.
Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2017-06/msg00862.html

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel