Re: [PATCH v3 7/7] x86: add pci-nommu implementation of map_resource

[Bjorn Helgaas <bhelgaas@google.com>]

On Fri, May 29, 2015 at 12:14:46PM -0500, wdavis@nvidia.com wrote:
> From: Will Davis <wdavis@nvidia.com>
> 
> Lookup the bus address of the resource by finding the parent host bridge,
> which may be different than the parent host bridge of the target device.
> 
> Signed-off-by: Will Davis <wdavis@nvidia.com>
> ---
>  arch/x86/kernel/pci-nommu.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/x86/kernel/pci-nommu.c b/arch/x86/kernel/pci-nommu.c
> index da15918..6384482 100644
> --- a/arch/x86/kernel/pci-nommu.c
> +++ b/arch/x86/kernel/pci-nommu.c
> @@ -38,6 +38,37 @@ static dma_addr_t nommu_map_page(struct device *dev, struct page *page,
>  	return bus;
>  }
>  
> +static dma_addr_t nommu_map_resource(struct device *dev, struct resource *res,
> +				     unsigned long offset, size_t size,
> +				     enum dma_data_direction dir,
> +				     struct dma_attrs *attrs)
> +{
> +	struct pci_bus *bus;
> +	struct pci_host_bridge *bridge;
> +	struct resource_entry *window;
> +	resource_size_t bus_offset = 0;
> +	dma_addr_t dma_address;
> +
> +	/* Find the parent host bridge of the resource, and determine the
> +	 * relative offset.
> +	 */
> +	list_for_each_entry(bus, &pci_root_buses, node) {
> +		bridge = to_pci_host_bridge(bus->bridge);
> +		resource_list_for_each_entry(window, &bridge->windows) {
> +			if (resource_contains(window->res, res))
> +				bus_offset = window->offset;
> +		}
> +	}

I don't think this is safe.  Assume we have the following topology, and
we want to set it up so 0000:00:00.0 can perform peer-to-peer DMA to
0001:00:01.0:

  pci_bus 0000:00: root bus resource [mem 0x80000000-0xffffffff] (bus address [0x80000000-0xffffffff])
  pci 0000:00:00.0: ...
  pci_bus 0001:00: root bus resource [mem 0x180000000-0x1ffffffff] (bus address [0x80000000-0xffffffff])
  pci 0001:00:01.0: reg 0x10: [mem 0x180000000-0x1803fffff 64bit]

I assume the way this works is that the driver for 0000:00:00.0 would call
this function with 0001:00:01.0 and [mem 0x180000000-0x1803fffff 64bit].

We'll figure out that the resource belongs to 0001:00, so we return a
dma_addr of 0x80000000, which is the bus address as seen by 0001:00:01.0.
But if 0000:00:00.0 uses that address, it refers to something in the
0000:00 hierarchy, not the 0001:00 hierarchy.

We talked about pci_bus_address() and pcibios_resource_to_bus() earlier.
What's the subtlety that makes them unusable here?  I'd rather not add more
uses of the pci_root_buses list if we can avoid it.

> +	dma_address = (res->start - bus_offset) + offset;
> +	WARN_ON(size == 0);
> +	if (!check_addr("map_resource", dev, dma_address, size))
> +		return DMA_ERROR_CODE;
> +	flush_write_buffers();
> +	return dma_address;
> +}
> +
> +

You added an extra blank line here (there was already an extra one before
nommu_sync_sg_for_device(), which is probably what you copied).

>  /* Map a set of buffers described by scatterlist in streaming
>   * mode for DMA.  This is the scatter-gather version of the
>   * above pci_map_single interface.  Here the scatter gather list
> @@ -93,6 +124,7 @@ struct dma_map_ops nommu_dma_ops = {
>  	.free			= dma_generic_free_coherent,
>  	.map_sg			= nommu_map_sg,
>  	.map_page		= nommu_map_page,
> +	.map_resource		= nommu_map_resource,
>  	.sync_single_for_device = nommu_sync_single_for_device,
>  	.sync_sg_for_device	= nommu_sync_sg_for_device,
>  	.is_phys		= 1,
> -- 
> 2.4.0
>