| Message ID | 20221207061815.7404-3-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
Hi Vikram, On 07/12/2022 07:18, Vikram Garhwal wrote: > > > Remove master device from the IOMMU. Adding some description on the purpose would be beneficial. > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > --- > xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++ > xen/include/xen/iommu.h | 2 ++ > 2 files changed, 40 insertions(+) > > diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c > index 457df333a0..a8ba0b0d17 100644 > --- a/xen/drivers/passthrough/device_tree.c > +++ b/xen/drivers/passthrough/device_tree.c > @@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d) > return 0; > } > > +int iommu_remove_dt_device(struct dt_device_node *np) > +{ > + const struct iommu_ops *ops = iommu_get_ops(); > + struct device *dev = dt_to_dev(np); > + int rc; > + Aren't we missing a check if iommu is enabled? > + if ( !ops ) > + return -EOPNOTSUPP; -EINVAL to match the return values returned by other functions? > + > + spin_lock(&dtdevs_lock); > + > + if ( iommu_dt_device_is_assigned_locked(np) ) { Incorrect coding style. The closing brace should be placed on the next line. > + rc = -EBUSY; > + goto fail; > + } > + > + /* > + * The driver which supports generic IOMMU DT bindings must have > + * these callback implemented. > + */ > + if ( !ops->remove_device ) { Incorrect coding style. The closing brace should be placed on the next line. > + rc = -EOPNOTSUPP; -EINVAL to match the return values returned by other functions? > + goto fail; > + } > + > + /* > + * Remove master device from the IOMMU if latter is present and available. > + */ No need for a multi-line comment style. > + rc = ops->remove_device(0, dev); > + > + if ( rc == 0 ) !rc is preffered. > + iommu_fwspec_free(dev); > + > +fail: > + spin_unlock(&dtdevs_lock); > + return rc; > +} > + > int iommu_add_dt_device(struct dt_device_node *np) > { > const struct iommu_ops *ops = iommu_get_ops(); > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 4f22fc1bed..1b36c0419d 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -225,6 +225,8 @@ int iommu_release_dt_devices(struct domain *d); > */ > int iommu_add_dt_device(struct dt_device_node *np); > > +int iommu_remove_dt_device(struct dt_device_node *np); These prototypes look to be placed in order. So your function should be placed before add function. > + > int iommu_do_dt_domctl(struct xen_domctl *, struct domain *, > XEN_GUEST_HANDLE_PARAM(xen_domctl_t)); > > -- > 2.17.1 > > ~Michal
Hi, On 23/01/2023 10:00, Michal Orzel wrote: >> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >> --- >> xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++ >> xen/include/xen/iommu.h | 2 ++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c >> index 457df333a0..a8ba0b0d17 100644 >> --- a/xen/drivers/passthrough/device_tree.c >> +++ b/xen/drivers/passthrough/device_tree.c >> @@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d) >> return 0; >> } >> >> +int iommu_remove_dt_device(struct dt_device_node *np) >> +{ >> + const struct iommu_ops *ops = iommu_get_ops(); >> + struct device *dev = dt_to_dev(np); >> + int rc; >> + > Aren't we missing a check if iommu is enabled? > >> + if ( !ops ) >> + return -EOPNOTSUPP; > -EINVAL to match the return values returned by other functions? The meaning of -EINVAL is quite overloaded. So it would be better to use a mix of errno to help differentiating the error paths. In this case, '!ops' means there are no possibility (read "support") to remove the device. So I think -EOPNOTUSUPP is suitable. > >> + >> + spin_lock(&dtdevs_lock); >> + >> + if ( iommu_dt_device_is_assigned_locked(np) ) { > Incorrect coding style. The closing brace should be placed on the next line. > >> + rc = -EBUSY; >> + goto fail; >> + } >> + >> + /* >> + * The driver which supports generic IOMMU DT bindings must have >> + * these callback implemented. >> + */ >> + if ( !ops->remove_device ) { > Incorrect coding style. The closing brace should be placed on the next line. > >> + rc = -EOPNOTSUPP; > -EINVAL to match the return values returned by other functions? Ditto. Cheers,
Hi Michal, On 1/23/23 2:00 AM, Michal Orzel wrote: > Hi Vikram, > > On 07/12/2022 07:18, Vikram Garhwal wrote: >> >> Remove master device from the IOMMU. > Adding some description on the purpose would be beneficial. will do. >> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >> --- >> xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++ >> xen/include/xen/iommu.h | 2 ++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c >> index 457df333a0..a8ba0b0d17 100644 >> --- a/xen/drivers/passthrough/device_tree.c >> +++ b/xen/drivers/passthrough/device_tree.c >> @@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d) >> return 0; >> } >> >> +int iommu_remove_dt_device(struct dt_device_node *np) >> +{ >> + const struct iommu_ops *ops = iommu_get_ops(); >> + struct device *dev = dt_to_dev(np); >> + int rc; >> + > Aren't we missing a check if iommu is enabled? IIUC your question: There is only one caller which is in dynamic programming part handle_remove_irq_iommu(). The call only happen if the dt_node has iommu property. >> + if ( !ops ) >> + return -EOPNOTSUPP; > -EINVAL to match the return values returned by other functions? > >> + >> + spin_lock(&dtdevs_lock); >> + >> + if ( iommu_dt_device_is_assigned_locked(np) ) { > Incorrect coding style. The closing brace should be placed on the next line. Fixed this for v5. > >> + rc = -EBUSY; >> + goto fail; >> + } >> + >> + /* >> + * The driver which supports generic IOMMU DT bindings must have >> + * these callback implemented. >> + */ >> + if ( !ops->remove_device ) { > Incorrect coding style. The closing brace should be placed on the next line. Fixed this for v5. > >> + rc = -EOPNOTSUPP; > -EINVAL to match the return values returned by other functions? > >> + goto fail; >> + } >> + >> + /* >> + * Remove master device from the IOMMU if latter is present and available. >> + */ > No need for a multi-line comment style. Fixed this for v5. > >> + rc = ops->remove_device(0, dev); >> + >> + if ( rc == 0 ) > !rc is preffered. Fixed this for v5. > >> + iommu_fwspec_free(dev); >> + >> +fail: >> + spin_unlock(&dtdevs_lock); >> + return rc; >> +} >> + >> int iommu_add_dt_device(struct dt_device_node *np) >> { >> const struct iommu_ops *ops = iommu_get_ops(); >> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h >> index 4f22fc1bed..1b36c0419d 100644 >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -225,6 +225,8 @@ int iommu_release_dt_devices(struct domain *d); >> */ >> int iommu_add_dt_device(struct dt_device_node *np); >> >> +int iommu_remove_dt_device(struct dt_device_node *np); > These prototypes look to be placed in order. So your function should be > placed before add function. Fixed this for v5. > >> + >> int iommu_do_dt_domctl(struct xen_domctl *, struct domain *, >> XEN_GUEST_HANDLE_PARAM(xen_domctl_t)); >> >> -- >> 2.17.1 >> >> > ~Michal
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c index 457df333a0..a8ba0b0d17 100644 --- a/xen/drivers/passthrough/device_tree.c +++ b/xen/drivers/passthrough/device_tree.c @@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d) return 0; } +int iommu_remove_dt_device(struct dt_device_node *np) +{ + const struct iommu_ops *ops = iommu_get_ops(); + struct device *dev = dt_to_dev(np); + int rc; + + if ( !ops ) + return -EOPNOTSUPP; + + spin_lock(&dtdevs_lock); + + if ( iommu_dt_device_is_assigned_locked(np) ) { + rc = -EBUSY; + goto fail; + } + + /* + * The driver which supports generic IOMMU DT bindings must have + * these callback implemented. + */ + if ( !ops->remove_device ) { + rc = -EOPNOTSUPP; + goto fail; + } + + /* + * Remove master device from the IOMMU if latter is present and available. + */ + rc = ops->remove_device(0, dev); + + if ( rc == 0 ) + iommu_fwspec_free(dev); + +fail: + spin_unlock(&dtdevs_lock); + return rc; +} + int iommu_add_dt_device(struct dt_device_node *np) { const struct iommu_ops *ops = iommu_get_ops(); diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 4f22fc1bed..1b36c0419d 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -225,6 +225,8 @@ int iommu_release_dt_devices(struct domain *d); */ int iommu_add_dt_device(struct dt_device_node *np); +int iommu_remove_dt_device(struct dt_device_node *np); + int iommu_do_dt_domctl(struct xen_domctl *, struct domain *, XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
Remove master device from the IOMMU. Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> --- xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++ xen/include/xen/iommu.h | 2 ++ 2 files changed, 40 insertions(+)