| Message ID | 20230518210658.66156-3-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SMMU handling for PCIe Passthrough on ARM | expand |
Hi Stewart, On 18/05/2023 23:06, Stewart Hildebrand wrote: > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Move code for processing DT IOMMU specifier to a separate helper. > This helper will be re-used for adding PCI devices by the subsequent > patches as we will need exact the same actions for processing > DT PCI-IOMMU specifier. > > While at it introduce NO_IOMMU to avoid magic "1". > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> # rename > --- > v2->v3: > * no change > > v1->v2: > * no change > > downstream->v1: > * trivial rebase > * s/dt_iommu_xlate/iommu_dt_xlate/ > > (cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from > the downstream branch poc/pci-passthrough from > https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git) > --- > xen/drivers/passthrough/device_tree.c | 42 +++++++++++++++++---------- > 1 file changed, 27 insertions(+), 15 deletions(-) > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > index b5bd13393b56..1b50f4670944 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -127,15 +127,39 @@ int iommu_release_dt_devices(struct domain *d) > return 0; > } > > +/* This correlation must not be altered */ > +#define NO_IOMMU 1 > + > +static int iommu_dt_xlate(struct device *dev, > + struct dt_phandle_args *iommu_spec) I think iommu_spec can be const. > +{ > + const struct iommu_ops *ops = iommu_get_ops(); > + int rc; > + > + if ( !dt_device_is_available(iommu_spec->np) ) > + return NO_IOMMU; > + > + rc = iommu_fwspec_init(dev, &iommu_spec->np->dev); > + if ( rc ) > + return rc; > + > + /* > + * Provide DT IOMMU specifier which describes the IOMMU master > + * interfaces of that device (device IDs, etc) to the driver. > + * The driver is responsible to decide how to interpret them. > + */ > + return ops->dt_xlate(dev, iommu_spec); Wouldn't it be better to move the check (!ops->dt_xlate) from iommu_add_dt_device to this helper? After all it is the only function that calls dt_xlate so for me it would be a natural placement. Looking at the next patch it will also reduce the similar check in iommu_add_dt_pci_sideband_ids. ~Michal
On 5/22/23 10:48, Michal Orzel wrote: > Hi Stewart, > > On 18/05/2023 23:06, Stewart Hildebrand wrote: >> >> >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Move code for processing DT IOMMU specifier to a separate helper. >> This helper will be re-used for adding PCI devices by the subsequent >> patches as we will need exact the same actions for processing >> DT PCI-IOMMU specifier. >> >> While at it introduce NO_IOMMU to avoid magic "1". >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> # rename >> --- >> v2->v3: >> * no change >> >> v1->v2: >> * no change >> >> downstream->v1: >> * trivial rebase >> * s/dt_iommu_xlate/iommu_dt_xlate/ >> >> (cherry picked from commit c26bab0415ca303df86aba1d06ef8edc713734d3 from >> the downstream branch poc/pci-passthrough from >> https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc.git) >> --- >> xen/drivers/passthrough/device_tree.c | 42 +++++++++++++++++---------- >> 1 file changed, 27 insertions(+), 15 deletions(-) >> >> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c >> index b5bd13393b56..1b50f4670944 100644 >> --- a/xen/drivers/passthrough/device_tree.c >> +++ b/xen/drivers/passthrough/device_tree.c >> @@ -127,15 +127,39 @@ int iommu_release_dt_devices(struct domain *d) >> return 0; >> } >> >> +/* This correlation must not be altered */ >> +#define NO_IOMMU 1 >> + >> +static int iommu_dt_xlate(struct device *dev, >> + struct dt_phandle_args *iommu_spec) > I think iommu_spec can be const. Yes, good catch >> +{ >> + const struct iommu_ops *ops = iommu_get_ops(); >> + int rc; >> + >> + if ( !dt_device_is_available(iommu_spec->np) ) >> + return NO_IOMMU; >> + >> + rc = iommu_fwspec_init(dev, &iommu_spec->np->dev); >> + if ( rc ) >> + return rc; >> + >> + /* >> + * Provide DT IOMMU specifier which describes the IOMMU master >> + * interfaces of that device (device IDs, etc) to the driver. >> + * The driver is responsible to decide how to interpret them. >> + */ >> + return ops->dt_xlate(dev, iommu_spec); > Wouldn't it be better to move the check (!ops->dt_xlate) from iommu_add_dt_device to this helper? > After all it is the only function that calls dt_xlate so for me it would be a natural placement. > Looking at the next patch it will also reduce the similar check in iommu_add_dt_pci_sideband_ids. Yes, I will move it
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index b5bd13393b56..1b50f4670944 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -127,15 +127,39 @@ int iommu_release_dt_devices(struct domain *d) return 0; } +/* This correlation must not be altered */ +#define NO_IOMMU 1 + +static int iommu_dt_xlate(struct device *dev, + struct dt_phandle_args *iommu_spec) +{ + const struct iommu_ops *ops = iommu_get_ops(); + int rc; + + if ( !dt_device_is_available(iommu_spec->np) ) + return NO_IOMMU; + + rc = iommu_fwspec_init(dev, &iommu_spec->np->dev); + if ( rc ) + return rc; + + /* + * Provide DT IOMMU specifier which describes the IOMMU master + * interfaces of that device (device IDs, etc) to the driver. + * The driver is responsible to decide how to interpret them. + */ + return ops->dt_xlate(dev, iommu_spec); +} + int iommu_add_dt_device(struct dt_device_node *np) { const struct iommu_ops *ops = iommu_get_ops(); struct dt_phandle_args iommu_spec; struct device *dev = dt_to_dev(np); - int rc = 1, index = 0; + int rc = NO_IOMMU, index = 0; if ( !iommu_enabled ) - return 1; + return NO_IOMMU; if ( !ops ) return -EINVAL; @@ -164,19 +188,7 @@ int iommu_add_dt_device(struct dt_device_node *np) if ( !ops->add_device || !ops->dt_xlate ) return -EINVAL; - if ( !dt_device_is_available(iommu_spec.np) ) - break; - - rc = iommu_fwspec_init(dev, &iommu_spec.np->dev); - if ( rc ) - break; - - /* - * Provide DT IOMMU specifier which describes the IOMMU master - * interfaces of that device (device IDs, etc) to the driver. - * The driver is responsible to decide how to interpret them. - */ - rc = ops->dt_xlate(dev, &iommu_spec); + rc = iommu_dt_xlate(dev, &iommu_spec); if ( rc ) break;