Re: [PATCH 1/2] iommu/amd: Add HATDis feature support

[Joao Martins <joao.m.martins@oracle.com>]

On 25/04/2025 17:25, Ankit Soni wrote:
> Hi Joao,
> Thank you for review this patch.
> 
> On Thu, Apr 24, 2025 at 01:19:43PM +0100, Joao Martins wrote:
>>>  {
>>> +	if (amd_iommu_hatdis)
>>> +		return false;
>>> +
>>>  	return iommu && (iommu->features & FEATURE_HDSUP);
>>>  }
>>>  
>>
>> It's strange we seem to somehow have host translation disabled, while it
>> advertises other translation-related features like the normal case.
> 
> If HATDis bit is enabled driver will ignore host(v1) page table related field,
> this includes nested page table.
> 

OK, I thought it was both actually

>>
>> In any case we should probably follow Intel's example (which does similar thing
>> to HATSDis) where we only call invoke IOMMU groups
>> iommu_device_register()/iommu_device_sysfs_add() with DMA translation enabled?
>> That should simplify most of the patch as those codepaths are not reachable via
>> kernel/userspace? Unless I am missing something ofc
>>
>> See also commit c40aaaac10 ("iommu/vt-d: Gracefully handle DMAR units with no
>> supported address widths"). I am not sure what else is the closest example here
>> besides intel-iommu equivalent.
> 
> With intel patch you mentioned above, it seems that it is mostly handling 
> "second stage translation support" disable, which will eventually disable dma 
> translation. And in AMD case, HATDis bit indicates host(v1) translation is not
> available, then attempt to use guest(v2) translation, and if both page
> table modes are not available then disable dma tranlation.

OK, I guess it makes sense if HATDis is v1 only.

My other call out was that when we disable dma-translation all together (aka
both modes), then we shouldn't advertise the IOMMU groups (internally and to
userspace) by not calling iommu_device_register()/iommu_device_sysfs_add().

	Joao