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

[Jason Gunthorpe <jgg@ziepe.ca>]

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.

Jason