RE: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context callback

[Liu, Yi L]

> From: Peter Xu <address@hidden>
> Sent: Tuesday, March 24, 2020 11:24 PM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [PATCH v1 07/22] intel_iommu: add set/unset_iommu_context 
> callback
> 
> On Tue, Mar 24, 2020 at 11:15:24AM +0000, Liu, Yi L wrote:
> 
> [...]
> 
> > > >  struct VTDIOTLBEntry {
> > > > @@ -271,6 +282,8 @@ struct IntelIOMMUState {
> > > >      /*
> > > >       * Protects IOMMU states in general.  Currently it protects the
> > > >       * per-IOMMU IOTLB cache, and context entry cache in 
> > > > VTDAddressSpace.
> > > > +     * Protect the update/usage of HostIOMMUContext pointer cached in
> > > > +     * VTDBus->dev_icx array as array elements may be updated by
> > > > + hotplug
> > >
> > > I think the context update does not need to be updated, because they
> > > should always be with the BQL, right?
> >
> > Hmmmm, maybe I used bad description. My purpose is to protect the
> > stored HostIOMMUContext pointer in vIOMMU. With
> > pci_device_set/unset_iommu_context,
> > vIOMMU have a copy of HostIOMMUContext. If VFIO container is released
> > (e.g. hotpulg out device), HostIOMMUContext will alos be released.
> > This will trigger the pci_device_unset_iommu_context() to clean the
> > copy. To avoid using a staled HostIOMMUContext in vIOMMU, vIOMMU
> > should have a lock to block the pci_device_unset_iommu_context()
> > calling until other threads finished their HostIOMMUContext usage. Do
> > you want a description update here or other preference?
> 
> Yeah, but hot plug/unplug will still take the BQL?
>
> Ah btw I think it's also OK to take the lock if you want or not sure about 
> whether
> we'll always take the BQL in these paths. 

I guess better to have an internal sync to avoid reference stales 
HostIOMMUContext. :-)

> But if so, instead of adding another
> "Protect the ..." sentence to the comment, would you mind list out what the 
> lock is
> protecting?
> 
>   /*
>    * iommu_lock protects:
>    * - per-IOMMU IOTLB caches
>    * - context entry caches
>    * - ...
>    */
> 
> Or anything better than that.  Thanks,

It looks good to me. Let me update it in next version.

Regards,
Yi Liu