Re: [PATCH] PCI: dwc: Modify the check about MSI DMA mask 32-bit

[Robin Murphy <robin.murphy@arm.com>]

On 2022-04-11 13:54, Christoph Hellwig wrote:
> On Mon, Apr 11, 2022 at 06:47:44PM +0900, Wangseok Lee wrote:
>>>> �Therefore,�the�dma_set_mask(32)(in�dw_pcie_host_init())
>>>> �was�modified�to�be�performed�only�when
>>>> �the�dev-dma_mask�is�not�set�larger�than�32�bits.
>>>
>>> So�what�sets�dev->dma_mask�to�a�larger�than�32-bit�value�here?
>>> We�need�to�find�and�fix�that.
>>
>> At the code of of_dma_configure_id() of driver/of/device.c..
>> In the 64bit system, if the dma start addr is used as 0x1'0000'0000
>> and the size is used as 0xf'0000'0000, "u64 end" is 0xf'ffff'ffff.
>> And the dma_mask value is changed from 0xffff'ffff'ffff'ffff to
>> 0xf'ffff'ffffff due to the code below.
> 
> That does look rather wrong to me, as any limitation should only
> be in the bus mask.  Unless I'm missing something (and Robin should
> know the code much better than I do) we should do something like
> the patch below:

Yeah, there's some smelly history here... Originally, of_dma_configure() 
pre-set the masks as an attempt to impose any restriction represented by 
DT "dma-ranges" - the platform it was implemented in aid of happened to 
have a 31-bit DMA range, which may well have coloured some implicit 
assumptions. IIRC, when I first implemented the separate bus_dma_mask to 
properly solve the general constraint problem, I left the other 
mask-setting in place since even though it shouldn't have served any 
purpose any more, I figured it wasn't actively harmful, and by that 
point it had been around long enough that I was a little wary of opening 
a can of worms if anything *had* erroneously started relying on it.

I'm not against making the change now though - I'm about to get to the 
point of turning all the dma_configure stuff inside-out in the course of 
my IOMMU rework anyway, so I fully expect to be breaking things and 
picking up the pieces. Getting this in first so it's easily bisectable 
and leaves me less code to further break seems most sensible :)

If you're happy to write up the patch, please also do the equivalent for 
acpi_arch_dma_setup() too.

This is all orthogonal to why the original patch in this thread is 
wrong, though. If the pcie-designware driver could somehow guarantee 
that all endpoint functions present, or able to appear later, can handle 
MSI addresses of some width >32, then it could set its DMA mask for the 
fake DMA mapping accordingly, but that has nothing at all to do with how 
many address bits might happen to be wired up on the external AXI interface.

Robin.

> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 874f031442dc7..b197861fcde08 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -113,8 +113,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>   {
>   	const struct iommu_ops *iommu;
>   	const struct bus_dma_region *map = NULL;
> -	u64 dma_start = 0;
> -	u64 mask, end, size = 0;
> +	u64 dma_start = 0, size = 0;
>   	bool coherent;
>   	int ret;
>   
> @@ -156,6 +155,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>   			kfree(map);
>   			return -EINVAL;
>   		}
> +
> +		dev->bus_dma_limit = dma_start + size - 1;
> +		dev->dma_range_map = map;
>   	}
>   
>   	/*
> @@ -169,25 +171,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
>   		dev->dma_mask = &dev->coherent_dma_mask;
>   	}
>   
> -	if (!size && dev->coherent_dma_mask)
> -		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> -	else if (!size)
> -		size = 1ULL << 32;
> -
> -	/*
> -	 * Limit coherent and dma mask based on size and default mask
> -	 * set by the driver.
> -	 */
> -	end = dma_start + size - 1;
> -	mask = DMA_BIT_MASK(ilog2(end) + 1);
> -	dev->coherent_dma_mask &= mask;
> -	*dev->dma_mask &= mask;
> -	/* ...but only set bus limit and range map if we found valid dma-ranges earlier */
> -	if (!ret) {
> -		dev->bus_dma_limit = end;
> -		dev->dma_range_map = map;
> -	}
> -
>   	coherent = of_dma_is_coherent(np);
>   	dev_dbg(dev, "device is%sdma coherent\n",
>   		coherent ? " " : " not ");