Re: [Xen-devel] [PATCH V4 6/8] iommu/arm: Add lightweight iommu_fwspec support

[Oleksandr <olekstysh@xxxxxxxxx>]

Hi, all.



> > > > > > > > > +struct iommu_fwspec {
> > > > > > > > > +    /* this device's IOMMU */
> > > > > > > > > +    struct device *iommu_dev;
> > > > > > > > > +    /* IOMMU driver private data for this device */
> > > > > > > > > +    void *iommu_priv;
> > > > > > > > > +    /* number of associated device IDs */
> > > > > > > > > +    unsigned int num_ids;
> > > > > > > > > +    /* IDs which this device may present to the IOMMU */
> > > > > > > > > +    uint32_t ids[1];
> > > > > > > > > +};
> > > > > > > > Note that you abuse xrealloc_flex_struct() when using it with such
> > > > > > > > a type: The last field is _not_ a flexible array member. Compilers
> > > > > > > > might legitimately warn if they can prove that you access
> > > > > > > > p->ids[1] anywhere, despite you (presumably) having allocated 
> > > > > > > > enough
> > > > > > > > space. (I haven't been able to think of a way for the macro to
> > > > > > > > actually detect and hence refuse such wrong uses.)
> > > > > > > Indeed, you are right. I am in doubt, whether to retain ported from
> > > > > > > Linux code (ids[1])
> > > > > > >
> > > > > > > and mention about such abuse or change it to deal with real 
> > > > > > > flexible
> > > > > > > array member (ids[]). Any thoughts?
> > > > > > I'm of the strong opinion that you should switch to [] (or at
> > > > > > least [0]) notation.
> > > > > I got it. Well, will switch to ids[] if there are no objections.
> > > > I suspect the rationale to use 1 rather than 0 is to avoid the 
> > > > re-allocation in
> > > > the common case where a device has a single ID.
> > > >
> > > > I would like to retain the similar behavior. The ids[1] is probably 
> > > > the most
> > > > pretty way to do it.
> > > >
> > > > Another solution would to use xmalloc_bytes() for the initial 
> > > > allocation of
> > > > xmalloc_bytes().
> > > Why not use xmalloc_flex_<whatever>() on the first allocation, too?
> > Hmm, why not? Looks like the xmalloc_flex_struct fits here.
> >
> > The first allocation would be:
> >
> > fwspec = xmalloc_flex_struct(struct iommu_fwspec,  ids, 1);
> >
> >
> > The re-allocation for the devices with single ID would do effectively 
> > nothing (assuming that _xrealloc will recognize that size is the same):
> > fwspec = xrealloc_flex_struct(fwspec, ids, fwspec->num_ids + num_ids);
> >
> >
> > Julien, what do you think?
> I am happy with that. The first allocation would need a comment on top 
> explaining the reason of the "1".
Sure, will add.



--
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel