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

On 6/23/21 6:25 AM, Robin Murphy wrote:

On 2021-06-21 15:32, Lu Baolu wrote:

quoted

Hi Robin,

On 2021/6/21 19:59, Robin Murphy wrote:

quoted

On 2021-06-21 11:34, John Garry wrote:

quoted

On 21/06/2021 11:00, Lu Baolu wrote:

quoted

quoted

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.

quoted

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