| Message ID | 20230607030220.22698-4-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMU handling for PCIe Passthrough on ARM | expand |
Hi Stewart, On 07/06/2023 04:02, 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> > --- > v3->v4: > * make dt_phandle_args *iommu_spec const > * move !ops->add_device check to helper > > 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 | 47 ++++++++++++++++++--------- > 1 file changed, 31 insertions(+), 16 deletions(-) > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > index c60e78eaf556..ff9e66ebf92a 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -127,15 +127,42 @@ int iommu_release_dt_devices(struct domain *d) > return 0; > } > > +/* This correlation must not be altered */ Please expand why. > +#define NO_IOMMU 1 Given that the value is returned, should not this be moved to an header and used by the callers? > + > +static int iommu_dt_xlate(struct device *dev, > + const struct dt_phandle_args *iommu_spec) > +{ > + const struct iommu_ops *ops = iommu_get_ops(); > + int rc; > + > + if ( !ops->dt_xlate ) > + return -EINVAL; > + > + 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; > @@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np) > * The driver which supports generic IOMMU DT bindings must have > * these callback implemented. The 's' was missing to callback. But now, I think you want to replace 'these' with 'this' as you move the second check. > */ > - if ( !ops->add_device || !ops->dt_xlate ) > + if ( !ops->add_device ) > 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; > Cheers,
On 6/29/23 18:29, Julien Grall wrote: > Hi Stewart, > > On 07/06/2023 04:02, 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> >> --- >> v3->v4: >> * make dt_phandle_args *iommu_spec const >> * move !ops->add_device check to helper >> >> 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 | 47 ++++++++++++++++++--------- >> 1 file changed, 31 insertions(+), 16 deletions(-) >> >> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c >> index c60e78eaf556..ff9e66ebf92a 100644 >> --- a/xen/drivers/passthrough/device_tree.c >> +++ b/xen/drivers/passthrough/device_tree.c >> @@ -127,15 +127,42 @@ int iommu_release_dt_devices(struct domain *d) >> return 0; >> } >> >> +/* This correlation must not be altered */ > > Please expand why. I don't have any insight regarding the rationale for why the comment was added. I don't believe the comment is adding any value, so I'll remove it. >> +#define NO_IOMMU 1 > > Given that the value is returned, should not this be moved to an header > and used by the callers? Moving it to a header: yes. I'll move it to xen/include/xen/iommu.h, as that is where the iommu_add_dt_device() prototype also lives. Used by callers: the callers currently are checking for error by comparing ( rc < 0 ). I think that's okay to leave as is since the comment in iommu.h by the prototype describe the possible return value for iommu_add_dt_device() as: * Return values: * 0 : device is protected by an IOMMU * <0 : device is not protected by an IOMMU, but must be (error condition) * >0 : device doesn't need to be protected by an IOMMU * (IOMMU is not enabled/present or device is not connected to it). >> + >> +static int iommu_dt_xlate(struct device *dev, >> + const struct dt_phandle_args *iommu_spec) >> +{ >> + const struct iommu_ops *ops = iommu_get_ops(); >> + int rc; >> + >> + if ( !ops->dt_xlate ) >> + return -EINVAL; >> + >> + 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; >> @@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np) >> * The driver which supports generic IOMMU DT bindings must have >> * these callback implemented. > > The 's' was missing to callback. But now, I think you want to replace > 'these' with 'this' as you move the second check. I will fix it >> */ >> - if ( !ops->add_device || !ops->dt_xlate ) >> + if ( !ops->add_device ) >> 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; >> > > Cheers, > > -- > Julien Grall
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index c60e78eaf556..ff9e66ebf92a 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -127,15 +127,42 @@ 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, + const struct dt_phandle_args *iommu_spec) +{ + const struct iommu_ops *ops = iommu_get_ops(); + int rc; + + if ( !ops->dt_xlate ) + return -EINVAL; + + 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; @@ -163,22 +190,10 @@ int iommu_add_dt_device(struct dt_device_node *np) * The driver which supports generic IOMMU DT bindings must have * these callback implemented. */ - if ( !ops->add_device || !ops->dt_xlate ) + if ( !ops->add_device ) 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;