| Message ID | 20230923012511.10379-5-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMUFD Dirty Tracking | expand |
On Sat, Sep 23, 2023 at 02:24:56AM +0100, Joao Martins wrote: > Throughout IOMMU domain lifetime that wants to use dirty tracking, some > guarantees are needed such that any device attached to the iommu_domain > supports dirty tracking. > > The idea is to handle a case where IOMMU in the system are assymetric > feature-wise and thus the capability may not be supported for all devices. > The enforcement is done by adding a flag into HWPT_ALLOC namely: > > IOMMUFD_HWPT_ALLOC_ENFORCE_DIRTY > > .. Passed in HWPT_ALLOC ioctl() flags. The enforcement is done by creating > a iommu_domain via domain_alloc_user() and validating the requested flags > with what the device IOMMU supports (and failing accordingly) advertised). > Advertising the new IOMMU domain feature flag requires that the individual > iommu driver capability is supported when a future device attachment > happens. > > Link: https://lore.kernel.org/kvm/20220721142421.GB4609@nvidia.com/ > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > drivers/iommu/iommufd/hw_pagetable.c | 8 ++++++-- > include/uapi/linux/iommufd.h | 3 +++ > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c > index 26a8a818ffa3..32e259245314 100644 > --- a/drivers/iommu/iommufd/hw_pagetable.c > +++ b/drivers/iommu/iommufd/hw_pagetable.c > @@ -83,7 +83,9 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, > > lockdep_assert_held(&ioas->mutex); > > - if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ops->domain_alloc_user) > + if ((flags & (IOMMU_HWPT_ALLOC_NEST_PARENT| > + IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) && > + !ops->domain_alloc_user) > return ERR_PTR(-EOPNOTSUPP); This seems strange, why are we testing flags here? shouldn't this just be if (flags && !ops->domain_alloc_user) return ERR_PTR(-EOPNOTSUPP); ? > hwpt = iommufd_object_alloc(ictx, hwpt, IOMMUFD_OBJ_HW_PAGETABLE); > @@ -157,7 +159,9 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) > struct iommufd_ioas *ioas; > int rc; > > - if (cmd->flags & ~IOMMU_HWPT_ALLOC_NEST_PARENT || cmd->__reserved) > + if ((cmd->flags & > + ~(IOMMU_HWPT_ALLOC_NEST_PARENT|IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) || > + cmd->__reserved) > return -EOPNOTSUPP; Please checkpatch your stuff, and even better feed the patches to clang-format and do most of what it says. Otherwise seems like the right thing to do Jason
On 13/10/2023 16:52, Jason Gunthorpe wrote: > On Sat, Sep 23, 2023 at 02:24:56AM +0100, Joao Martins wrote: >> Throughout IOMMU domain lifetime that wants to use dirty tracking, some >> guarantees are needed such that any device attached to the iommu_domain >> supports dirty tracking. >> >> The idea is to handle a case where IOMMU in the system are assymetric >> feature-wise and thus the capability may not be supported for all devices. >> The enforcement is done by adding a flag into HWPT_ALLOC namely: >> >> IOMMUFD_HWPT_ALLOC_ENFORCE_DIRTY >> >> .. Passed in HWPT_ALLOC ioctl() flags. The enforcement is done by creating >> a iommu_domain via domain_alloc_user() and validating the requested flags >> with what the device IOMMU supports (and failing accordingly) advertised). >> Advertising the new IOMMU domain feature flag requires that the individual >> iommu driver capability is supported when a future device attachment >> happens. >> >> Link: https://lore.kernel.org/kvm/20220721142421.GB4609@nvidia.com/ >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> drivers/iommu/iommufd/hw_pagetable.c | 8 ++++++-- >> include/uapi/linux/iommufd.h | 3 +++ >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c >> index 26a8a818ffa3..32e259245314 100644 >> --- a/drivers/iommu/iommufd/hw_pagetable.c >> +++ b/drivers/iommu/iommufd/hw_pagetable.c >> @@ -83,7 +83,9 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, >> >> lockdep_assert_held(&ioas->mutex); >> >> - if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ops->domain_alloc_user) >> + if ((flags & (IOMMU_HWPT_ALLOC_NEST_PARENT| >> + IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) && >> + !ops->domain_alloc_user) >> return ERR_PTR(-EOPNOTSUPP); > > This seems strange, why are we testing flags here? shouldn't this just > be > > if (flags && !ops->domain_alloc_user) > return ERR_PTR(-EOPNOTSUPP); > > ? Yeah makes sense, let me switch to that. It achieves the same without into the weeds of missing a flag update. And any flag essentially requires alloc_user regardless. > >> hwpt = iommufd_object_alloc(ictx, hwpt, IOMMUFD_OBJ_HW_PAGETABLE); >> @@ -157,7 +159,9 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) >> struct iommufd_ioas *ioas; >> int rc; >> >> - if (cmd->flags & ~IOMMU_HWPT_ALLOC_NEST_PARENT || cmd->__reserved) >> + if ((cmd->flags & >> + ~(IOMMU_HWPT_ALLOC_NEST_PARENT|IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) || >> + cmd->__reserved) >> return -EOPNOTSUPP; > > Please checkpatch your stuff, I always do this, and there was no issues reported on this patch. Sometimes there's one or another I ignore albeit very rarely (e.g. passing 1 character post 80 column gap and which fixing makes the code uglier). > and even better feed the patches to > clang-format and do most of what it says. > This one I don't (but I wasn't aware either that it's a thing) > Otherwise seems like the right thing to do > > Jason
On Fri, Oct 13, 2023 at 05:14:26PM +0100, Joao Martins wrote: > >> hwpt = iommufd_object_alloc(ictx, hwpt, IOMMUFD_OBJ_HW_PAGETABLE); > >> @@ -157,7 +159,9 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) > >> struct iommufd_ioas *ioas; > >> int rc; > >> > >> - if (cmd->flags & ~IOMMU_HWPT_ALLOC_NEST_PARENT || cmd->__reserved) > >> + if ((cmd->flags & > >> + ~(IOMMU_HWPT_ALLOC_NEST_PARENT|IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) || > >> + cmd->__reserved) > >> return -EOPNOTSUPP; > > > > Please checkpatch your stuff, > > I always do this, and there was no issues reported on this patch. Really? The missing spaces around ' | ' are not kernel style.. Jason
On 13/10/2023 17:16, Jason Gunthorpe wrote: > On Fri, Oct 13, 2023 at 05:14:26PM +0100, Joao Martins wrote: >>>> hwpt = iommufd_object_alloc(ictx, hwpt, IOMMUFD_OBJ_HW_PAGETABLE); >>>> @@ -157,7 +159,9 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) >>>> struct iommufd_ioas *ioas; >>>> int rc; >>>> >>>> - if (cmd->flags & ~IOMMU_HWPT_ALLOC_NEST_PARENT || cmd->__reserved) >>>> + if ((cmd->flags & >>>> + ~(IOMMU_HWPT_ALLOC_NEST_PARENT|IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) || >>>> + cmd->__reserved) >>>> return -EOPNOTSUPP; >>> >>> Please checkpatch your stuff, >> >> I always do this, and there was no issues reported on this patch. > > Really? The missing spaces around ' | ' are not kernel style.. > I just ran it and it doesn't complain really. But btw I am not doing spaces around single | ? Only around ' || ' and that is quite common in kernel code?
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index 26a8a818ffa3..32e259245314 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -83,7 +83,9 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, lockdep_assert_held(&ioas->mutex); - if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ops->domain_alloc_user) + if ((flags & (IOMMU_HWPT_ALLOC_NEST_PARENT| + IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) && + !ops->domain_alloc_user) return ERR_PTR(-EOPNOTSUPP); hwpt = iommufd_object_alloc(ictx, hwpt, IOMMUFD_OBJ_HW_PAGETABLE); @@ -157,7 +159,9 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) struct iommufd_ioas *ioas; int rc; - if (cmd->flags & ~IOMMU_HWPT_ALLOC_NEST_PARENT || cmd->__reserved) + if ((cmd->flags & + ~(IOMMU_HWPT_ALLOC_NEST_PARENT|IOMMU_HWPT_ALLOC_ENFORCE_DIRTY)) || + cmd->__reserved) return -EOPNOTSUPP; idev = iommufd_get_device(ucmd, cmd->dev_id); diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h index 4a7c5c8fdbb4..cd94a9d8ce66 100644 --- a/include/uapi/linux/iommufd.h +++ b/include/uapi/linux/iommufd.h @@ -352,9 +352,12 @@ struct iommu_vfio_ioas { * @IOMMU_HWPT_ALLOC_NEST_PARENT: If set, allocate a domain which can serve * as the parent domain in the nesting * configuration. + * @IOMMU_HWPT_ALLOC_ENFORCE_DIRTY: Dirty tracking support for device IOMMU is + * enforced on device attachment */ enum iommufd_hwpt_alloc_flags { IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0, + IOMMU_HWPT_ALLOC_ENFORCE_DIRTY = 1 << 1, }; /**
Throughout IOMMU domain lifetime that wants to use dirty tracking, some guarantees are needed such that any device attached to the iommu_domain supports dirty tracking. The idea is to handle a case where IOMMU in the system are assymetric feature-wise and thus the capability may not be supported for all devices. The enforcement is done by adding a flag into HWPT_ALLOC namely: IOMMUFD_HWPT_ALLOC_ENFORCE_DIRTY .. Passed in HWPT_ALLOC ioctl() flags. The enforcement is done by creating a iommu_domain via domain_alloc_user() and validating the requested flags with what the device IOMMU supports (and failing accordingly) advertised). Advertising the new IOMMU domain feature flag requires that the individual iommu driver capability is supported when a future device attachment happens. Link: https://lore.kernel.org/kvm/20220721142421.GB4609@nvidia.com/ Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- drivers/iommu/iommufd/hw_pagetable.c | 8 ++++++-- include/uapi/linux/iommufd.h | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-)