linux-kernel - Re: [PATCH v2 1/2] iommu/amd: Add HATDis feature support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <hpvc46sa3t72tj32yz2mrcwzmp6ikg4npbulx5uhv7tj62uqni@ghloowl3sg3z>
Date: Mon, 12 May 2025 05:50:43 +0000
From: Ankit Soni <Ankit.Soni@....com>
To: Joao Martins <joao.m.martins@...cle.com>
CC: <iommu@...ts.linux.dev>, <suravee.suthikulpanit@....com>,
	<vasant.hegde@....com>, <joro@...tes.org>, <will@...nel.org>,
	<robin.murphy@....com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] iommu/amd: Add HATDis feature support

Hi Joao,

On Thu, May 08, 2025 at 06:04:12PM +0100, Joao Martins wrote:
> >  
> >  		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;
> >  
> 
> My suggestion in the past in the past has been to do:
> 
> if (!amd_iommu_hatdis || amd_iommu_pgtable != PD_MODE_NONE) {
> 	ret = iommu_device_sysfs_add(...)
> 	if (ret)
> 		return ret;
> }
> 
> if (!amd_iommu_hatdis || amd_iommu_pgtable != PD_MODE_NONE)
> 	iommu_device_register(&iommu->iommu, &amd_iommu_ops, NULL);
> 
> Also as a to simplify the attach/probe path i.e. remove the chunks in
> amd_iommu_probe_device/init_device_table_dma.
> 
> Or is it that the generation of an ILLEGAL_DEVICE_TABLE_ENTRY event means we
> still need to be careful here?
> 

Correction in my last statement, with above check in iommu_device_register(),
machine is not booting and INVALID_DEVICE_REQUEST is generated continuously.
so iommu_device_register() needs to be called.

> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index f34209b08b4c..4e9a57377b8c 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -2393,6 +2393,13 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
> >  					     pci_max_pasids(to_pci_dev(dev)));
> >  	}
> >  
> > +	if (amd_iommu_pgtable == PD_MODE_NONE) {
> > +		pr_warn_once("%s: DMA translation not supported by iommu.\n",
> > +			     __func__);
> > +		iommu_dev = ERR_PTR(-ENODEV);
> > +		goto out_err;
> > +	}
> > +
> 
> Strange place to put it after all the initialization has been done. Shouldn't it
> be at the top of the function?
> 

In amd_iommu_probe_device() driver is setting ir domain with 
amd_iommu_set_pci_msi_domain(), hence i had chosen this place for return.

> >  out_err:
> >  
> >  	iommu_completion_wait(iommu);
> > @@ -2480,6 +2487,9 @@ static int pdom_setup_pgtable(struct protection_domain *domain,
> >  	case PD_MODE_V2:
> >  		fmt = AMD_IOMMU_V2;
> >  		break;
> > +	case PD_MODE_NONE:
> > +		WARN_ON_ONCE(1);
> > +		return -EPERM;
> >  	}
> >  
> >  	domain->iop.pgtbl.cfg.amd.nid = dev_to_node(dev);> @@ -2501,6 +2511,9 @@
> static inline u64 dma_max_address(enum protection_domain_mode pgtable)
> >  
> >  static bool amd_iommu_hd_support(struct amd_iommu *iommu)
> >  {
> > +	if (amd_iommu_hatdis)
> > +		return false;
> > +
> >  	return iommu && (iommu->features & FEATURE_HDSUP);
> >  }
> >  
> 
> If there's no IOMMU group, how can we allocate a paging domain with dirty
> support for host/v1 pgtable? I don't think you need this part?

This check is for when amd_iommu_hatdis is set and driver has set v2 page table,
then driver driver need to take care about dirty support.