Re: [PATCH 10/13] PCI: Only treat non-pref mmio64 as pref if all bridges have MEM_64

[Bjorn Helgaas]

On Thu, Apr 20, 2017 at 10:04:57PM -0700, Yinghai Lu wrote:
> If any bridge up to root only have 32bit pref mmio, We don't need to
> treat device non-pref mmio64 as as pref mmio64.

I don't understand the reasoning here.  

Apparently it is only safe to put a non-prefetchable PCIe BAR in a
prefetchable window if all upstream bridges have a 64-bit prefetchable
window?  Why is that?  Why is it important that the prefetchable
window be 64-bit?  I don't see this mentioned in the implementation
note.

Does this mean "PCI: Check pref compatible bit for mem64 resource of
PCIe device" can make unsafe assignments until we apply this patch?
If so, I don't want that sort of bisection hole.

What bad thing happens without this patch?

> We need to move pci_bridge_check_ranges calling early.
> For parent bridges pref mmio BAR may not allocated by BIOS, res flags
> is still 0, we need to have it correct set before we check them for
> child device resources.
> 
> -v2: check all bus resources instead of just res[15].
> 
> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> Tested-by: Khalid Aziz <khalid.aziz@xxxxxxxxxx>
> ---
>  drivers/pci/setup-bus.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 3de66e6..b3fd314 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -735,6 +735,29 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i)
>  	return -EINVAL;
>  }
>  
> +static bool pci_up_path_over_pref_mem64(struct pci_bus *bus)
> +{
> +	if (pci_is_root_bus(bus))
> +		return true;
> +
> +	if (bus->self) {
> +		int i;
> +		bool found = false;
> +		struct resource *res;
> +
> +		pci_bus_for_each_resource(bus, res, i)
> +			if (res->flags & IORESOURCE_MEM_64) {
> +				found = true;
> +				break;
> +			}
> +
> +		if (!found)
> +			return false;
> +	}
> +
> +	return pci_up_path_over_pref_mem64(bus->parent);
> +}
> +
>  int pci_resource_pref_compatible(const struct pci_dev *dev,
>  				 struct resource *res)
>  {
> @@ -743,7 +766,8 @@ int pci_resource_pref_compatible(const struct pci_dev *dev,
>  
>  	if ((res->flags & IORESOURCE_MEM) &&
>  	    (res->flags & IORESOURCE_MEM_64) &&
> -	    dev->on_all_pcie_path)
> +	    dev->on_all_pcie_path &&
> +	    pci_up_path_over_pref_mem64(dev->bus))
>  		return res->flags | IORESOURCE_PREFETCH;
>  
>  	return res->flags;
> @@ -1236,6 +1260,10 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
>  	struct resource *b_res;
>  	int ret;
>  
> +	if (!pci_is_root_bus(bus) &&
> +	    (bus->self->class >> 8) == PCI_CLASS_BRIDGE_PCI)
> +		pci_bridge_check_ranges(bus);
> +
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		struct pci_bus *b = dev->subordinate;
>  		if (!b)
> @@ -1263,7 +1291,6 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
>  		break;
>  
>  	case PCI_CLASS_BRIDGE_PCI:
> -		pci_bridge_check_ranges(bus);
>  		if (bus->self->is_hotplug_bridge) {
>  			additional_io_size  = pci_hotplug_io_size;
>  			additional_mem_size = pci_hotplug_mem_size;
> -- 
> 2.9.3
>