linux-kernel - Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Sep 2023 21:14:09 +0800
From:   Michael Shavit <mshavit@...gle.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     iommu@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, will@...nel.org,
        robin.murphy@....com, nicolinc@...dia.com,
        jean-philippe@...aro.org, tina.zhang@...el.com,
        Joerg Roedel <joro@...tes.org>,
        Kevin Tian <kevin.tian@...el.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>,
        Tomas Krcka <krckatom@...zon.de>,
        Yicong Yang <yangyicong@...ilicon.com>
Subject: Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

On Tue, Sep 5, 2023 at 8:42 PM Jason Gunthorpe <jgg@...dia.com> wrote:
>
> On Tue, Sep 05, 2023 at 07:49:14PM +0800, Michael Shavit wrote:
> > Create a new iommu_domain subclass for SVA iommu domains to hold the
> > data previously stored in the dynamically allocated arm_smmu_bond. Add a
> > simple count of attached SVA domains to arm_smmu_master to replace the
> > list of bonds.
> >
> > Signed-off-by: Michael Shavit <mshavit@...gle.com>
> > ---
> >
> >  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 70 +++++++------------
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
> >  3 files changed, 26 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > index 9fb6907c5e7d4..0342c0f35d55a 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > @@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier {
> >
> >  #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
> >
> > -struct arm_smmu_bond {
> > -     struct mm_struct                *mm;
> > +struct arm_smmu_sva_domain {
> > +     struct iommu_domain             iommu_domain;
> >       struct arm_smmu_mmu_notifier    *smmu_mn;
> > -     struct list_head                list;
> >  };
> >
> > -#define sva_to_bond(handle) \
> > -     container_of(handle, struct arm_smmu_bond, sva)
> > +#define to_sva_domain(domain) \
> > +     container_of(domain, struct arm_smmu_sva_domain, iommu_domain)
>
> I'm not sure about this? This seems like a strange direction
>
> The SVA domain and a UNMANAGED/PAGING domain should be basically the
> same thing. Making a sva_domain a completely different type looks like
> it would stand in the way of that?

Agreed that's the eventual destination of all these re-works, but the
stage isn't fully set for that yet. IMO this is a simpler improvement
to get through for now, and I don't see it being an obstacle in the
future.

> > @@ -545,12 +526,11 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
> >
> >  struct iommu_domain *arm_smmu_sva_domain_alloc(void)
> >  {
> > -     struct iommu_domain *domain;
> > +     struct arm_smmu_sva_domain *sva_domain;
> >
> > -     domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> > -     if (!domain)
> > +     sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL);
> > +     if (!sva_domain)
> >               return NULL;
> > -     domain->ops = &arm_smmu_sva_domain_ops;
> > -
> > -     return domain;
> > +     sva_domain->iommu_domain.ops = &arm_smmu_sva_domain_ops;
>
> arm_smmu_sva_domain_free() should container_of before freeing, but
> gross to assume the iommu_domain is the first member.

Oh good catch I missed updating the free.