| Message ID | 20180102092809.1841-7-manish.jaggi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | acpi: arm: IORT Support for Xen | expand |
Hi Manish, Please use scripts/get_maintainers.pl to CC relevant maintainers. I have done it for you this time. Title: s/spacific/specific/ On 02/01/18 09:28, manish.jaggi@linaro.org wrote: > From: Manish Jaggi <manish.jaggi@linaro.org> > > Merge few more changes from linux kernel code (v4.14) into iommu.c > Modify code specifc to xen. I appreciate you pick-up the series from Sameer. I would also have appreciated if you have addressed my remarks from there. Sameer explain why he imported fwnode. This has been dropped here. Also, I think you probably want a bit more context in the commit message about implement fwnode.h in common code. Within this series, fwnode seems to only be used by Arm. So what would be the advantage to get that in xen/? Is it going to be used by x86 or taken advantage in common code? > > Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org> > --- > xen/drivers/passthrough/iommu.c | 75 +++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/device.h | 11 ++++-- > xen/include/xen/iommu.h | 22 ++++++++++++ > 3 files changed, 106 insertions(+), 2 deletions(-) > > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index 1aecf7cf34..408f44106d 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -13,6 +13,7 @@ > */ > > #include <xen/sched.h> > +#include <xen/fwnode.h> > #include <xen/iommu.h> > #include <xen/paging.h> > #include <xen/guest_access.h> > @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key) > } > } > > +/** > + * fwnode_handle_put - Drop reference to a device node > + * @fwnode: Pointer to the device node to drop the reference to. > + * > + * This has to be used when terminating device_for_each_child_node() iteration > + * with break or return to prevent stale device node references from being left > + * behind. > + */ > +void fwnode_handle_put(struct fwnode_handle *fwnode) > +{ > + fwnode_call_void_op(fwnode, put); This file is following Xen coding style. And therefore you should use Xen coding. > +} > + > +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 = kzalloc(sizeof(*fwspec), GFP_KERNEL); You define kzalloc in a later patch and hence break the build. *All* the patches should build one by one to help bisectability. But given the side of the code and the fact you are going to fix the coding style. It might be easier to use Xen name here. > + if (!fwspec) > + return -ENOMEM; > +#if 0 > + of_node_get(to_of_node(iommu_fwnode)); > +#endif > + 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) { > + fwnode_handle_put(fwspec->iommu_fwnode); > + kfree(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; > + size_t size; > + int i; > + > + if (!fwspec) > + return -EINVAL; > + > + size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]); > + if (size > sizeof(*fwspec)) { > + //TBD: fwspec = krealloc(dev->iommu_fwspec, size, GFP_KERNEL); Hmmm? > + if (!fwspec) > + return -ENOMEM; > + > + dev->iommu_fwspec = fwspec; > + } > + > + for (i = 0; i < num_ids; i++) > + fwspec->ids[fwspec->num_ids + i] = ids[i]; > + > + fwspec->num_ids += num_ids; > + return 0; > + > +} > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h > index 6734ae8efd..f78482ca0c 100644 > --- a/xen/include/asm-arm/device.h > +++ b/xen/include/asm-arm/device.h > @@ -6,6 +6,8 @@ > enum device_type > { > DEV_DT, > + DEV_ACPI, You don't use DEV_ACPI in this patch. So why is there? > + DEV_PCI, > }; > > struct dev_archdata { > @@ -18,8 +20,13 @@ struct device > enum device_type type; > #ifdef CONFIG_HAS_DEVICE_TREE > struct dt_device_node *of_node; /* Used by drivers imported from Linux */ As said on Sameer's patches, I was expecting a todo in the code after the discussion about leave of_node here. > +#endif > +#ifdef CONFIG_ACPI > + void *acpi_node; You don't use acpi_node in this patch. So why is it there? > #endif > struct dev_archdata archdata; > + struct fwnode_handle *fwnode; /* firmware device node */ Ditto. > + struct iommu_fwspec *iommu_fwspec; > }; > > typedef struct device device_t; > @@ -27,8 +34,8 @@ typedef struct device device_t; > #include <xen/device_tree.h> > > /* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */ > -#define dev_is_pci(dev) ((void)(dev), 0) > -#define dev_is_dt(dev) ((dev->type == DEV_DT) > +#define dev_is_pci(dev) (dev->type == DEV_PCI) > +#define dev_is_dt(dev) (dev->type == DEV_DT) Those two changes does not belong to this patch. It is likely 2 separate patches: 1# fixing dev_is_dt because of the missing parenthese 2# implementing dev_is_dt > > enum device_class > { > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 33c8b221dc..56b169bae9 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -208,4 +208,26 @@ 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; > > +/** > + * 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 /* _IOMMU_H_ */ > Cheers,
On 01/19/2018 12:21 AM, Julien Grall wrote: > Hi Manish, > > Please use scripts/get_maintainers.pl to CC relevant maintainers. I > have done it for you this time. > > > Title: s/spacific/specific/ > > On 02/01/18 09:28, manish.jaggi@linaro.org wrote: >> From: Manish Jaggi <manish.jaggi@linaro.org> >> >> Merge few more changes from linux kernel code (v4.14) into iommu.c >> Modify code specifc to xen. > > I appreciate you pick-up the series from Sameer. I would also have > appreciated if you have addressed my remarks from there. > > Sameer explain why he imported fwnode. This has been dropped here. Also, > I think you probably want a bit more context in the commit message > about implement fwnode.h in common code. > > Within this series, fwnode seems to only be used by Arm. So what would > be the advantage to get that in xen/? Is it going to be used by x86 or > taken advantage in common code? Do you want patch code (iommu.h iommu.c) to be moved to xen/arch/arm In patch 4, i have created a new file xen/include/xen/fwnode.h Should I move it to asm-arm ? > >> >> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org> >> --- >> xen/drivers/passthrough/iommu.c | 75 >> +++++++++++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/device.h | 11 ++++-- >> xen/include/xen/iommu.h | 22 ++++++++++++ >> 3 files changed, 106 insertions(+), 2 deletions(-) >> >> diff --git a/xen/drivers/passthrough/iommu.c >> b/xen/drivers/passthrough/iommu.c >> index 1aecf7cf34..408f44106d 100644 >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -13,6 +13,7 @@ >> */ >> #include <xen/sched.h> >> +#include <xen/fwnode.h> >> #include <xen/iommu.h> >> #include <xen/paging.h> >> #include <xen/guest_access.h> >> @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key) >> } >> } >> +/** >> + * fwnode_handle_put - Drop reference to a device node >> + * @fwnode: Pointer to the device node to drop the reference to. >> + * >> + * This has to be used when terminating device_for_each_child_node() >> iteration >> + * with break or return to prevent stale device node references from >> being left >> + * behind. >> + */ >> +void fwnode_handle_put(struct fwnode_handle *fwnode) >> +{ >> + fwnode_call_void_op(fwnode, put); > > This file is following Xen coding style. And therefore you should use > Xen coding. > >> +} >> + >> +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 = kzalloc(sizeof(*fwspec), GFP_KERNEL); > > You define kzalloc in a later patch and hence break the build. *All* > the patches should build one by one to help bisectability. > > But given the side of the code and the fact you are going to fix the > coding style. It might be easier to use Xen name here. > >> + if (!fwspec) >> + return -ENOMEM; >> +#if 0 >> + of_node_get(to_of_node(iommu_fwnode)); >> +#endif >> + 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) { >> + fwnode_handle_put(fwspec->iommu_fwnode); >> + kfree(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; >> + size_t size; >> + int i; >> + >> + if (!fwspec) >> + return -EINVAL; >> + >> + size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + >> num_ids]); >> + if (size > sizeof(*fwspec)) { >> + //TBD: fwspec = krealloc(dev->iommu_fwspec, size, >> GFP_KERNEL); > > Hmmm? > >> + if (!fwspec) >> + return -ENOMEM; >> + >> + dev->iommu_fwspec = fwspec; >> + } >> + >> + for (i = 0; i < num_ids; i++) >> + fwspec->ids[fwspec->num_ids + i] = ids[i]; >> + >> + fwspec->num_ids += num_ids; >> + return 0; >> + >> +} >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h >> index 6734ae8efd..f78482ca0c 100644 >> --- a/xen/include/asm-arm/device.h >> +++ b/xen/include/asm-arm/device.h >> @@ -6,6 +6,8 @@ >> enum device_type >> { >> DEV_DT, >> + DEV_ACPI, > > You don't use DEV_ACPI in this patch. So why is there? > >> + DEV_PCI, >> }; >> struct dev_archdata { >> @@ -18,8 +20,13 @@ struct device >> enum device_type type; >> #ifdef CONFIG_HAS_DEVICE_TREE >> struct dt_device_node *of_node; /* Used by drivers imported >> from Linux */ > > As said on Sameer's patches, I was expecting a todo in the code after > the discussion about leave of_node here. I might have missed your comment on sameers patch, could you please restate > >> +#endif >> +#ifdef CONFIG_ACPI >> + void *acpi_node; > > You don't use acpi_node in this patch. So why is it there? > >> #endif >> struct dev_archdata archdata; >> + struct fwnode_handle *fwnode; /* firmware device node */ > > Ditto. > >> + struct iommu_fwspec *iommu_fwspec; >> }; >> typedef struct device device_t; >> @@ -27,8 +34,8 @@ typedef struct device device_t; >> #include <xen/device_tree.h> >> /* TODO: Correctly implement dev_is_pci when PCI is supported on >> ARM */ >> -#define dev_is_pci(dev) ((void)(dev), 0) >> -#define dev_is_dt(dev) ((dev->type == DEV_DT) >> +#define dev_is_pci(dev) (dev->type == DEV_PCI) >> +#define dev_is_dt(dev) (dev->type == DEV_DT) > > Those two changes does not belong to this patch. It is likely 2 > separate patches: > 1# fixing dev_is_dt because of the missing parenthese > 2# implementing dev_is_dt > >> enum device_class >> { >> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >> index 33c8b221dc..56b169bae9 100644 >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -208,4 +208,26 @@ 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; >> +/** >> + * 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 /* _IOMMU_H_ */ >> > > Cheers, >
Hi Julien, On 01/19/2018 12:21 AM, Julien Grall wrote: >> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h >> index 6734ae8efd..f78482ca0c 100644 >> --- a/xen/include/asm-arm/device.h >> +++ b/xen/include/asm-arm/device.h >> @@ -6,6 +6,8 @@ >> enum device_type >> { >> DEV_DT, >> + DEV_ACPI, > > You don't use DEV_ACPI in this patch. So why is there? > >> + DEV_PCI, >> }; >> struct dev_archdata { >> @@ -18,8 +20,13 @@ struct device >> enum device_type type; >> #ifdef CONFIG_HAS_DEVICE_TREE >> struct dt_device_node *of_node; /* Used by drivers imported >> from Linux */ > > As said on Sameer's patches, I was expecting a todo in the code after > the discussion about leave of_node here. I think you are referring to https://patchwork.kernel.org/patch/9963109/ Could you please add what TODO you wish to add ? I could not find any discussion on of_node in the mail chain -Regards Manish <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p>Hi Julien,<br> </p> <br> <div class="moz-cite-prefix">On 01/19/2018 12:21 AM, Julien Grall wrote:<br> </div> <blockquote type="cite" cite="mid:521998b8-1179-bc87-db7b-a4e3aee0c644@linaro.org"> <blockquote type="cite" style="color: #000000;">diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h <br> index 6734ae8efd..f78482ca0c 100644 <br> --- a/xen/include/asm-arm/device.h <br> +++ b/xen/include/asm-arm/device.h <br> @@ -6,6 +6,8 @@ <br> enum device_type <br> { <br> DEV_DT, <br> + DEV_ACPI, <br> </blockquote> <br> You don't use DEV_ACPI in this patch. So why is there? <br> <br> <blockquote type="cite" style="color: #000000;">+ DEV_PCI, <br> }; <br> struct dev_archdata { <br> @@ -18,8 +20,13 @@ struct device <br> enum device_type type; <br> #ifdef CONFIG_HAS_DEVICE_TREE <br> struct dt_device_node *of_node; /* Used by drivers imported from Linux */ <br> </blockquote> <br> As said on Sameer's patches, I was expecting a todo in the code after the discussion about leave of_node here. </blockquote> I think you are referring to <a class="moz-txt-link-freetext" href="https://patchwork.kernel.org/patch/9963109/">https://patchwork.kernel.org/patch/9963109/</a><br> Could you please add what TODO you wish to add ?<br> <br> I could not find any discussion on of_node in the mail chain<br> <br> <br> -Regards<br> Manish<br> </body> </html>
Hi Julien, On 01/19/2018 12:21 AM, Julien Grall wrote: >> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h >> index 6734ae8efd..f78482ca0c 100644 >> --- a/xen/include/asm-arm/device.h >> +++ b/xen/include/asm-arm/device.h >> @@ -6,6 +6,8 @@ >> enum device_type >> { >> DEV_DT, >> + DEV_ACPI, > > You don't use DEV_ACPI in this patch. So why is there? > >> + DEV_PCI, >> }; >> struct dev_archdata { >> @@ -18,8 +20,13 @@ struct device >> enum device_type type; >> #ifdef CONFIG_HAS_DEVICE_TREE >> struct dt_device_node *of_node; /* Used by drivers imported >> from Linux */ > > As said on Sameer's patches, I was expecting a todo in the code after > the discussion about leave of_node here. I think you are referring to https://patchwork.kernel.org/patch/9963109/ Could you please add what TODO you wish to add ? I could not find any discussion on of_node in the mail chain -Regards Manish
On 06/03/18 13:44, Manish Jaggi wrote: > Hi Julien, > > > On 01/19/2018 12:21 AM, Julien Grall wrote: >>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h >>> index 6734ae8efd..f78482ca0c 100644 >>> --- a/xen/include/asm-arm/device.h >>> +++ b/xen/include/asm-arm/device.h >>> @@ -6,6 +6,8 @@ >>> enum device_type >>> { >>> DEV_DT, >>> + DEV_ACPI, >> >> You don't use DEV_ACPI in this patch. So why is there? >> >>> + DEV_PCI, >>> }; >>> struct dev_archdata { >>> @@ -18,8 +20,13 @@ struct device >>> enum device_type type; >>> #ifdef CONFIG_HAS_DEVICE_TREE >>> struct dt_device_node *of_node; /* Used by drivers imported >>> from Linux */ >> >> As said on Sameer's patches, I was expecting a todo in the code after >> the discussion about leave of_node here. > I think you are referring to https://patchwork.kernel.org/patch/9963109/ > Could you please add what TODO you wish to add ? > > I could not find any discussion on of_node in the mail chain Usually when I say: "I was expecting ..." it means that was discussed on a previous version. In that case it is "[RFC 2/6] arm64: Add definitions for fwnode_handle". Below the conversation: Me: I am a bit surprised you don't rework struct dev. As of_node is now redundant with fwnode. Sameer: I agree that this will eventually be removed. I have kept this in now just to maintain compatibility (compilation and otherwise) with smmuv2 driver. I will add a comment to indicate this. So that it can be easily identified and remove when we do a final cleanup. Can I prefix the comment with with XEN:TODO:? Me: A TODO would be nice, but who is going to do the rework? Cheers,
On 06/03/18 10:27, Manish Jaggi wrote: > > > On 01/19/2018 12:21 AM, Julien Grall wrote: >> Hi Manish, >> >> Please use scripts/get_maintainers.pl to CC relevant maintainers. I >> have done it for you this time. >> >> >> Title: s/spacific/specific/ >> >> On 02/01/18 09:28, manish.jaggi@linaro.org wrote: >>> From: Manish Jaggi <manish.jaggi@linaro.org> >>> >>> Merge few more changes from linux kernel code (v4.14) into iommu.c >>> Modify code specifc to xen. >> >> I appreciate you pick-up the series from Sameer. I would also have >> appreciated if you have addressed my remarks from there. >> >> Sameer explain why he imported fwnode. This has been dropped here. Also, >> I think you probably want a bit more context in the commit message >> about implement fwnode.h in common code. >> >> Within this series, fwnode seems to only be used by Arm. So what would >> be the advantage to get that in xen/? Is it going to be used by x86 or >> taken advantage in common code? > Do you want patch code (iommu.h iommu.c) to be moved to xen/arch/arm > In patch 4, i have created a new file xen/include/xen/fwnode.h > Should I move it to asm-arm ? Please read my e-mail and answer the question. If you answer by no, then move it to asm-arm. If you answer by yes, then write the rationale in the commit message. It looks like to me we might only need that in Arm, but I wanted your input as the author of the patch. You likely have a reason to put this code here, am I right? Cheers, >> >>> >>> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org> >>> --- >>> xen/drivers/passthrough/iommu.c | 75 >>> +++++++++++++++++++++++++++++++++++++++++ >>> xen/include/asm-arm/device.h | 11 ++++-- >>> xen/include/xen/iommu.h | 22 ++++++++++++ >>> 3 files changed, 106 insertions(+), 2 deletions(-) >>> >>> diff --git a/xen/drivers/passthrough/iommu.c >>> b/xen/drivers/passthrough/iommu.c >>> index 1aecf7cf34..408f44106d 100644 >>> --- a/xen/drivers/passthrough/iommu.c >>> +++ b/xen/drivers/passthrough/iommu.c >>> @@ -13,6 +13,7 @@ >>> */ >>> #include <xen/sched.h> >>> +#include <xen/fwnode.h> >>> #include <xen/iommu.h> >>> #include <xen/paging.h> >>> #include <xen/guest_access.h> >>> @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key) >>> } >>> } >>> +/** >>> + * fwnode_handle_put - Drop reference to a device node >>> + * @fwnode: Pointer to the device node to drop the reference to. >>> + * >>> + * This has to be used when terminating device_for_each_child_node() >>> iteration >>> + * with break or return to prevent stale device node references from >>> being left >>> + * behind. >>> + */ >>> +void fwnode_handle_put(struct fwnode_handle *fwnode) >>> +{ >>> + fwnode_call_void_op(fwnode, put); >> >> This file is following Xen coding style. And therefore you should use >> Xen coding. >> >>> +} >>> + >>> +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 = kzalloc(sizeof(*fwspec), GFP_KERNEL); >> >> You define kzalloc in a later patch and hence break the build. *All* >> the patches should build one by one to help bisectability. >> >> But given the side of the code and the fact you are going to fix the >> coding style. It might be easier to use Xen name here. >> >>> + if (!fwspec) >>> + return -ENOMEM; >>> +#if 0 >>> + of_node_get(to_of_node(iommu_fwnode)); >>> +#endif >>> + 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) { >>> + fwnode_handle_put(fwspec->iommu_fwnode); >>> + kfree(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; >>> + size_t size; >>> + int i; >>> + >>> + if (!fwspec) >>> + return -EINVAL; >>> + >>> + size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + >>> num_ids]); >>> + if (size > sizeof(*fwspec)) { >>> + //TBD: fwspec = krealloc(dev->iommu_fwspec, size, >>> GFP_KERNEL); >> >> Hmmm? >> >>> + if (!fwspec) >>> + return -ENOMEM; >>> + >>> + dev->iommu_fwspec = fwspec; >>> + } >>> + >>> + for (i = 0; i < num_ids; i++) >>> + fwspec->ids[fwspec->num_ids + i] = ids[i]; >>> + >>> + fwspec->num_ids += num_ids; >>> + return 0; >>> + >>> +} >>> /* >>> * Local variables: >>> * mode: C >>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h >>> index 6734ae8efd..f78482ca0c 100644 >>> --- a/xen/include/asm-arm/device.h >>> +++ b/xen/include/asm-arm/device.h >>> @@ -6,6 +6,8 @@ >>> enum device_type >>> { >>> DEV_DT, >>> + DEV_ACPI, >> >> You don't use DEV_ACPI in this patch. So why is there? >> >>> + DEV_PCI, >>> }; >>> struct dev_archdata { >>> @@ -18,8 +20,13 @@ struct device >>> enum device_type type; >>> #ifdef CONFIG_HAS_DEVICE_TREE >>> struct dt_device_node *of_node; /* Used by drivers imported >>> from Linux */ >> >> As said on Sameer's patches, I was expecting a todo in the code after >> the discussion about leave of_node here. > I might have missed your comment on sameers patch, could you please restate >> >>> +#endif >>> +#ifdef CONFIG_ACPI >>> + void *acpi_node; >> >> You don't use acpi_node in this patch. So why is it there? >> >>> #endif >>> struct dev_archdata archdata; >>> + struct fwnode_handle *fwnode; /* firmware device node */ >> >> Ditto. >> >>> + struct iommu_fwspec *iommu_fwspec; >>> }; >>> typedef struct device device_t; >>> @@ -27,8 +34,8 @@ typedef struct device device_t; >>> #include <xen/device_tree.h> >>> /* TODO: Correctly implement dev_is_pci when PCI is supported on >>> ARM */ >>> -#define dev_is_pci(dev) ((void)(dev), 0) >>> -#define dev_is_dt(dev) ((dev->type == DEV_DT) >>> +#define dev_is_pci(dev) (dev->type == DEV_PCI) >>> +#define dev_is_dt(dev) (dev->type == DEV_DT) >> >> Those two changes does not belong to this patch. It is likely 2 >> separate patches: >> 1# fixing dev_is_dt because of the missing parenthese >> 2# implementing dev_is_dt >> >>> enum device_class >>> { >>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >>> index 33c8b221dc..56b169bae9 100644 >>> --- a/xen/include/xen/iommu.h >>> +++ b/xen/include/xen/iommu.h >>> @@ -208,4 +208,26 @@ 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; >>> +/** >>> + * 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 /* _IOMMU_H_ */ >>> >> >> Cheers, >> >
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 1aecf7cf34..408f44106d 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -13,6 +13,7 @@ */ #include <xen/sched.h> +#include <xen/fwnode.h> #include <xen/iommu.h> #include <xen/paging.h> #include <xen/guest_access.h> @@ -507,6 +508,80 @@ static void iommu_dump_p2m_table(unsigned char key) } } +/** + * fwnode_handle_put - Drop reference to a device node + * @fwnode: Pointer to the device node to drop the reference to. + * + * This has to be used when terminating device_for_each_child_node() iteration + * with break or return to prevent stale device node references from being left + * behind. + */ +void fwnode_handle_put(struct fwnode_handle *fwnode) +{ + fwnode_call_void_op(fwnode, put); +} + +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 = kzalloc(sizeof(*fwspec), GFP_KERNEL); + if (!fwspec) + return -ENOMEM; +#if 0 + of_node_get(to_of_node(iommu_fwnode)); +#endif + 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) { + fwnode_handle_put(fwspec->iommu_fwnode); + kfree(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; + size_t size; + int i; + + if (!fwspec) + return -EINVAL; + + size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]); + if (size > sizeof(*fwspec)) { + //TBD: fwspec = krealloc(dev->iommu_fwspec, size, GFP_KERNEL); + if (!fwspec) + return -ENOMEM; + + dev->iommu_fwspec = fwspec; + } + + for (i = 0; i < num_ids; i++) + fwspec->ids[fwspec->num_ids + i] = ids[i]; + + fwspec->num_ids += num_ids; + return 0; + +} /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h index 6734ae8efd..f78482ca0c 100644 --- a/xen/include/asm-arm/device.h +++ b/xen/include/asm-arm/device.h @@ -6,6 +6,8 @@ enum device_type { DEV_DT, + DEV_ACPI, + DEV_PCI, }; struct dev_archdata { @@ -18,8 +20,13 @@ struct device enum device_type type; #ifdef CONFIG_HAS_DEVICE_TREE struct dt_device_node *of_node; /* Used by drivers imported from Linux */ +#endif +#ifdef CONFIG_ACPI + void *acpi_node; #endif struct dev_archdata archdata; + struct fwnode_handle *fwnode; /* firmware device node */ + struct iommu_fwspec *iommu_fwspec; }; typedef struct device device_t; @@ -27,8 +34,8 @@ typedef struct device device_t; #include <xen/device_tree.h> /* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */ -#define dev_is_pci(dev) ((void)(dev), 0) -#define dev_is_dt(dev) ((dev->type == DEV_DT) +#define dev_is_pci(dev) (dev->type == DEV_PCI) +#define dev_is_dt(dev) (dev->type == DEV_DT) enum device_class { diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 33c8b221dc..56b169bae9 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -208,4 +208,26 @@ 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; +/** + * 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 /* _IOMMU_H_ */