| Message ID | 1505954230-18892-4-git-send-email-sgoel@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@codeaurora.org> > --- > 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. > + if ( !fwspec_n ) > + return -ENOMEM; > + > + memcpy(fwspec_n, fwspec, size); > + xfree(fwspec); > + } > + > + for (i = 0; i < num_ids; i++) > + fwspec_n->ids[fwspec_n->num_ids + i] = ids[i]; > + > + fwspec_n->num_ids += num_ids; > + dev->iommu_fwspec = fwspec_n; > + > + return 0; > +} > diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h > index 78c38fe..5027c87 100644 > --- a/xen/include/asm-arm/device.h > +++ b/xen/include/asm-arm/device.h > @@ -21,6 +21,7 @@ struct device > struct dt_device_node *of_node; /* Used by drivers imported from Linux */ > #endif > struct fwnode_handle *fwnode; /*fw device node identifier */ > + struct iommu_fwspec *iommu_fwspec; > struct dev_archdata archdata; > }; > > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 0dac4f3..34e8d68 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -208,4 +208,33 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb); > extern struct spinlock iommu_pt_cleanup_lock; > extern struct page_list_head iommu_pt_cleanup_list; > > +/** > + * Following block was ported from Linux to help with the implementation of > + * arm64 iommu devices. Hence the architecture specific compile > + */ > + > +#if defined(CONFIG_ARM) If it is Arm only, then it should be moved in asm-arm/iommu.h. > +/** > + * struct iommu_fwspec - per-device IOMMU instance data > + * @ops: ops for this device's IOMMU > + * @iommu_fwnode: firmware handle for this device's IOMMU > + * @iommu_priv: IOMMU driver private data for this device > + * @num_ids: number of associated device IDs > + * @ids: IDs which this device may present to the IOMMU > + */ > +struct iommu_fwspec { > + const struct iommu_ops *ops; > + struct fwnode_handle *iommu_fwnode; > + void *iommu_priv; > + unsigned int num_ids; > + u32 ids[1]; > +}; > + > +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, > + const struct iommu_ops *ops); > +void iommu_fwspec_free(struct device *dev); > +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); > +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode); > + > +#endif > #endif /* _IOMMU_H_ */ > Cheers,
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@codeaurora.org> >> --- >> 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,
On 10/12/2017 7:05 AM, 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@codeaurora.org> >> --- >> 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? That is true. I will fix this. > >> +} >> + >> +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 *). Forgot why I did this. I will change it back. I just copied the alignment value from xzalloc. > >> + 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? I will fix this. > >> + 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. > See above. >> + if ( !fwspec_n ) >> + return -ENOMEM; >> + >> + memcpy(fwspec_n, fwspec, size); >> + xfree(fwspec); >> + } >> + >> + for (i = 0; i < num_ids; i++) >> + fwspec_n->ids[fwspec_n->num_ids + i] = ids[i]; >> + >> + fwspec_n->num_ids += num_ids; >> + dev->iommu_fwspec = fwspec_n; >> + >> + return 0; >> +} >> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h >> index 78c38fe..5027c87 100644 >> --- a/xen/include/asm-arm/device.h >> +++ b/xen/include/asm-arm/device.h >> @@ -21,6 +21,7 @@ struct device >> struct dt_device_node *of_node; /* Used by drivers imported from Linux */ >> #endif >> struct fwnode_handle *fwnode; /*fw device node identifier */ >> + struct iommu_fwspec *iommu_fwspec; >> struct dev_archdata archdata; >> }; >> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >> index 0dac4f3..34e8d68 100644 >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -208,4 +208,33 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb); >> extern struct spinlock iommu_pt_cleanup_lock; >> extern struct page_list_head iommu_pt_cleanup_list; >> +/** >> + * Following block was ported from Linux to help with the implementation of >> + * arm64 iommu devices. Hence the architecture specific compile >> + */ >> + >> +#if defined(CONFIG_ARM) > > If it is Arm only, then it should be moved in asm-arm/iommu.h. Will do. > >> +/** >> + * struct iommu_fwspec - per-device IOMMU instance data >> + * @ops: ops for this device's IOMMU >> + * @iommu_fwnode: firmware handle for this device's IOMMU >> + * @iommu_priv: IOMMU driver private data for this device >> + * @num_ids: number of associated device IDs >> + * @ids: IDs which this device may present to the IOMMU >> + */ >> +struct iommu_fwspec { >> + const struct iommu_ops *ops; >> + struct fwnode_handle *iommu_fwnode; >> + void *iommu_priv; >> + unsigned int num_ids; >> + u32 ids[1]; >> +}; >> + >> +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, >> + const struct iommu_ops *ops); >> +void iommu_fwspec_free(struct device *dev); >> +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); >> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode); >> + >> +#endif >> #endif /* _IOMMU_H_ */ >> > > Cheers, >
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(); +} + +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 *)); + 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 ) + { + 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 *)); + if ( !fwspec_n ) + return -ENOMEM; + + memcpy(fwspec_n, fwspec, size); + xfree(fwspec); + } + + for (i = 0; i < num_ids; i++) + fwspec_n->ids[fwspec_n->num_ids + i] = ids[i]; + + fwspec_n->num_ids += num_ids; + dev->iommu_fwspec = fwspec_n; + + return 0; +} diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h index 78c38fe..5027c87 100644 --- a/xen/include/asm-arm/device.h +++ b/xen/include/asm-arm/device.h @@ -21,6 +21,7 @@ struct device struct dt_device_node *of_node; /* Used by drivers imported from Linux */ #endif struct fwnode_handle *fwnode; /*fw device node identifier */ + struct iommu_fwspec *iommu_fwspec; struct dev_archdata archdata; }; diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 0dac4f3..34e8d68 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -208,4 +208,33 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb); extern struct spinlock iommu_pt_cleanup_lock; extern struct page_list_head iommu_pt_cleanup_list; +/** + * Following block was ported from Linux to help with the implementation of + * arm64 iommu devices. Hence the architecture specific compile + */ + +#if defined(CONFIG_ARM) +/** + * struct iommu_fwspec - per-device IOMMU instance data + * @ops: ops for this device's IOMMU + * @iommu_fwnode: firmware handle for this device's IOMMU + * @iommu_priv: IOMMU driver private data for this device + * @num_ids: number of associated device IDs + * @ids: IDs which this device may present to the IOMMU + */ +struct iommu_fwspec { + const struct iommu_ops *ops; + struct fwnode_handle *iommu_fwnode; + void *iommu_priv; + unsigned int num_ids; + u32 ids[1]; +}; + +int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode, + const struct iommu_ops *ops); +void iommu_fwspec_free(struct device *dev); +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode); + +#endif #endif /* _IOMMU_H_ */
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@codeaurora.org> --- 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(+)