[PATCH v4 2/4] s390/pci: store DMA offset in bus_dma_region

Matthew Rosato posted 4 patches 1 year ago
There is a newer version of this series
[PATCH v4 2/4] s390/pci: store DMA offset in bus_dma_region
Posted by Matthew Rosato 1 year ago
PCI devices on s390 have a DMA offset that is reported via CLP.  In
preparation for allowing identity domains, setup the bus_dma_region
for all PCI devices using the reported CLP value.

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/pci/pci_bus.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
index 857afbc4828f..b5c35b8e47a4 100644
--- a/arch/s390/pci/pci_bus.c
+++ b/arch/s390/pci/pci_bus.c
@@ -19,6 +19,7 @@
 #include <linux/jump_label.h>
 #include <linux/pci.h>
 #include <linux/printk.h>
+#include <linux/dma-direct.h>
 
 #include <asm/pci_clp.h>
 #include <asm/pci_dma.h>
@@ -283,10 +284,27 @@ static struct zpci_bus *zpci_bus_alloc(int topo, bool topo_is_tid)
 	return zbus;
 }
 
+static void pci_dma_range_setup(struct pci_dev *pdev)
+{
+	struct zpci_dev *zdev = to_zpci(pdev);
+	struct bus_dma_region *map;
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return;
+
+	map->cpu_start = 0;
+	map->dma_start = PAGE_ALIGN(zdev->start_dma);
+	map->size = max(PAGE_ALIGN_DOWN(zdev->end_dma + 1) - map->dma_start, 0);
+	pdev->dev.dma_range_map = map;
+}
+
 void pcibios_bus_add_device(struct pci_dev *pdev)
 {
 	struct zpci_dev *zdev = to_zpci(pdev);
 
+	pci_dma_range_setup(pdev);
+
 	/*
 	 * With pdev->no_vf_scan the common PCI probing code does not
 	 * perform PF/VF linking.
-- 
2.48.1
Re: [PATCH v4 2/4] s390/pci: store DMA offset in bus_dma_region
Posted by Niklas Schnelle 1 year ago
On Fri, 2025-02-07 at 15:53 -0500, Matthew Rosato wrote:
> PCI devices on s390 have a DMA offset that is reported via CLP.  In
> preparation for allowing identity domains, setup the bus_dma_region
> for all PCI devices using the reported CLP value.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>  arch/s390/pci/pci_bus.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/s390/pci/pci_bus.c b/arch/s390/pci/pci_bus.c
> index 857afbc4828f..b5c35b8e47a4 100644
> --- a/arch/s390/pci/pci_bus.c
> +++ b/arch/s390/pci/pci_bus.c
> @@ -19,6 +19,7 @@
>  #include <linux/jump_label.h>
>  #include <linux/pci.h>
>  #include <linux/printk.h>
> +#include <linux/dma-direct.h>
>  
>  #include <asm/pci_clp.h>
>  #include <asm/pci_dma.h>
> @@ -283,10 +284,27 @@ static struct zpci_bus *zpci_bus_alloc(int topo, bool topo_is_tid)
>  	return zbus;
>  }
>  
> +static void pci_dma_range_setup(struct pci_dev *pdev)
> +{
> +	struct zpci_dev *zdev = to_zpci(pdev);
> +	struct bus_dma_region *map;
> +
> +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> +	if (!map)
> +		return;
> +
> +	map->cpu_start = 0;
> +	map->dma_start = PAGE_ALIGN(zdev->start_dma);
> +	map->size = max(PAGE_ALIGN_DOWN(zdev->end_dma + 1) - map->dma_start, 0);

Ugh, this is my fault as I suggested it, but this max() doesn't work
here. The zdev->end_dma is unsigned and so is map->dma_start so if the
former is smaller underflow will occur and the max() won't save us.
It's largely a theoretical issue since zdev->end_dma + 1 should always
be larger than zdev->start_dma, but now the max() looks like we thought
of that, but then it doesn't work.

If we handle it maybe just go with:

aligned_end = PAGE_ALIGN_DOWN(zdev->end_dma + 1);
if (aligned_end >= map->dma_start)
     map->size = aligned_end - map->dma_start;
else
     map->size = 0;


> +	pdev->dev.dma_range_map = map;
> +}
> +
>  void pcibios_bus_add_device(struct pci_dev *pdev)
>  {
>  	struct zpci_dev *zdev = to_zpci(pdev);
>  
> +	pci_dma_range_setup(pdev);
> +
>  	/*
>  	 * With pdev->no_vf_scan the common PCI probing code does not
>  	 * perform PF/VF linking.
Re: [PATCH v4 2/4] s390/pci: store DMA offset in bus_dma_region
Posted by Matthew Rosato 12 months ago
>> +static void pci_dma_range_setup(struct pci_dev *pdev)
>> +{
>> +	struct zpci_dev *zdev = to_zpci(pdev);
>> +	struct bus_dma_region *map;
>> +
>> +	map = kzalloc(sizeof(*map), GFP_KERNEL);
>> +	if (!map)
>> +		return;
>> +
>> +	map->cpu_start = 0;
>> +	map->dma_start = PAGE_ALIGN(zdev->start_dma);
>> +	map->size = max(PAGE_ALIGN_DOWN(zdev->end_dma + 1) - map->dma_start, 0);
> 
> Ugh, this is my fault as I suggested it, but this max() doesn't work
> here. The zdev->end_dma is unsigned and so is map->dma_start so if the
> former is smaller underflow will occur and the max() won't save us.
> It's largely a theoretical issue since zdev->end_dma + 1 should always
> be larger than zdev->start_dma, but now the max() looks like we thought
> of that, but then it doesn't work.
> 
> If we handle it maybe just go with:
> 
> aligned_end = PAGE_ALIGN_DOWN(zdev->end_dma + 1);
> if (aligned_end >= map->dma_start)
>      map->size = aligned_end - map->dma_start;
> else
>      map->size = 0;
> 


Given that it's not really something that's supposed to happen, would it make sense then to add a 

WARN_ON_ONCE(map->size == 0);

At the end of this?
Re: [PATCH v4 2/4] s390/pci: store DMA offset in bus_dma_region
Posted by Niklas Schnelle 12 months ago
On Wed, 2025-02-12 at 10:23 -0500, Matthew Rosato wrote:
> > > +static void pci_dma_range_setup(struct pci_dev *pdev)
> > > +{
> > > +	struct zpci_dev *zdev = to_zpci(pdev);
> > > +	struct bus_dma_region *map;
> > > +
> > > +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> > > +	if (!map)
> > > +		return;
> > > +
> > > +	map->cpu_start = 0;
> > > +	map->dma_start = PAGE_ALIGN(zdev->start_dma);
> > > +	map->size = max(PAGE_ALIGN_DOWN(zdev->end_dma + 1) - map->dma_start, 0);
> > 
> > Ugh, this is my fault as I suggested it, but this max() doesn't work
> > here. The zdev->end_dma is unsigned and so is map->dma_start so if the
> > former is smaller underflow will occur and the max() won't save us.
> > It's largely a theoretical issue since zdev->end_dma + 1 should always
> > be larger than zdev->start_dma, but now the max() looks like we thought
> > of that, but then it doesn't work.
> > 
> > If we handle it maybe just go with:
> > 
> > aligned_end = PAGE_ALIGN_DOWN(zdev->end_dma + 1);
> > if (aligned_end >= map->dma_start)
> >      map->size = aligned_end - map->dma_start;
> > else
> >      map->size = 0;
> > 
> 
> 
> Given that it's not really something that's supposed to happen, would it make sense then to add a 
> 
> WARN_ON_ONCE(map->size == 0);
> 
> At the end of this?


Yes that makes sense to me