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

[Robin Murphy <robin.murphy@arm.com>]

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@huawei.com>
>>> ---
>>>   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.