Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()

[John Garry <john.garry@huawei.com>]

On 21/06/2021 06:17, Lu Baolu wrote:
> On 2021/6/18 19:34, John Garry wrote:
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 60b1ec42e73b..ff221d3ddcbc 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -349,10 +349,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))
>> -        iommu_dma_strict = strict;
>> +    iommu_dma_strict = true;
>>   }
> 

Hi baolu,

> Sorry for this late comment.
>  > Normally the cache invalidation policy should come from the user. We
> have pre-build kernel option and also a kernel boot command iommu.strict
> to override it. These seem reasonable.
> 
> We also have a helper (iommu_set_dma_strict()) so that the vendor iommu
> driver could squeeze in and change the previous settings mostly due to:
> 
> a) vendor iommu driver specific kernel boot command. (We are about to
>     deprecate those.)
> 
> b) quirky hardware.
> 
> c) kernel optimization (e.x. strict mode in VM environment).
> 
> a) and b) are mandatory, while c) is optional. In any instance should c)
> override the flush mode specified by the user. Hence, probably we should
> also have another helper like:
> 
> void iommu_set_dma_strict_optional()
> {
>      if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>          iommu_dma_strict = true;
> }
> 
> Any thoughts?

What you are suggesting is a change in policy from mainline code. 
Currently for c) we always set strict enabled, regardless of any user 
cmdline input. But now you are saying that you want iommu.strict to 
override in particular scenario, right?

In that case I would think it's better to rework the current API, like 
adding an option to "force" strict mode:

void iommu_set_dma_strict(bool force)
{
      	if (force == true)
		iommu_dma_strict = true;
	else if (!(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
		iommu_dma_strict = true;
}

So we would use iommu_set_dma_strict(true) for a) and b), but 
iommu_set_dma_strict(false) for c).

Then I am not sure what you want to do with the accompanying print for 
c). It was:
"IOMMU batching is disabled due to virtualization"

And now is from this series:
"IOMMU batching disallowed due to virtualization"

Using iommu_get_dma_strict(domain) is not appropriate here to know the 
current mode (so we know whether to print).

Note that this change would mean that the current series would require 
non-trivial rework, which would be unfortunate so late in the cycle.

Thanks,
John
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu