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

[Ankit Soni]

Hi,

On Wed, Apr 30, 2025 at 05:05:02PM +0530, Vasant Hegde wrote:
> Hi,
> 
> On 4/23/2025 12:20 PM, Ankit Soni wrote:
> > --- a/drivers/iommu/amd/init.c
> > +++ b/drivers/iommu/amd/init.c
> > @@ -151,7 +151,7 @@ struct ivmd_header {
> >  bool amd_iommu_dump;
> >  bool amd_iommu_irq_remap __read_mostly;
> >  
> > -enum protection_domain_mode amd_iommu_pgtable = PD_MODE_V1;
> > +enum protection_domain_mode amd_iommu_pgtable = PD_MODE_NONE;
> 
> 
> Why not keep it as `PD_MODE_V1` (as its our default page table) so that we can
> remove explict `PD_MODE_V1` assignment in below hunk?
> 

Yes it will remove `PD_MODE_V1` assignment hunk. But, Intension to keep
PD_MODE_NONE is more clean code, where by default page table mode is NONE,
and with proper checks and default/command line input, driver changes it.

> >  /* Guest page table level */
> >  int amd_iommu_gpt_level = PAGE_MODE_4_LEVEL;
> >  
> > @@ -168,6 +168,9 @@ static int amd_iommu_target_ivhd_type;
> >  u64 amd_iommu_efr;
> >  u64 amd_iommu_efr2;
> >  
> > +/* dma translation not supported*/
> 
> Host (v2) page table is not supported.
> 

I'll rephrase this.

> > +bool amd_iommu_hatdis;
> > +
> >  /* SNP is enabled on the system? */
> >  bool amd_iommu_snp_en;
> >  EXPORT_SYMBOL(amd_iommu_snp_en);
> > @@ -1798,6 +1801,11 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
> >  		if (h->efr_reg & BIT(IOMMU_EFR_XTSUP_SHIFT))
> >  			amd_iommu_xt_mode = IRQ_REMAP_X2APIC_MODE;
> >  
> > +		if (h->efr_attr & BIT(IOMMU_IVHD_ATTR_HATDIS_SHIFT)) {
> > +			pr_warn_once("Host Address Translation is not supported.\n");
> > +			amd_iommu_hatdis = true;
> > +		}
> > +
> >  		early_iommu_features_init(iommu, h);
> >  
> >  		break;
> > @@ -2582,7 +2590,7 @@ static void init_device_table_dma(struct amd_iommu_pci_seg *pci_seg)
> >  	u32 devid;
> >  	struct dev_table_entry *dev_table = pci_seg->dev_table;
> >  
> > -	if (dev_table == NULL)
> > +	if (!dev_table || amd_iommu_pgtable == PD_MODE_NONE)
> >  		return;
> >  
> >  	for (devid = 0; devid <= pci_seg->last_bdf; ++devid) {
> > @@ -3095,6 +3103,17 @@ static int __init early_amd_iommu_init(void)
> >  		}
> >  	}
> >  
> > +	if (amd_iommu_hatdis) {
> 
> I missed it in the internal review. This introduces ordering dependency (v2 page
> table check should be done before this). IMO its worth to add an explicit
> comment so that its clear.
> 
> -Vasant
> 

Sure, i'll move this below v2 page table check.

-Ankit