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. 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,
--
Peter Xu
