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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxx]
> Sent: 2017年7月4日 15:27
> To: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxx; Steve Capper <Steve.Capper@xxxxxxx>; Kaly Xin
> <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU
> bindings
> 
> 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.
> 

Yes, it seems the register_smmu_master has the same issue. I will try to fix 
it, and
abstract the same logic of register_smmu_master and 
arm_smmu_add_generic_master_id
to a separate function.


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

Got it.

> 
> >> > +   }
> >> > +
> >> > +   /*
> >> > +    * 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.
> 

Yes, that's the reason why we use u32. Thanks for your reminder,
Although the master->cfg.streamids is using u16, we'd better to keep
U32 here and add a warning message to notice whom set "stream-match-mask"

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

Yes, we don't currently share SMRs.

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

If the fwspec work can be merged recently, I think it's good to rebase
On it.

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

Sorry about it, I forgot this comment. I will add this comment to 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