| Message ID | 20230923012511.10379-17-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMUFD Dirty Tracking | expand |
Hi Joao, On 9/23/2023 8:25 AM, Joao Martins wrote: > Add the domain_alloc_user op implementation. To that end, refactor > amd_iommu_domain_alloc() to receive a dev pointer and flags, while > renaming it to .. such that it becomes a common function shared with > domain_alloc_user() implementation. The sole difference with > domain_alloc_user() is that we initialize also other fields that > iommu_domain_alloc() does. It lets it return the iommu domain > correctly initialized in one function. > > This is in preparation to add dirty enforcement on AMD implementation > of domain_alloc_user. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > drivers/iommu/amd/iommu.c | 46 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 43 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 95bd7c25ba6f..af36c627022f 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -37,6 +37,7 @@ > #include <asm/iommu.h> > #include <asm/gart.h> > #include <asm/dma.h> > +#include <uapi/linux/iommufd.h> > > #include "amd_iommu.h" > #include "../dma-iommu.h" > @@ -2155,7 +2156,10 @@ static inline u64 dma_max_address(void) > return ((1ULL << PM_LEVEL_SHIFT(amd_iommu_gpt_level)) - 1); > } > > -static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) > +static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, > + struct amd_iommu *iommu, > + struct device *dev, > + u32 flags) Instead of passing in the struct amd_iommu here, what if we just derive it in the do_iommu_domain_alloc() as needed? This way, we don't need to ... (see below) > { > struct protection_domain *domain; > > @@ -2164,19 +2168,54 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) > * default to use IOMMU_DOMAIN_DMA[_FQ]. > */ > if (amd_iommu_snp_en && (type == IOMMU_DOMAIN_IDENTITY)) > - return NULL; > + return ERR_PTR(-EINVAL); > > domain = protection_domain_alloc(type); > if (!domain) > - return NULL; > + return ERR_PTR(-ENOMEM); > > domain->domain.geometry.aperture_start = 0; > domain->domain.geometry.aperture_end = dma_max_address(); > domain->domain.geometry.force_aperture = true; > > + if (dev) { > + domain->domain.type = type; > + domain->domain.pgsize_bitmap = > + iommu->iommu.ops->pgsize_bitmap; > + domain->domain.ops = > + iommu->iommu.ops->default_domain_ops; > + } > + > return &domain->domain; > } > > +static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) > +{ > + struct iommu_domain *domain; > + > + domain = do_iommu_domain_alloc(type, NULL, NULL, 0); ... pass iommu = NULL here unnecessarily. > + if (IS_ERR(domain)) > + return NULL; > + > + return domain; > +} > + > +static struct iommu_domain *amd_iommu_domain_alloc_user(struct device *dev, > + u32 flags) > +{ > + unsigned int type = IOMMU_DOMAIN_UNMANAGED; > + struct amd_iommu *iommu; > + > + iommu = rlookup_amd_iommu(dev); > + if (!iommu) > + return ERR_PTR(-ENODEV); We should not need to derive this here. Other than this part. Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Thanks, Suravee
On 17/10/2023 03:00, Suthikulpanit, Suravee wrote: > Hi Joao, > > On 9/23/2023 8:25 AM, Joao Martins wrote: >> Add the domain_alloc_user op implementation. To that end, refactor >> amd_iommu_domain_alloc() to receive a dev pointer and flags, while >> renaming it to .. such that it becomes a common function shared with >> domain_alloc_user() implementation. The sole difference with >> domain_alloc_user() is that we initialize also other fields that >> iommu_domain_alloc() does. It lets it return the iommu domain >> correctly initialized in one function. >> >> This is in preparation to add dirty enforcement on AMD implementation >> of domain_alloc_user. >> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> drivers/iommu/amd/iommu.c | 46 ++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 43 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c >> index 95bd7c25ba6f..af36c627022f 100644 >> --- a/drivers/iommu/amd/iommu.c >> +++ b/drivers/iommu/amd/iommu.c >> @@ -37,6 +37,7 @@ >> #include <asm/iommu.h> >> #include <asm/gart.h> >> #include <asm/dma.h> >> +#include <uapi/linux/iommufd.h> >> #include "amd_iommu.h" >> #include "../dma-iommu.h" >> @@ -2155,7 +2156,10 @@ static inline u64 dma_max_address(void) >> return ((1ULL << PM_LEVEL_SHIFT(amd_iommu_gpt_level)) - 1); >> } >> -static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) >> +static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, >> + struct amd_iommu *iommu, >> + struct device *dev, >> + u32 flags) > > Instead of passing in the struct amd_iommu here, what if we just derive it in > the do_iommu_domain_alloc() as needed? This way, we don't need to ... (see below) > Hmm, you mean to derive amd_iommu from the dev pointer. Yeah, sounds good. >> { >> struct protection_domain *domain; >> @@ -2164,19 +2168,54 @@ static struct iommu_domain >> *amd_iommu_domain_alloc(unsigned type) >> * default to use IOMMU_DOMAIN_DMA[_FQ]. >> */ >> if (amd_iommu_snp_en && (type == IOMMU_DOMAIN_IDENTITY)) >> - return NULL; >> + return ERR_PTR(-EINVAL); >> domain = protection_domain_alloc(type); >> if (!domain) >> - return NULL; >> + return ERR_PTR(-ENOMEM); >> domain->domain.geometry.aperture_start = 0; >> domain->domain.geometry.aperture_end = dma_max_address(); >> domain->domain.geometry.force_aperture = true; >> + if (dev) { >> + domain->domain.type = type; >> + domain->domain.pgsize_bitmap = >> + iommu->iommu.ops->pgsize_bitmap; >> + domain->domain.ops = >> + iommu->iommu.ops->default_domain_ops; >> + } >> + >> return &domain->domain; >> } >> +static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) >> +{ >> + struct iommu_domain *domain; >> + >> + domain = do_iommu_domain_alloc(type, NULL, NULL, 0); > > ... pass iommu = NULL here unnecessarily. > OK >> + if (IS_ERR(domain)) >> + return NULL; >> + >> + return domain; >> +} >> + >> +static struct iommu_domain *amd_iommu_domain_alloc_user(struct device *dev, >> + u32 flags) >> +{ >> + unsigned int type = IOMMU_DOMAIN_UNMANAGED; >> + struct amd_iommu *iommu; >> + >> + iommu = rlookup_amd_iommu(dev); >> + if (!iommu) >> + return ERR_PTR(-ENODEV); > > We should not need to derive this here. > > Other than this part. > OK, will do. > Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > Thanks! Here's the diff diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index af36c627022f..cfc7d2992aa6 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -2157,11 +2157,17 @@ static inline u64 dma_max_address(void) } static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, - struct amd_iommu *iommu, struct device *dev, u32 flags) { struct protection_domain *domain; + struct amd_iommu *iommu = NULL; + + if (dev) { + iommu = rlookup_amd_iommu(dev); + if (!iommu) + return ERR_PTR(-ENODEV); + } /* * Since DTE[Mode]=0 is prohibited on SNP-enabled system, @@ -2178,7 +2184,7 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, domain->domain.geometry.aperture_end = dma_max_address(); domain->domain.geometry.force_aperture = true; - if (dev) { + if (iommu) { domain->domain.type = type; domain->domain.pgsize_bitmap = iommu->iommu.ops->pgsize_bitmap; @@ -2193,7 +2199,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) { struct iommu_domain *domain; - domain = do_iommu_domain_alloc(type, NULL, NULL, 0); + domain = do_iommu_domain_alloc(type, NULL, 0); if (IS_ERR(domain)) return NULL; @@ -2204,16 +2210,11 @@ static struct iommu_domain *amd_iommu_domain_alloc_user(struct device *dev, u32 flags) { unsigned int type = IOMMU_DOMAIN_UNMANAGED; - struct amd_iommu *iommu; - - iommu = rlookup_amd_iommu(dev); - if (!iommu) - return ERR_PTR(-ENODEV); if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) return ERR_PTR(-EOPNOTSUPP); - return do_iommu_domain_alloc(type, iommu, dev, flags); + return do_iommu_domain_alloc(type, dev, flags); }
On Tue, Oct 17, 2023 at 10:07:11AM +0100, Joao Martins wrote: > > static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, > - struct amd_iommu *iommu, > struct device *dev, > u32 flags) > { > struct protection_domain *domain; > + struct amd_iommu *iommu = NULL; > + > + if (dev) { > + iommu = rlookup_amd_iommu(dev); > + if (!iommu) This really shouldn't be rlookup_amd_iommu, didn't the series fixing this get merged? Jason
On 17/10/2023 14:10, Jason Gunthorpe wrote: > On Tue, Oct 17, 2023 at 10:07:11AM +0100, Joao Martins wrote: >> >> static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, >> - struct amd_iommu *iommu, >> struct device *dev, >> u32 flags) >> { >> struct protection_domain *domain; >> + struct amd_iommu *iommu = NULL; >> + >> + if (dev) { >> + iommu = rlookup_amd_iommu(dev); >> + if (!iommu) > > This really shouldn't be rlookup_amd_iommu, didn't the series fixing > this get merged? From the latest linux-next, it's still there.
On 17/10/2023 15:14, Joao Martins wrote: > On 17/10/2023 14:10, Jason Gunthorpe wrote: >> On Tue, Oct 17, 2023 at 10:07:11AM +0100, Joao Martins wrote: >>> >>> static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, >>> - struct amd_iommu *iommu, >>> struct device *dev, >>> u32 flags) >>> { >>> struct protection_domain *domain; >>> + struct amd_iommu *iommu = NULL; >>> + >>> + if (dev) { >>> + iommu = rlookup_amd_iommu(dev); >>> + if (!iommu) >> >> This really shouldn't be rlookup_amd_iommu, didn't the series fixing >> this get merged? > > From the latest linux-next, it's still there. > I'm assuming you refer to this new helper: https://lore.kernel.org/linux-iommu/20231013151652.6008-3-vasant.hegde@amd.com/ But it's part 3 out of a 4-part multi-series; and only the first part has been merged. Joao
On Tue, Oct 17, 2023 at 03:37:47PM +0100, Joao Martins wrote: > On 17/10/2023 15:14, Joao Martins wrote: > > On 17/10/2023 14:10, Jason Gunthorpe wrote: > >> On Tue, Oct 17, 2023 at 10:07:11AM +0100, Joao Martins wrote: > >>> > >>> static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, > >>> - struct amd_iommu *iommu, > >>> struct device *dev, > >>> u32 flags) > >>> { > >>> struct protection_domain *domain; > >>> + struct amd_iommu *iommu = NULL; > >>> + > >>> + if (dev) { > >>> + iommu = rlookup_amd_iommu(dev); > >>> + if (!iommu) > >> > >> This really shouldn't be rlookup_amd_iommu, didn't the series fixing > >> this get merged? > > > > From the latest linux-next, it's still there. > > > I'm assuming you refer to this new helper: > > https://lore.kernel.org/linux-iommu/20231013151652.6008-3-vasant.hegde@amd.com/ > > But it's part 3 out of a 4-part multi-series; and only the first part has been > merged. Okay, then nothing to do here :\ Jason
On 10/17/2023 8:07 PM, Joao Martins wrote: > On 17/10/2023 15:14, Joao Martins wrote: >> On 17/10/2023 14:10, Jason Gunthorpe wrote: >>> On Tue, Oct 17, 2023 at 10:07:11AM +0100, Joao Martins wrote: >>>> >>>> static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, >>>> - struct amd_iommu *iommu, >>>> struct device *dev, >>>> u32 flags) >>>> { >>>> struct protection_domain *domain; >>>> + struct amd_iommu *iommu = NULL; >>>> + >>>> + if (dev) { >>>> + iommu = rlookup_amd_iommu(dev); >>>> + if (!iommu) >>> >>> This really shouldn't be rlookup_amd_iommu, didn't the series fixing >>> this get merged? >> >> From the latest linux-next, it's still there. >> > I'm assuming you refer to this new helper: > > https://lore.kernel.org/linux-iommu/20231013151652.6008-3-vasant.hegde@amd.com/ > > But it's part 3 out of a 4-part multi-series; and only the first part has been > merged. That's correct. Part 2 parts are merged. For now you can use rlookup_amd_iommu(). I can fix it later. -Vasant
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 95bd7c25ba6f..af36c627022f 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -37,6 +37,7 @@ #include <asm/iommu.h> #include <asm/gart.h> #include <asm/dma.h> +#include <uapi/linux/iommufd.h> #include "amd_iommu.h" #include "../dma-iommu.h" @@ -2155,7 +2156,10 @@ static inline u64 dma_max_address(void) return ((1ULL << PM_LEVEL_SHIFT(amd_iommu_gpt_level)) - 1); } -static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) +static struct iommu_domain *do_iommu_domain_alloc(unsigned int type, + struct amd_iommu *iommu, + struct device *dev, + u32 flags) { struct protection_domain *domain; @@ -2164,19 +2168,54 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) * default to use IOMMU_DOMAIN_DMA[_FQ]. */ if (amd_iommu_snp_en && (type == IOMMU_DOMAIN_IDENTITY)) - return NULL; + return ERR_PTR(-EINVAL); domain = protection_domain_alloc(type); if (!domain) - return NULL; + return ERR_PTR(-ENOMEM); domain->domain.geometry.aperture_start = 0; domain->domain.geometry.aperture_end = dma_max_address(); domain->domain.geometry.force_aperture = true; + if (dev) { + domain->domain.type = type; + domain->domain.pgsize_bitmap = + iommu->iommu.ops->pgsize_bitmap; + domain->domain.ops = + iommu->iommu.ops->default_domain_ops; + } + return &domain->domain; } +static struct iommu_domain *amd_iommu_domain_alloc(unsigned type) +{ + struct iommu_domain *domain; + + domain = do_iommu_domain_alloc(type, NULL, NULL, 0); + if (IS_ERR(domain)) + return NULL; + + return domain; +} + +static struct iommu_domain *amd_iommu_domain_alloc_user(struct device *dev, + u32 flags) +{ + unsigned int type = IOMMU_DOMAIN_UNMANAGED; + struct amd_iommu *iommu; + + iommu = rlookup_amd_iommu(dev); + if (!iommu) + return ERR_PTR(-ENODEV); + + if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) + return ERR_PTR(-EOPNOTSUPP); + + return do_iommu_domain_alloc(type, iommu, dev, flags); +} + static void amd_iommu_domain_free(struct iommu_domain *dom) { struct protection_domain *domain; @@ -2464,6 +2503,7 @@ static bool amd_iommu_enforce_cache_coherency(struct iommu_domain *domain) const struct iommu_ops amd_iommu_ops = { .capable = amd_iommu_capable, .domain_alloc = amd_iommu_domain_alloc, + .domain_alloc_user = amd_iommu_domain_alloc_user, .probe_device = amd_iommu_probe_device, .release_device = amd_iommu_release_device, .probe_finalize = amd_iommu_probe_finalize,
Add the domain_alloc_user op implementation. To that end, refactor amd_iommu_domain_alloc() to receive a dev pointer and flags, while renaming it to .. such that it becomes a common function shared with domain_alloc_user() implementation. The sole difference with domain_alloc_user() is that we initialize also other fields that iommu_domain_alloc() does. It lets it return the iommu domain correctly initialized in one function. This is in preparation to add dirty enforcement on AMD implementation of domain_alloc_user. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- drivers/iommu/amd/iommu.c | 46 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-)