Re: [PATCH v2 2/5] iommu/amd: Introduce rw_semaphore for Device Table Entry (DTE)

[Suthikulpanit, Suravee]

Hi,

On 8/30/2024 2:34 AM, Jason Gunthorpe wrote:
> On Thu, Aug 29, 2024 at 06:07:23PM +0000, Suravee Suthikulpanit wrote:
>
> > -static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
> > +static void clear_dte_entry(struct amd_iommu *iommu, struct iommu_dev_data *dev_data)
> >   {
> > +	u16 devid = dev_data->devid;
> >   	struct dev_table_entry *dev_table = get_dev_table(iommu);
> >   
> > +	down_write(&dev_data->dte_sem);
> > +
> >   	/* remove entry from the device table seen by the hardware */
> >   	dev_table[devid].data[0]  = DTE_FLAG_V;
>
> The rest of this function doesn't seem very good either:
>
> 	/* remove entry from the device table seen by the hardware */
> 	dev_table[devid].data[0]  = DTE_FLAG_V;
>
> 	if (!amd_iommu_snp_en)
> 		dev_table[devid].data[0] |= DTE_FLAG_TV;
>
> 	dev_table[devid].data[1] &= DTE_FLAG_MASK;
>
> This should also cmpxchg the whole 128 bit quantity, you don't want
> the HW to see a tear on HostPageTableRootPtr.
>
> Which is back to my other point. Don't edit the DTE in place.

Thanks for pointing this out. I missed the detail in this function while 
going through the driver to update the code, which updates live DTE. 
I'll change this.

> There is no such thing as 'clear' in the iommu domain API. The DTE is
> either PAGING, BLOCKED or IDENTITY, and any write to it should be
> writing an entire DTE for that target.
>
> I guess clear is actually trying to set the DTE to BLOCKING?

Yes, it's called from blocked_domain->attach_dev() and when doing 
amd_iommu_detach_device().

> Also no need to get the lock here because you don't touch 128 bit
> quanta [1] which holds the IRQ stuff that is racey. This is already
> locked by the iommu core group lock.

Actually, we Need to preserve DTE[96:106] because certain fields are 
programmed using value in IVRS table from early init phase. So, to avoid 
try_cmpxchg128 failure, I need to spin_lock on dte_lock across the 
get_dte256() and update_dte256()

Thanks,
Suravee