Re: [PATCH v3 06/10] iommu/amd: Reduce domain lock scope in attach device path

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/10/18 19:54, Jason Gunthorpe wrote:
> On Fri, Oct 18, 2024 at 10:38:05AM +0800, Baolu Lu wrote:
>> On 2024/10/17 21:32, Jason Gunthorpe wrote:
>>> On Wed, Oct 16, 2024 at 05:34:57AM +0000, Vasant Hegde wrote:
>>>> Currently attach device path takes protection domain lock followed by
>>>> dev_data lock. Most of the operations in this function is specific to
>>>> device data except pdom_attach_iommu() where it updates protection
>>>> domain structure. Hence reduce the scope of protection domain lock.
>>>>
>>>> Note that this changes the locking order. Now it takes device lock
>>>> before taking domain lock (group->mutex -> dev_data->lock ->
>>>> pdom->lock). dev_data->lock is used only in device attachment path.
>>>> So changing order is fine. It will not create any issue.
>>>>
>>>> Finally move numa node assignment to pdom_attach_iommu().
>>> numa node assignment should only be done during domain
>>> allocation. This is important because the domain can be mapped prior
>>> to being attached and without the right nid table levels will be mis
>>> allocated.
>> Sorry for off the topic, just a question about the domain numa ID.
>>
>> When a domain is about to be attached to a device on a different numa
>> node, what should the iommu driver do? Should it:
>>
>> - Ignore the difference (probably log an informational message);
> I would say silently ignore it..
> 
> The only case that probably does this is VFIO, and VFIO is probably
> doing it deliberately to save memory.
> 
>> - Return -EINVAL to force the upper layer to allocate a different domain
>>    for the device?
> This seems extreme..
> 
> I think if qemu or something wants to optimize this we need to return
> numa information in the get_infos to let it make the right decisions
> about domain allocation without being forced into anything.

Yea, thanks! This aligns with what we have in the intel driver.

Thanks,
baolu