> -----Original Message----- > From: Stefano Stabellini [mailto:sstabell...@kernel.org] > Sent: 2017年7月6日 2:15 > To: Wei Chen <wei.c...@arm.com> > Cc: Julien Grall <julien.gr...@arm.com>; Stefano Stabellini > <sstabell...@kernel.org>; Kaly Xin <kaly....@arm.com>; nd <n...@arm.com>; > Steve > Capper <steve.cap...@arm.com>; xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH 6/7] xen/arm: SMMU: Support generic IOMMU > bindings > > On Wed, 5 Jul 2017, Wei Chen wrote: > > > >> > +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" > > Even if you are using a u32 for fwid, you are still losing all the top > 16 bits in the operations above because of the (u16) casts. This code > looks wrong.
I think it's better to change the arm_smmu_add_generic_master_id(..., u16 fwid) to arm_smmu_add_generic_master_id(..., u32 fwid). And keep the fwid in arm_smmu_of_xlate to u32 to guarantee the data integrity from device tree. Let arm_smmu_add_generic_master_id to determine whether to drop top 16-bits or not. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
