Re: [Xen-devel] [RFC v2 3/7] xen/passthrough/arm: Introduce iommu_fwspec

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 12/10/17 14:05, Julien Grall wrote:
> Hi,
>
> On 21/09/17 01:37, Sameer Goel wrote:
> > Introduce a common structure to hold the fw (ACPI or DT) defined
> > configuration for SMMU hw. The current use case is for arm SMMUs. So,
> > making this architecture specific.
> >
> > Based on Linux kernel commit 57f98d2f61e1: iommu: Introduce iommu_fwspec
> > Signed-off-by: Sameer Goel <sgoel@xxxxxxxxxxxxxx>
> > ---
> >   xen/drivers/passthrough/arm/iommu.c | 66 
> > +++++++++++++++++++++++++++++++++++++
> >   xen/include/asm-arm/device.h        |  1 +
> >   xen/include/xen/iommu.h             | 29 ++++++++++++++++
> >   3 files changed, 96 insertions(+)
> >
> > diff --git a/xen/drivers/passthrough/arm/iommu.c 
> > b/xen/drivers/passthrough/arm/iommu.c
> > index 95b1abb..41c6497 100644
> > --- a/xen/drivers/passthrough/arm/iommu.c
> > +++ b/xen/drivers/passthrough/arm/iommu.c
> > @@ -73,3 +73,69 @@ int arch_iommu_populate_page_table(struct domain *d)
> >       /* The IOMMU shares the p2m with the CPU */
> >       return -ENOSYS;
> >   }
> > +
> > +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle 
> > *fwnode)
> > +{
> > +    return iommu_get_ops();
> Can you please add a comment explain why you always return iommu_get_ops()?
>
> Would it be possible that the device is not behind an IOMMU?
>
> > +}
> > +
> > +int iommu_fwspec_init(struct device *dev, struct fwnode_handle 
> > *iommu_fwnode,
> > +        const struct iommu_ops *ops)
> > +{
> > +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > +
> > +    if ( fwspec )
> > +        return ops == fwspec->ops ? 0 : -EINVAL;
> > +
> > +    fwspec = _xzalloc(sizeof(struct iommu_fwspec), sizeof(void *));
> On the previous version this was xzalloc(struct iommu_fwspec), why?
>
> I also don't understand the align on sizeof(void *).
>
> > +    if ( !fwspec )
> > +        return -ENOMEM;
> > +
> > +    fwspec->iommu_fwnode = iommu_fwnode;
> > +    fwspec->ops = ops;
> > +    dev->iommu_fwspec = fwspec;
> > +
> > +    return 0;
> > +}
> > +
> > +void iommu_fwspec_free(struct device *dev)
> > +{
> > +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > +
> > +    if ( fwspec )
> > +    {
> Linux is dropping the reference on the iommu_fwnode. Are we never 
> expecting to take reference on the it in Xen?
> > +        xfree(fwspec);
> > +        dev->iommu_fwspec = NULL;
> > +    }
> > +}
> > +
> > +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> > +{
> > +    struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> > +    struct iommu_fwspec *fwspec_n = NULL;
> > +    size_t size, size_n;
> > +    int i;
> > +
> > +    if ( !fwspec )
> > +        return -EINVAL;
> > +
> > +    size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids]);
> > +    size_n = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 
> > num_ids]);
> > +    if ( size_n > size )
> > +    { > +        fwspec_n = _xzalloc(size_n, sizeof(void *));
> Same question about _xzalloc() here.
Also, please see the comment I just made on "[RFC 3/6] Introduce _xrealloc".

I would prefer to explore the possibility of a generic helper rather 
than open-coding it. I think we have enough information in hand to get 
the size of the old region.
Cheers,

--
Julien Grall

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