linux-kernel - Re: [PATCH v3 3/5] iommu/s390: support iova_to

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <25965f63fd72c43fdd3d4a3e2a3f70608f68ffe9.camel@linux.ibm.com>
Date: Tue, 11 Mar 2025 15:19:29 +0100
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Matthew Rosato <mjrosato@...ux.ibm.com>, joro@...tes.org, will@...nel.org,
        robin.murphy@....com, gerald.schaefer@...ux.ibm.com
Cc: hca@...ux.ibm.com, gor@...ux.ibm.com, agordeev@...ux.ibm.com,
        svens@...ux.ibm.com, borntraeger@...ux.ibm.com, clg@...hat.com,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org
Subject: Re: [PATCH v3 3/5] iommu/s390: support iova_to_phys for additional
 table regions

On Fri, 2025-02-28 at 16:44 -0500, Matthew Rosato wrote:
> The origin_type of the dma_table is used to determine how many table
> levels must be traversed for the translation.
> 
> Signed-off-by: Matthew Rosato <mjrosato@...ux.ibm.com>
> ---
>  arch/s390/include/asm/pci_dma.h |  2 ++
>  drivers/iommu/s390-iommu.c      | 52 ++++++++++++++++++++++++++++++++-
>  2 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/pci_dma.h b/arch/s390/include/asm/pci_dma.h
> index 42d7cc4262ca..8d8962e4fd58 100644
> --- a/arch/s390/include/asm/pci_dma.h
> +++ b/arch/s390/include/asm/pci_dma.h
> @@ -55,6 +55,8 @@ enum zpci_ioat_dtype {
>  #define ZPCI_PT_BITS			8
>  #define ZPCI_ST_SHIFT			(ZPCI_PT_BITS + PAGE_SHIFT)
>  #define ZPCI_RT_SHIFT			(ZPCI_ST_SHIFT + ZPCI_TABLE_BITS)
> +#define ZPCI_RS_SHIFT			(ZPCI_RT_SHIFT + ZPCI_TABLE_BITS)
> +#define ZPCI_RF_SHIFT			(ZPCI_RS_SHIFT + ZPCI_TABLE_BITS)
>  
>  #define ZPCI_RTE_FLAG_MASK		0x3fffUL
>  #define ZPCI_RTE_ADDR_MASK		(~ZPCI_RTE_FLAG_MASK)
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index f2cda0ce0fe9..0a6aad11c327 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -36,6 +36,16 @@ struct s390_domain {
>  
>  static struct iommu_domain blocking_domain;
>  
> +static inline unsigned int calc_rfx(dma_addr_t ptr)
> +{
> +	return ((unsigned long)ptr >> ZPCI_RF_SHIFT) & ZPCI_INDEX_MASK;
> +}
> +
> +static inline unsigned int calc_rsx(dma_addr_t ptr)
> +{
> +	return ((unsigned long)ptr >> ZPCI_RS_SHIFT) & ZPCI_INDEX_MASK;
> +}
> +
>  static inline unsigned int calc_rtx(dma_addr_t ptr)
>  {
>  	return ((unsigned long)ptr >> ZPCI_RT_SHIFT) & ZPCI_INDEX_MASK;
> @@ -759,6 +769,43 @@ static int s390_iommu_map_pages(struct iommu_domain *domain,
>  	return rc;
>  }
>  
> +static unsigned long *get_rto_from_iova(struct s390_domain *domain,
> +					dma_addr_t iova)
> +{
> +	unsigned long *rfo, *rso, *rto;
> +	unsigned long rfe, rse;
> +	unsigned int rfx, rsx;
> +
> +	switch (domain->origin_type) {
> +	case ZPCI_TABLE_TYPE_RFX:
> +		rfo = domain->dma_table;
> +		goto itp_rf;
> +	case ZPCI_TABLE_TYPE_RSX:
> +		rso = domain->dma_table;
> +		goto itp_rs;
> +	case ZPCI_TABLE_TYPE_RTX:
> +		return domain->dma_table;
> +	default:
> +		return NULL;
> +	}
> +
> +itp_rf:
> +	rfx = calc_rfx(iova);
> +	rfe = READ_ONCE(rfo[rfx]);
> +	if (!reg_entry_isvalid(rfe))
> +		return NULL;
> +	rso = get_rf_rso(rfe);
> +
> +itp_rs:
> +	rsx = calc_rsx(iova);
> +	rse = READ_ONCE(rso[rsx]);
> +	if (!reg_entry_isvalid(rse))
> +		return NULL;
> +	rto = get_rs_rto(rse);
> +
> +	return rto;
> +}

I played around with re-organizing the above as the goto out of the
switch feels a bit cumbersome. One variant I came up with is a separate
get_rso_from_iova() function like below:

static unsigned long *get_rso_from_iova(struct s390_domain *domain,
					dma_addr_t iova)
{
	unsigned long *rfo;
	unsigned long rfe;
	unsigned int rfx;

	switch (domain->origin_type) {
	case ZPCI_TABLE_TYPE_RFX:
		rfo = domain->dma_table;
		rfx = calc_rfx(iova);
		rfe = READ_ONCE(rfo[rfx]);
		if (!reg_entry_isvalid(rfe))
			return NULL;
		return get_rf_rso(rfe);
	case ZPCI_TABLE_TYPE_RSX:
		return domain->dma_table;
	default:
		return NULL;
	}
}

static unsigned long *get_rto_from_iova(struct s390_domain *domain,
					dma_addr_t iova)
{
	unsigned long *rso;
	unsigned long rse;
	unsigned int rsx;

	switch (domain->origin_type) {
	case ZPCI_TABLE_TYPE_RFX:
	case ZPCI_TABLE_TYPE_RSX:
		rso = get_rso_from_iova(domain, iova);
		rsx = calc_rsx(iova);
		rse = READ_ONCE(rso[rsx]);
		if (!reg_entry_isvalid(rse))
			return NULL;
		return get_rs_rto(rse);
	case ZPCI_TABLE_TYPE_RTX:
		return domain->dma_table;
	default:
		return NULL;
	}
}

I think this is slightly cleaner but not by enough that I'd say we have
to do it this way and I leave the choice to you.

> +
>  static phys_addr_t s390_iommu_iova_to_phys(struct iommu_domain *domain,
>  					   dma_addr_t iova)
>  {
> @@ -772,10 +819,13 @@ static phys_addr_t s390_iommu_iova_to_phys(struct iommu_domain *domain,
>  	    iova > domain->geometry.aperture_end)
>  		return 0;
>  
> +	rto = get_rto_from_iova(s390_domain, iova);
> +	if (!rto)
> +		return 0;
> +
>  	rtx = calc_rtx(iova);
>  	sx = calc_sx(iova);
>  	px = calc_px(iova);
> -	rto = s390_domain->dma_table;
>  
>  	rte = READ_ONCE(rto[rtx]);
>  	if (reg_entry_isvalid(rte)) {

So with or without my suggestion.

Reviewed-by: Niklas Schnelle <schnelle@...ux.ibm.com>