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

[Wei Chen <Wei.Chen@xxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> Sent: 2017年7月6日 2:15
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Julien Grall <Julien.Grall@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>; Steve
> Capper <Steve.Capper@xxxxxxx>; xen-devel@xxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel