On Sat, Oct 21, 2023 at 01:38:04PM -0300, Jason Gunthorpe wrote:
On Fri, Oct 20, 2023 at 11:59:13AM -0700, Nicolin Chen wrote:
On Fri, Oct 20, 2023 at 10:55:01AM -0300, Jason Gunthorpe wrote:
On Fri, Oct 20, 2023 at 02:43:58AM +0000, Tian, Kevin wrote:
What we want to prevent is attaching a non-CC device to a CC domain or upgrade a non-CC domain to CC since in both case the non-CC device will be broken due to incompatible page table format.
[..]
Who cares about such consistency? sure the result is different due to order:
creating hwpt for dev1 (non-CC) then later attaching hwpt to dev2 (CC) will succeed;
creating hwpt for dev2 (CC) then later attaching hwpt to dev1 (non-CC) will fail then the user should create a new hwpt for dev1;
AH... So really what the Intel driver wants is not upgrade to CC but *downgrade* from CC.
non-CC is the type that is universally applicable, so if we come across a non-CC capable device the proper/optimal thing is to degrade the HWPT and re-use it, not allocate a new HWPT.
So the whole thing is upside down.
As changing the IOPTEs in flight seems hard, and I don't want to see the Intel driver get slowed down to accomodate this, I think you are right to say this should be a creation time property only.
I still think userspace should be able to select it so it can minimize the number of HWPTs required.
But the user shouldn't assume such explicit consistency since it's not defined in our uAPI. All we defined is that the attaching may fail due to incompatibility for whatever reason then the user can always try creating a new hwpt for the to-be-attached device. From this regard I don't see providing consistency of result is necessary. 😊
Anyhow, OK, lets add a comment summarizing your points and remove the cc upgrade at attach time (sorry Nicolin/Yi!)
Ack. I will send a small removal series. I assume it should CC stable tree also?
No, it seems more like tidying that fixing a functional issue, do I misunderstand?
Hmm. Maybe the misunderstanding is mine -- Kevin was asking if it was already a bug and you answered yes: https://lore.kernel.org/linux-iommu/20231016115736.GP3952@nvidia.com/
If this shouldn't be a bug fix, I could just merge them into a single tidying patch and add the comments you suggested below.
And where should we add this comment? Kdoc of the alloc uAPI?
Maybe right in front of the only enforce_cc op callback?
OK. How about this? Might be a bit verbose though: /* * enforce_cache_coherenc must be determined during the HWPT allocation. * Note that a HWPT (non-CC) created for a device (non-CC) can be later * reused by another device (either non-CC or CC). However, A HWPT (CC) * created for a device (CC) cannot be reused by another device (non-CC) * but only devices (CC). Instead user space in this case would need to * allocate a separate HWPT (non-CC). */
Thanks Nic