https://lore.kernel.org/linux-iommu/[email protected]/
The conversation above concluded that a hwpt should only enforce cache
coherency per device at the stage of its allocation, and it should not
be changed or updated in the attach/replace routines.
Add two patches dropping the enforce_cache_coherency calls from attach
and replce routines respectively, since they were introduced with two
different commits.
Nicolin Chen (2):
iommufd/device: Drop enforce_cache_coherency in
iommufd_device_do_replace
iommufd/device: Drop enforce_cache_coherency in
iommufd_hw_pagetable_attach
drivers/iommu/iommufd/device.c | 19 ++-----------------
drivers/iommu/iommufd/hw_pagetable.c | 2 +-
drivers/iommu/iommufd/iommufd_private.h | 1 -
3 files changed, 3 insertions(+), 19 deletions(-)
--
2.42.0
According to the conversion in the following link:
https://lore.kernel.org/linux-iommu/[email protected]/
The enforce_cache_coherency should be set/enforced in the hwpt allocation
routine. The iommu driver in its attach_dev() op should decide whether to
reject or not a device that doesn't match with the configuration of cache
coherency. Drop the enforce_cache_coherency piece in replace(). Also move
the remaining "num_devices++" piece closer to the refcount that uses this
num_devices.
Cc: [email protected]
Fixes: e88d4ec154a8 ("iommufd: Add iommufd_device_replace()")
Suggested-by: Tian, Kevin <[email protected]>
Signed-off-by: Nicolin Chen <[email protected]>
---
drivers/iommu/iommufd/device.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index e88fa73a45e6..c93f3478f808 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -429,16 +429,6 @@ iommufd_device_do_replace(struct iommufd_device *idev,
return NULL;
}
- /* Try to upgrade the domain we have */
- list_for_each_entry(cur, &igroup->device_list, group_item) {
- num_devices++;
- if (cur->enforce_cache_coherency) {
- rc = iommufd_hw_pagetable_enforce_cc(hwpt);
- if (rc)
- goto err_unlock;
- }
- }
-
old_hwpt = igroup->hwpt;
if (hwpt->ioas != old_hwpt->ioas) {
list_for_each_entry(cur, &igroup->device_list, group_item) {
@@ -465,6 +455,8 @@ iommufd_device_do_replace(struct iommufd_device *idev,
igroup->hwpt = hwpt;
+ list_for_each_entry(cur, &igroup->device_list, group_item)
+ num_devices++;
/*
* Move the refcounts held by the device_list to the new hwpt. Retain a
* refcount for this thread as the caller will free it.
--
2.42.0
On 2023/10/21 8:37, Nicolin Chen wrote:
> According to the conversion in the following link:
> https://lore.kernel.org/linux-iommu/[email protected]/
>
> The enforce_cache_coherency should be set/enforced in the hwpt allocation
> routine. The iommu driver in its attach_dev() op should decide whether to
> reject or not a device that doesn't match with the configuration of cache
> coherency. Drop the enforce_cache_coherency piece in replace(). Also move
> the remaining "num_devices++" piece closer to the refcount that uses this
> num_devices.
>
> Cc: [email protected]
> Fixes: e88d4ec154a8 ("iommufd: Add iommufd_device_replace()")
> Suggested-by: Tian, Kevin <[email protected]>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
> drivers/iommu/iommufd/device.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index e88fa73a45e6..c93f3478f808 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -429,16 +429,6 @@ iommufd_device_do_replace(struct iommufd_device *idev,
> return NULL;
> }
>
> - /* Try to upgrade the domain we have */
> - list_for_each_entry(cur, &igroup->device_list, group_item) {
> - num_devices++;
> - if (cur->enforce_cache_coherency) {
> - rc = iommufd_hw_pagetable_enforce_cc(hwpt);
> - if (rc)
> - goto err_unlock;
> - }
> - }
> -
> old_hwpt = igroup->hwpt;
> if (hwpt->ioas != old_hwpt->ioas) {
> list_for_each_entry(cur, &igroup->device_list, group_item) {
> @@ -465,6 +455,8 @@ iommufd_device_do_replace(struct iommufd_device *idev,
>
> igroup->hwpt = hwpt;
>
> + list_for_each_entry(cur, &igroup->device_list, group_item)
> + num_devices++;
Minor: How about using list_count_nodes()?
> /*
> * Move the refcounts held by the device_list to the new hwpt. Retain a
> * refcount for this thread as the caller will free it.
Either way,
Reviewed-by: Lu Baolu <[email protected]>
Best regards,
baolu
On 2023/10/21 8:37, Nicolin Chen wrote:
> https://lore.kernel.org/linux-iommu/[email protected]/
> The conversation above concluded that a hwpt should only enforce cache
> coherency per device at the stage of its allocation, and it should not
> be changed or updated in the attach/replace routines.
>
> Add two patches dropping the enforce_cache_coherency calls from attach
> and replce routines respectively, since they were introduced with two
> different commits.
>
> Nicolin Chen (2):
> iommufd/device: Drop enforce_cache_coherency in
> iommufd_device_do_replace
> iommufd/device: Drop enforce_cache_coherency in
> iommufd_hw_pagetable_attach
>
> drivers/iommu/iommufd/device.c | 19 ++-----------------
> drivers/iommu/iommufd/hw_pagetable.c | 2 +-
> drivers/iommu/iommufd/iommufd_private.h | 1 -
> 3 files changed, 3 insertions(+), 19 deletions(-)
Hi Kevin and Jason,
With these two fixes, there's no issue in the intel driver any more. Do
I understand it right?
Best regards,
baolu
On Sat, Oct 21, 2023 at 09:32:32AM +0800, Baolu Lu wrote:
> On 2023/10/21 8:37, Nicolin Chen wrote:
> > https://lore.kernel.org/linux-iommu/[email protected]/
> > The conversation above concluded that a hwpt should only enforce cache
> > coherency per device at the stage of its allocation, and it should not
> > be changed or updated in the attach/replace routines.
> >
> > Add two patches dropping the enforce_cache_coherency calls from attach
> > and replce routines respectively, since they were introduced with two
> > different commits.
> >
> > Nicolin Chen (2):
> > iommufd/device: Drop enforce_cache_coherency in
> > iommufd_device_do_replace
> > iommufd/device: Drop enforce_cache_coherency in
> > iommufd_hw_pagetable_attach
> >
> > drivers/iommu/iommufd/device.c | 19 ++-----------------
> > drivers/iommu/iommufd/hw_pagetable.c | 2 +-
> > drivers/iommu/iommufd/iommufd_private.h | 1 -
> > 3 files changed, 3 insertions(+), 19 deletions(-)
>
> Hi Kevin and Jason,
>
> With these two fixes, there's no issue in the intel driver any more. Do
> I understand it right?
I think so, as long as it is an allocation only time flag there isn't
much trouble for the driver.
VFIO, I think, still does the old algorithm however.
Jason
On Sat, Oct 21, 2023 at 09:25:18AM +0800, Baolu Lu wrote:
> > @@ -465,6 +455,8 @@ iommufd_device_do_replace(struct iommufd_device *idev,
> >
> > igroup->hwpt = hwpt;
> >
> > + list_for_each_entry(cur, &igroup->device_list, group_item)
> > + num_devices++;
>
> Minor: How about using list_count_nodes()?
That's better I think. Replaced with:
+ num_devices = list_count_nodes(&igroup->device_list);
> > /*
> > * Move the refcounts held by the device_list to the new hwpt. Retain a
> > * refcount for this thread as the caller will free it.
>
> Either way,
>
> Reviewed-by: Lu Baolu <[email protected]>
Thanks!
Nic
> From: Baolu Lu <[email protected]>
> Sent: Saturday, October 21, 2023 9:33 AM
>
> On 2023/10/21 8:37, Nicolin Chen wrote:
> > https://lore.kernel.org/linux-
> iommu/[email protected]/
> > The conversation above concluded that a hwpt should only enforce cache
> > coherency per device at the stage of its allocation, and it should not
> > be changed or updated in the attach/replace routines.
> >
> > Add two patches dropping the enforce_cache_coherency calls from attach
> > and replce routines respectively, since they were introduced with two
> > different commits.
> >
> > Nicolin Chen (2):
> > iommufd/device: Drop enforce_cache_coherency in
> > iommufd_device_do_replace
> > iommufd/device: Drop enforce_cache_coherency in
> > iommufd_hw_pagetable_attach
> >
> > drivers/iommu/iommufd/device.c | 19 ++-----------------
> > drivers/iommu/iommufd/hw_pagetable.c | 2 +-
> > drivers/iommu/iommufd/iommufd_private.h | 1 -
> > 3 files changed, 3 insertions(+), 19 deletions(-)
>
> Hi Kevin and Jason,
>
> With these two fixes, there's no issue in the intel driver any more. Do
> I understand it right?
>
But code-wise it's still good to explicitly disallow enforce-cc on a
non-empty domain if there is no plan to support it. Just no Fix to
stable.
On 10/23/23 10:55 AM, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Saturday, October 21, 2023 9:33 AM
>>
>> On 2023/10/21 8:37, Nicolin Chen wrote:
>>> https://lore.kernel.org/linux-
>> iommu/[email protected]/
>>> The conversation above concluded that a hwpt should only enforce cache
>>> coherency per device at the stage of its allocation, and it should not
>>> be changed or updated in the attach/replace routines.
>>>
>>> Add two patches dropping the enforce_cache_coherency calls from attach
>>> and replce routines respectively, since they were introduced with two
>>> different commits.
>>>
>>> Nicolin Chen (2):
>>> iommufd/device: Drop enforce_cache_coherency in
>>> iommufd_device_do_replace
>>> iommufd/device: Drop enforce_cache_coherency in
>>> iommufd_hw_pagetable_attach
>>>
>>> drivers/iommu/iommufd/device.c | 19 ++-----------------
>>> drivers/iommu/iommufd/hw_pagetable.c | 2 +-
>>> drivers/iommu/iommufd/iommufd_private.h | 1 -
>>> 3 files changed, 3 insertions(+), 19 deletions(-)
>>
>> Hi Kevin and Jason,
>>
>> With these two fixes, there's no issue in the intel driver any more. Do
>> I understand it right?
>>
>
> But code-wise it's still good to explicitly disallow enforce-cc on a
> non-empty domain if there is no plan to support it. Just no Fix to
> stable.
Yes. Make sense. The device driver implementation should be self-
contained.
Best regards,
baolu