[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU bindings
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |