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

[Stefano Stabellini <sstabellini@kernel.org>]

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. 

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