Re: [PATCH v12 5/5] iommu: Remove mode argument from iommu_set_dma_strict()

[Robin Murphy]

On 2021-06-14 18:03, John Garry wrote:
> On 14/06/2021 17:25, Robin Murphy wrote:
> > On 2021-06-11 13:20, John Garry wrote:
> > > We only ever now set strict mode enabled in iommu_set_dma_strict(), so
> > > just remove the argument.
> > >
> > > Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
> > > ---
> > >   drivers/iommu/amd/init.c    | 2 +-
> > >   drivers/iommu/intel/iommu.c | 6 +++---
> > >   drivers/iommu/iommu.c       | 5 ++---
> > >   include/linux/iommu.h       | 2 +-
> > >   4 files changed, 7 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> > > index 0e6ae6d68f14..27e9677ec303 100644
> > > --- a/drivers/iommu/amd/init.c
> > > +++ b/drivers/iommu/amd/init.c
> > > @@ -3098,7 +3098,7 @@ static int __init parse_amd_iommu_options(char 
> > > *str)
> > >   {
> > >       for (; *str; ++str) {
> > >           if (strncmp(str, "fullflush", 9) == 0)
> > > -            iommu_set_dma_strict(true);
> > > +            iommu_set_dma_strict();
> > >           if (strncmp(str, "force_enable", 12) == 0)
> > >               amd_iommu_force_enable = true;
> > >           if (strncmp(str, "off", 3) == 0)
> > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > > index 6763e516362c..e77b8b6e7838 100644
> > > --- a/drivers/iommu/intel/iommu.c
> > > +++ b/drivers/iommu/intel/iommu.c
> > > @@ -452,7 +452,7 @@ static int __init intel_iommu_setup(char *str)
> > >               pr_warn("intel_iommu=forcedac deprecated; use 
> > > iommu.forcedac instead\n");
> > >               iommu_dma_forcedac = true;
> > >           } else if (!strncmp(str, "strict", 6)) {
> > > -            iommu_set_dma_strict(true);
> > > +            iommu_set_dma_strict();
> > >           } else if (!strncmp(str, "sp_off", 6)) {
> > >               pr_info("Disable supported super page\n");
> > >               intel_iommu_superpage = 0;
> > > @@ -4392,7 +4392,7 @@ int __init intel_iommu_init(void)
> > >            */
> > >           if (cap_caching_mode(iommu->cap)) {
> > >               pr_warn("IOMMU batching disallowed due to 
> > > virtualization\n");
> > > -            iommu_set_dma_strict(true);
> > > +            iommu_set_dma_strict();
> > >           }
> > >           iommu_device_sysfs_add(&iommu->iommu, NULL,
> > >                          intel_iommu_groups,
> > > @@ -5663,7 +5663,7 @@ static void quirk_calpella_no_shadow_gtt(struct 
> > > pci_dev *dev)
> > >       } else if (dmar_map_gfx) {
> > >           /* we have to ensure the gfx device is idle before we flush */
> > >           pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
> > > -        iommu_set_dma_strict(true);
> > > +        iommu_set_dma_strict();
> > >       }
> > >   }
> > >   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0040, 
> > > quirk_calpella_no_shadow_gtt);
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index ccbd5d4c1a50..146cb71c7441 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -350,10 +350,9 @@ static int __init iommu_dma_setup(char *str)
> > >   }
> > >   early_param("iommu.strict", iommu_dma_setup);
> > > -void iommu_set_dma_strict(bool strict)
> > > +void iommu_set_dma_strict(void)
> > >   {
> > > -    if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
> >
> > We shouldn't need to keep IOMMU_CMD_LINE_STRICT at all now, since it 
> > was only to prevent a driver's "default lazy" setting passed in here 
> > from downgrading an explicitly-set strict mode.
> >
> > With that cleaned up too,
>
> Patch 1/5 mentions whether the invalidation policy comes from the 
> cmdline - similar to the default domain type print - so I was going to 
> keep that.

Oh, silly me, I'd forgotten that already and was just looking at my 
local tree... Let's keep it for consistency with how we report the 
domain type then.

> And then maybe we should also set it from the deprecated x86 
> driver-specific params.

I don't think it's worth exporting more low-level guts to allow that to 
happen - tying in to iommu_set_dma_strict() would be too late, as 
before. I think the separate pr_warn()s which announce the relevant 
parameter is deprecated (but has still taken effect) should be enough.

Cheers,
Robin.