Re: [PATCH v3] PCI: dwc: Strengthen the MSI address allocation logic

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

On 13/02/2024 2:52 am, Ajay Agarwal wrote:
> On Tue, Feb 06, 2024 at 07:53:19PM +0300, Serge Semin wrote:
>> On Tue, Feb 06, 2024 at 10:12:44PM +0530, Ajay Agarwal wrote:
>>> On Mon, Feb 05, 2024 at 12:52:45AM +0300, Serge Semin wrote:
>>>> On Sun, Feb 04, 2024 at 04:54:25PM +0530, Ajay Agarwal wrote:
>>>>> There can be platforms that do not use/have 32-bit DMA addresses
>>>>> but want to enumerate endpoints which support only 32-bit MSI
>>>>> address. The current implementation of 32-bit IOVA allocation can
>>>>> fail for such platforms, eventually leading to the probe failure.
>>>>>
>>>>> If there vendor driver has already setup the MSI address using
>>>>> some mechanism, use the same. This method can be used by the
>>>>> platforms described above to support EPs they wish to.
>>>>>
>>>>> Else, if the memory region is not reserved, try to allocate a
>>>>> 32-bit IOVA. Additionally, if this allocation also fails, attempt
>>>>> a 64-bit allocation for probe to be successful. If the 64-bit MSI
>>>>> address is allocated, then the EPs supporting 32-bit MSI address
>>>>> will not work.
>>>>>
>>>>> Signed-off-by: Ajay Agarwal <ajayagarwal@google.com>
>>>>> ---
>>>>> Changelog since v2:
>>>>>   - If the vendor driver has setup the msi_data, use the same
>>>>>
>>>>> Changelog since v1:
>>>>>   - Use reserved memory, if it exists, to setup the MSI data
>>>>>   - Fallback to 64-bit IOVA allocation if 32-bit allocation fails
>>>>>
>>>>>   .../pci/controller/dwc/pcie-designware-host.c | 26 ++++++++++++++-----
>>>>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> index d5fc31f8345f..512eb2d6591f 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> @@ -374,10 +374,18 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>>>>   	 * order not to miss MSI TLPs from those devices the MSI target
>>>>>   	 * address has to be within the lowest 4GB.
>>>>>   	 *
>>>>
>>>>> -	 * Note until there is a better alternative found the reservation is
>>>>> -	 * done by allocating from the artificially limited DMA-coherent
>>>>> -	 * memory.
>>>>
>>>> Why do you keep deleting this statement? The driver still uses the
>>>> DMA-coherent memory as a workaround. Your solution doesn't solve the
>>>> problem completely. This is another workaround. One more time: the
>>>> correct solution would be to allocate a 32-bit address or some range
>>>> within the 4GB PCIe bus memory with no _RAM_ or some other IO behind.
>>>> Your solution relies on the platform firmware/glue-driver doing that,
>>>> which isn't universally applicable. So please don't drop the comment.
>>>>
>>> ACK.
>>>
>>>>> +	 * Check if the vendor driver has setup the MSI address already. If yes,
>>>>> +	 * pick up the same.
>>>>
>>>> This is inferred from the code below. So drop it.
>>>>
>>> ACK.
>>>
>>>>> This will be helpful for platforms that do not
>>>>> +	 * use/have 32-bit DMA addresses but want to use endpoints which support
>>>>> +	 * only 32-bit MSI address.
>>>>
>>>> Please merge it into the first part of the comment as like: "Permit
>>>> the platforms to override the MSI target address if they have a free
>>>> PCIe-bus memory specifically reserved for that."
>>>>
>>> ACK.
>>>
>>>>> +	 * Else, if the memory region is not reserved, try to allocate a 32-bit
>>>>> +	 * IOVA. Additionally, if this allocation also fails, attempt a 64-bit
>>>>> +	 * allocation. If the 64-bit MSI address is allocated, then the EPs
>>>>> +	 * supporting 32-bit MSI address will not work.
>>>>
>>>> This is easily inferred from the code below. So drop it.
>>>>
>>> ACK.
>>>
>>>>>   	 */
>>>>
>>>>> +	if (pp->msi_data)
>>>>
>>>> Note this is a physical address for which even zero value might be
>>>> valid. In this case it's the address of the PCIe bus space for which
>>>> AFAICS zero isn't reserved for something special.
>>>>
>>
>>> That is a fair point. What do you suggest we do? Shall we define another
>>> op `set_msi_data` (like init/msi_init/start_link) and if it is defined
>>> by the vendor, then call it? Then vendor has to set the pp->msi_data
>>> there? Let me know.
>>
>> You can define a new capability flag here
>> drivers/pci/controller/dwc/pcie-designware.h (see DW_PCIE_CAP_* macros)
>> , set it in the glue driver by means of the dw_pcie_cap_set() macro
>> function and instead of checking msi_data value test the flag for
>> being set by dw_pcie_cap_is().
>>
> Sure, good suggestion. ACK.
> 
>>>
>>>>> +		return 0;
>>>>> +
>>>>>   	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>>>>   	if (ret)
>>>>>   		dev_warn(dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
>>>>> @@ -385,9 +393,15 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
>>>>>   	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>>>>>   					GFP_KERNEL);
>>>>>   	if (!msi_vaddr) {
>>>>> -		dev_err(dev, "Failed to alloc and map MSI data\n");
>>>>> -		dw_pcie_free_msi(pp);
>>>>> -		return -ENOMEM;
>>>>> +		dev_warn(dev, "Failed to alloc 32-bit MSI data. Attempting 64-bit now\n");
>>>>> +		dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>>>>> +		msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
>>>>> +						GFP_KERNEL);
>>>>> +		if (!msi_vaddr) {
>>>>> +			dev_err(dev, "Failed to alloc and map MSI data\n");
>>>>> +			dw_pcie_free_msi(pp);
>>>>> +			return -ENOMEM;
>>>>> +		}
>>>>
>>>> On Tue, Jan 30, 2024 at 08:40:48PM +0000, Robin Murphy wrote:
>>>>> Yeah, something like that. Personally I'd still be tempted to try some
>>>>> mildly more involved logic to just have a single dev_warn(), but I think
>>>>> that's less important than just having something which clearly works.
>>>>
>>>> I guess this can be done but in a bit clumsy way. Like this:
>>>>
>>>> 	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) ||
>>>> 	      !dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, GFP_KERNEL);
>>>> 	if (ret) {
>>>> 		dev_warn(dev, "Failed to allocate 32-bit MSI target address\n");
>>>>
>>>> 		dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>>>> 		ret = !dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data, GFP_KERNEL);
>>>> 		if (ret) {
>>>> 			dev_err(dev, "Failed to allocate MSI target address\n");
>>>> 			return -ENOMEM;
>>>
>>> As you pointed out already, this looks pretty clumsy. I think we should
>>> stick to the more descriptive and readable code that I suggested.
>>
>> I do not know which solution is better really. Both have pros and
>> cons. Let's wait for Bjorn, Mani or Robin opinion about this.
>>
>> -Serge(y)
>>
> Bjorn/Mani/Robin,
> Can you please provide your comment?

FWIW I had a go at sketching out what I think I'd do, on top of the v3
patch. Apparently I'm not in a too-clever-for-my-own-good mood today,
since what came out seems to have ended up pretty much just simplifying
the pre-existing code. I'll leave the choice up to you.

Thanks,
Robin.

----->8-----
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 512eb2d6591f..7b68c65e5d11 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -328,7 +328,7 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
  	struct device *dev = pci->dev;
  	struct platform_device *pdev = to_platform_device(dev);
-	u64 *msi_vaddr;
+	u64 *msi_vaddr = NULL;
  	int ret;
  	u32 ctrl, num_ctrls;
  
@@ -387,18 +387,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
  		return 0;
  
  	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
-	if (ret)
-		dev_warn(dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
-
-	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
-					GFP_KERNEL);
+	if (!ret)
+		msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
+						GFP_KERNEL);
  	if (!msi_vaddr) {
-		dev_warn(dev, "Failed to alloc 32-bit MSI data. Attempting 64-bit now\n");
+		dev_warn(dev, "Failed to configure 32-bit MSI address. Devices with only 32-bit MSI support may not work properly\n");
  		dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
  		msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
  						GFP_KERNEL);
  		if (!msi_vaddr) {
-			dev_err(dev, "Failed to alloc and map MSI data\n");
+			dev_err(dev, "Failed to configure MSI address\n");
  			dw_pcie_free_msi(pp);
  			return -ENOMEM;
  		}