We only ever now set strict mode enabled in iommu_set_dma_strict(), so
just remove the argument.
Signed-off-by: John Garry <[email protected]>
Reviewed-by: Robin Murphy <[email protected]>
---
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 1e641cb6dddc..6e12a615117b 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3099,7 +3099,7 @@ static int __init parse_amd_iommu_options(char *str)
for (; *str; ++str) {
if (strncmp(str, "fullflush", 9) == 0) {
pr_warn("amd_iommu=fullflush deprecated; use iommu.strict=1 instead\n");
- iommu_set_dma_strict(true);
+ iommu_set_dma_strict();
}
if (strncmp(str, "force_enable", 12) == 0)
amd_iommu_force_enable = true;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 06666f9d8116..77d0834fb0df 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -454,7 +454,7 @@ static int __init intel_iommu_setup(char *str)
iommu_dma_forcedac = true;
} else if (!strncmp(str, "strict", 6)) {
pr_warn("intel_iommu=strict deprecated; use iommu.strict=1 instead\n");
- 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;
@@ -4382,7 +4382,7 @@ int __init intel_iommu_init(void)
*/
if (cap_caching_mode(iommu->cap)) {
pr_info_once("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,
@@ -5699,7 +5699,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 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;
}
bool iommu_get_dma_strict(struct iommu_domain *domain)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..754f67d6dd90 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain *domain);
int iommu_set_pgtable_quirks(struct iommu_domain *domain,
unsigned long quirks);
-void iommu_set_dma_strict(bool val);
+void iommu_set_dma_strict(void);
bool iommu_get_dma_strict(struct iommu_domain *domain);
extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
--
2.26.2
On 2021/6/18 19:34, 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 <[email protected]>
> Reviewed-by: Robin Murphy <[email protected]>
> ---
> 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 1e641cb6dddc..6e12a615117b 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3099,7 +3099,7 @@ static int __init parse_amd_iommu_options(char *str)
> for (; *str; ++str) {
> if (strncmp(str, "fullflush", 9) == 0) {
> pr_warn("amd_iommu=fullflush deprecated; use iommu.strict=1 instead\n");
> - iommu_set_dma_strict(true);
> + iommu_set_dma_strict();
> }
> if (strncmp(str, "force_enable", 12) == 0)
> amd_iommu_force_enable = true;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 06666f9d8116..77d0834fb0df 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -454,7 +454,7 @@ static int __init intel_iommu_setup(char *str)
> iommu_dma_forcedac = true;
> } else if (!strncmp(str, "strict", 6)) {
> pr_warn("intel_iommu=strict deprecated; use iommu.strict=1 instead\n");
> - 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;
> @@ -4382,7 +4382,7 @@ int __init intel_iommu_init(void)
> */
> if (cap_caching_mode(iommu->cap)) {
> pr_info_once("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,
> @@ -5699,7 +5699,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 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;
> }
>
> bool iommu_get_dma_strict(struct iommu_domain *domain)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32d448050bf7..754f67d6dd90 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain *domain);
> int iommu_set_pgtable_quirks(struct iommu_domain *domain,
> unsigned long quirks);
>
> -void iommu_set_dma_strict(bool val);
> +void iommu_set_dma_strict(void);
> bool iommu_get_dma_strict(struct iommu_domain *domain);
>
> extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
>
Reviewed-by: Lu Baolu <[email protected]>
Best regards,
baolu
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;
> }
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?
Best regards,
baolu
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
On 2021/6/21 16:12, John Garry wrote:
> 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,
Hi John,
>
>> 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).
Yes. We need to distinguish the "must" and "nice-to-have" cases of
setting strict mode.
>
> 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.
This patch series looks good to me and I have added by reviewed-by.
Probably we could make another patch series to improve it so that the
kernel optimization should not override the user setting.
Best regards,
baolu
On 21/06/2021 11:00, Lu Baolu wrote:
>> 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).
>
> Yes. We need to distinguish the "must" and "nice-to-have" cases of
> setting strict mode.
>
>>
>> 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.
>
> This patch series looks good to me and I have added by reviewed-by.
> Probably we could make another patch series to improve it so that the
> kernel optimization should not override the user setting.
On a personal level I would be happy with that approach, but I think
it's better to not start changing things right away in a follow-up series.
So how about we add this patch (which replaces 6/6 "iommu: Remove mode
argument from iommu_set_dma_strict()")?
Robin, any opinion?
------->8---------
[PATCH] iommu/vt-d: Make "iommu.strict" override batching due to
virtualization
As a change in policy, make iommu.strict cmdline argument override
whether we disable batching due to virtualization.
The API of iommu_set_dma_strict() is changed to accept a "force"
argument, which means that we always set iommu_dma_strict true,
regardless of whether we already set via cmdline. Also return a boolean,
to tell whether iommu_dma_strict was set or not.
Note that in all pre-existing callsites of iommu_set_dma_strict(),
argument strict was true, so this argument is dropped.
Signed-off-by: John Garry <[email protected]>
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 06666f9d8116..e8d65239b359 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4380,10 +4380,8 @@ int __init intel_iommu_init(void)
* is likely to be much lower than the overhead of synchronizing
* the virtual and physical IOMMU page-tables.
*/
- if (cap_caching_mode(iommu->cap)) {
+ if (cap_caching_mode(iommu->cap) && iommu_set_dma_strict(false))
pr_info_once("IOMMU batching disallowed due to virtualization\n");
- iommu_set_dma_strict(true);
- }
iommu_device_sysfs_add(&iommu->iommu, NULL,
intel_iommu_groups,
"%s", iommu->name);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 60b1ec42e73b..1434bee64af3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -349,10 +349,14 @@ static int __init iommu_dma_setup(char *str)
}
early_param("iommu.strict", iommu_dma_setup);
-void iommu_set_dma_strict(bool strict)
+/* Return true if we set iommu_dma_strict */
+bool iommu_set_dma_strict(bool force)
{
- if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
- iommu_dma_strict = strict;
+ if (force || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) {
+ iommu_dma_strict = true;
+ return true;
+ }
+ return false;
}
bool iommu_get_dma_strict(struct iommu_domain *domain)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..f17b20234296 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain *domain);
int iommu_set_pgtable_quirks(struct iommu_domain *domain,
unsigned long quirks);
-void iommu_set_dma_strict(bool val);
+bool iommu_set_dma_strict(bool force);
bool iommu_get_dma_strict(struct iommu_domain *domain);
extern int report_iommu_fault(struct iommu_domain *domain, struct
device *dev,
--
2.26.2
On 2021-06-21 11:34, John Garry wrote:
> On 21/06/2021 11:00, Lu Baolu wrote:
>>> 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).
>>
>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>> setting strict mode.
>>
>>>
>>> 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.
>>
>> This patch series looks good to me and I have added by reviewed-by.
>> Probably we could make another patch series to improve it so that the
>> kernel optimization should not override the user setting.
>
> On a personal level I would be happy with that approach, but I think
> it's better to not start changing things right away in a follow-up series.
>
> So how about we add this patch (which replaces 6/6 "iommu: Remove mode
> argument from iommu_set_dma_strict()")?
>
> Robin, any opinion?
For me it boils down to whether there are any realistic workloads where
non-strict mode *would* still perform better under virtualisation. The
only reason for the user to explicitly pass "iommu.strict=0" is because
they expect it to increase unmap performance; if it's only ever going to
lead to an unexpected performance loss, I don't see any value in
overriding the kernel's decision purely for the sake of subservience.
If there *are* certain valid cases for allowing it for people who really
know what they're doing, then we should arguably also log a counterpart
message to say "we're honouring your override but beware it may have the
opposite effect to what you expect" for the benefit of other users who
assume it's a generic go-faster knob. At that point it starts getting
non-trivial enough that I'd want to know for sure it's worthwhile.
The other reason this might be better to revisit later is that an AMD
equivalent is still in flight[1], and there might be more that can
eventually be factored out. I think both series are pretty much good to
merge for 5.14, but time's already tight to sort out the conflicts which
exist as-is, without making them any worse.
Robin.
[1]
https://lore.kernel.org/linux-iommu/[email protected]/
>
> ------->8---------
>
> [PATCH] iommu/vt-d: Make "iommu.strict" override batching due to
> virtualization
>
> As a change in policy, make iommu.strict cmdline argument override
> whether we disable batching due to virtualization.
>
> The API of iommu_set_dma_strict() is changed to accept a "force"
> argument, which means that we always set iommu_dma_strict true,
> regardless of whether we already set via cmdline. Also return a boolean,
> to tell whether iommu_dma_strict was set or not.
>
> Note that in all pre-existing callsites of iommu_set_dma_strict(),
> argument strict was true, so this argument is dropped.
>
> Signed-off-by: John Garry <[email protected]>
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 06666f9d8116..e8d65239b359 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4380,10 +4380,8 @@ int __init intel_iommu_init(void)
> * is likely to be much lower than the overhead of synchronizing
> * the virtual and physical IOMMU page-tables.
> */
> - if (cap_caching_mode(iommu->cap)) {
> + if (cap_caching_mode(iommu->cap) && iommu_set_dma_strict(false))
> pr_info_once("IOMMU batching disallowed due to
> virtualization\n");
> - iommu_set_dma_strict(true);
> - }
> iommu_device_sysfs_add(&iommu->iommu, NULL,
> intel_iommu_groups,
> "%s", iommu->name);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 60b1ec42e73b..1434bee64af3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -349,10 +349,14 @@ static int __init iommu_dma_setup(char *str)
> }
> early_param("iommu.strict", iommu_dma_setup);
>
> -void iommu_set_dma_strict(bool strict)
> +/* Return true if we set iommu_dma_strict */
> +bool iommu_set_dma_strict(bool force)
> {
> - if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
> - iommu_dma_strict = strict;
> + if (force || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) {
> + iommu_dma_strict = true;
> + return true;
> + }
> + return false;
> }
>
> bool iommu_get_dma_strict(struct iommu_domain *domain)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 32d448050bf7..f17b20234296 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain *domain);
> int iommu_set_pgtable_quirks(struct iommu_domain *domain,
> unsigned long quirks);
>
> -void iommu_set_dma_strict(bool val);
> +bool iommu_set_dma_strict(bool force);
> bool iommu_get_dma_strict(struct iommu_domain *domain);
>
> extern int report_iommu_fault(struct iommu_domain *domain, struct
> device *dev,
On 21/06/2021 12:59, Robin Murphy wrote:
+ Nadav
>> On a personal level I would be happy with that approach, but I think
>> it's better to not start changing things right away in a follow-up series.
>>
>> So how about we add this patch (which replaces 6/6 "iommu: Remove mode
>> argument from iommu_set_dma_strict()")?
>>
>> Robin, any opinion?
> For me it boils down to whether there are any realistic workloads where
> non-strict mode*would* still perform better under virtualisation. The
> only reason for the user to explicitly pass "iommu.strict=0" is because
> they expect it to increase unmap performance; if it's only ever going to
> lead to an unexpected performance loss, I don't see any value in
> overriding the kernel's decision purely for the sake of subservience.
>
> If there*are* certain valid cases for allowing it for people who really
> know what they're doing, then we should arguably also log a counterpart
> message to say "we're honouring your override but beware it may have the
> opposite effect to what you expect" for the benefit of other users who
> assume it's a generic go-faster knob. At that point it starts getting
> non-trivial enough that I'd want to know for sure it's worthwhile.
>
> The other reason this might be better to revisit later is that an AMD
> equivalent is still in flight[1], and there might be more that can
> eventually be factored out. I think both series are pretty much good to
> merge for 5.14, but time's already tight to sort out the conflicts which
> exist as-is, without making them any worse.
ok, fine. Can revisit.
As for getting these merged, I'll dry-run merging both of those series
to see the conflicts. It doesn't look too problematic from a glance.
Cheers,
John
>
> Robin.
>
> [1]
> https://lore.kernel.org/linux-iommu/[email protected]/
>
>> ------->8---------
>>
>> [PATCH] iommu/vt-d: Make "iommu.strict" override batching due to
>> virtualization
>>
>> As a change in policy, make iommu.strict cmdline argument override
>> whether we disable batching due to virtualization.
>>
>> The API of iommu_set_dma_strict() is changed to accept a "force"
>> argument, which means that we always set iommu_dma_strict true,
>> regardless of whether we already set via cmdline. Also return a boolean,
>> to tell whether iommu_dma_strict was set or not.
>>
>> Note that in all pre-existing callsites of iommu_set_dma_strict(),
>> argument strict was true, so this argument is dropped.
>>
>> Signed-off-by: John Garry<[email protected]>
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 06666f9d8116..e8d65239b359 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4380,10 +4380,8 @@ int __init intel_iommu_init(void)
>> * is likely to be much lower than the overhead of synchronizing
>> * the virtual and physical IOMMU page-tables.
>> */
>> - if (cap_caching_mode(iommu->cap)) {
>> + if (cap_caching_mode(iommu->cap) && iommu_set_dma_strict(false))
>> pr_info_once("IOMMU batching disallowed due to
>> virtualization\n");
>> - iommu_set_dma_strict(true);
>> - }
>> iommu_device_sysfs_add(&iommu->iommu, NULL,
>> intel_iommu_groups,
>> "%s", iommu->name);
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 60b1ec42e73b..1434bee64af3 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -349,10 +349,14 @@ static int __init iommu_dma_setup(char *str)
>> }
>> early_param("iommu.strict", iommu_dma_setup);
>>
>> -void iommu_set_dma_strict(bool strict)
>> +/* Return true if we set iommu_dma_strict */
>> +bool iommu_set_dma_strict(bool force)
>> {
>> - if (strict || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT))
>> - iommu_dma_strict = strict;
>> + if (force || !(iommu_cmd_line & IOMMU_CMD_LINE_STRICT)) {
>> + iommu_dma_strict = true;
>> + return true;
>> + }
>> + return false;
>> }
>>
>> bool iommu_get_dma_strict(struct iommu_domain *domain)
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 32d448050bf7..f17b20234296 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -476,7 +476,7 @@ int iommu_enable_nesting(struct iommu_domain *domain);
>> int iommu_set_pgtable_quirks(struct iommu_domain *domain,
>> unsigned long quirks);
>>
>> -void iommu_set_dma_strict(bool val);
>> +bool iommu_set_dma_strict(bool force);
>> bool iommu_get_dma_strict(struct iommu_domain *domain);
>>
>> extern int report_iommu_fault(struct iommu_domain *domain, struct
>> device *dev,
Hi Robin,
On 2021/6/21 19:59, Robin Murphy wrote:
> On 2021-06-21 11:34, John Garry wrote:
>> On 21/06/2021 11:00, Lu Baolu wrote:
>>>> 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).
>>>
>>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>>> setting strict mode.
>>>
>>>>
>>>> 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.
>>>
>>> This patch series looks good to me and I have added by reviewed-by.
>>> Probably we could make another patch series to improve it so that the
>>> kernel optimization should not override the user setting.
>>
>> On a personal level I would be happy with that approach, but I think
>> it's better to not start changing things right away in a follow-up
>> series.
>>
>> So how about we add this patch (which replaces 6/6 "iommu: Remove mode
>> argument from iommu_set_dma_strict()")?
>>
>> Robin, any opinion?
>
> For me it boils down to whether there are any realistic workloads where
> non-strict mode *would* still perform better under virtualisation. The
At present, we see that strict mode has better performance in the
virtualization environment because it will make the shadow page table
management more efficient. When the hardware supports nested
translation, we may have to re-evaluate this since there's no need for
a shadowing page table anymore.
> only reason for the user to explicitly pass "iommu.strict=0" is because
> they expect it to increase unmap performance; if it's only ever going to
> lead to an unexpected performance loss, I don't see any value in
> overriding the kernel's decision purely for the sake of subservience.
>
> If there *are* certain valid cases for allowing it for people who really
> know what they're doing, then we should arguably also log a counterpart
> message to say "we're honouring your override but beware it may have the
> opposite effect to what you expect" for the benefit of other users who
> assume it's a generic go-faster knob. At that point it starts getting
> non-trivial enough that I'd want to know for sure it's worthwhile.
>
> The other reason this might be better to revisit later is that an AMD
> equivalent is still in flight[1], and there might be more that can
> eventually be factored out. I think both series are pretty much good to
> merge for 5.14, but time's already tight to sort out the conflicts which
> exist as-is, without making them any worse.
Agreed. We could revisit it later.
Best regards,
baolu
On 2021-06-21 15:32, Lu Baolu wrote:
> Hi Robin,
>
> On 2021/6/21 19:59, Robin Murphy wrote:
>> On 2021-06-21 11:34, John Garry wrote:
>>> On 21/06/2021 11:00, Lu Baolu wrote:
>>>>> 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).
>>>>
>>>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>>>> setting strict mode.
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> This patch series looks good to me and I have added by reviewed-by.
>>>> Probably we could make another patch series to improve it so that the
>>>> kernel optimization should not override the user setting.
>>>
>>> On a personal level I would be happy with that approach, but I think
>>> it's better to not start changing things right away in a follow-up
>>> series.
>>>
>>> So how about we add this patch (which replaces 6/6 "iommu: Remove
>>> mode argument from iommu_set_dma_strict()")?
>>>
>>> Robin, any opinion?
>>
>> For me it boils down to whether there are any realistic workloads
>> where non-strict mode *would* still perform better under
>> virtualisation. The
>
> At present, we see that strict mode has better performance in the
> virtualization environment because it will make the shadow page table
> management more efficient. When the hardware supports nested
> translation, we may have to re-evaluate this since there's no need for
> a shadowing page table anymore.
I guess I was assuming that in most cases, proper nested mode could look
distinct enough that we'd be able to treat it differently in the first
place. For instance, if it's handing guest tables directly to the
hardware, would the host have any reason to still set the "caching mode"
ID bit?
Robin.
On 6/23/21 6:25 AM, Robin Murphy wrote:
> On 2021-06-21 15:32, Lu Baolu wrote:
>> Hi Robin,
>>
>> On 2021/6/21 19:59, Robin Murphy wrote:
>>> On 2021-06-21 11:34, John Garry wrote:
>>>> On 21/06/2021 11:00, Lu Baolu wrote:
>>>>>> 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).
>>>>>
>>>>> Yes. We need to distinguish the "must" and "nice-to-have" cases of
>>>>> setting strict mode.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> This patch series looks good to me and I have added by reviewed-by.
>>>>> Probably we could make another patch series to improve it so that the
>>>>> kernel optimization should not override the user setting.
>>>>
>>>> On a personal level I would be happy with that approach, but I think
>>>> it's better to not start changing things right away in a follow-up
>>>> series.
>>>>
>>>> So how about we add this patch (which replaces 6/6 "iommu: Remove
>>>> mode argument from iommu_set_dma_strict()")?
>>>>
>>>> Robin, any opinion?
>>>
>>> For me it boils down to whether there are any realistic workloads
>>> where non-strict mode *would* still perform better under
>>> virtualisation. The
>>
>> At present, we see that strict mode has better performance in the
>> virtualization environment because it will make the shadow page table
>> management more efficient. When the hardware supports nested
>> translation, we may have to re-evaluate this since there's no need for
>> a shadowing page table anymore.
>
> I guess I was assuming that in most cases, proper nested mode could look
> distinct enough that we'd be able to treat it differently in the first
> place. For instance, if it's handing guest tables directly to the
> hardware, would the host have any reason to still set the "caching mode"
> ID bit?
For Intel VT-d, yes, simply for compatible purpose. The guest kernel
may use page tables that are not compatible with the first level
translation. In this case, we must roll back to shadow page table.
>
> Robin.
Best regards,
baolu