[PATCH v4 0/5] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

[PATCH v4 0/5] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

[Yongji Xie]

This series introduces a way for PCI resource allocator to force
MMIO BARs not to share PAGE_SIZE. This would make sense to VFIO 
driver. Because current VFIO implementation disallows to mmap 
sub-page(size < PAGE_SIZE) MMIO BARs which may share the same page 
with other BARs for security reasons. Thus, we have to handle mmio 
access to these BARs in QEMU emulation rather than in guest which 
will cause some performance loss.

In our solution, we try to make use of the existing code path of
resource_alignment kernel parameter and add a macro to set default
alignment for it. Thus we can define this macro by default on some
archs which may easily hit the performance issue because of their
64K page.

In this series, patch 1,2,3 fixed bugs of using resource_alignment;
patch 4 tried to add a new option for resource_alignment to use 
IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 5
adds a macro to set the default alignment of all MMIO BARs.

Changelog v4:
- Rebased against v4.8-rc1
- Drop one irrelevant patch
- Drop the patch that adding wildcard to resource_alignment to enforce
  the alignment of all MMIO BARs to be at least PAGE_SIZE
- Change the format of option "noresize" of resource_alignment
- Code style improvements

Changelog v3:
- Ignore enforced alignment to fixed BARs
- Fix issue that disabling memory decoding when reassigning the alignment
- Only enable default alignment on PowerNV platform

Changelog v2:
- Ignore enforced alignment to VF BARs on pci_reassigndev_resource_alignment()

Yongji Xie (5):
  PCI: Ignore enforced alignment when kernel uses existing firmware setup
  PCI: Ignore enforced alignment to VF BARs
  PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()
  PCI: Add a new option for resource_alignment to reassign alignment
  PCI: Add a macro to set default alignment for all PCI devices

 Documentation/kernel-parameters.txt |    9 +++--
 arch/powerpc/include/asm/pci.h      |    4 ++
 drivers/pci/pci.c                   |   71 ++++++++++++++++++++++++++---------
 3 files changed, 64 insertions(+), 20 deletions(-)

-- 
1.7.9.5

[PATCH v4 1/5] PCI: Ignore enforced alignment when kernel uses existing firmware setup

[Yongji Xie]

PCI resources allocator will use firmware setup and not try to
reassign resource when PCI_PROBE_ONLY or IORESOURCE_PCI_FIXED
is set.

The enforced alignment in pci_reassigndev_resource_alignment()
should be ignored in this case. Otherwise, some PCI devices'
resources would be released here and not re-allocated.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/pci.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..2d85a96 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4959,6 +4959,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
 
 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
+	if (pci_has_flag(PCI_PROBE_ONLY)) {
+		if (*p)
+			pr_info_once("PCI: resource_alignment ignored with PCI_PROBE_ONLY\n");
+		spin_unlock(&resource_alignment_lock);
+		return 0;
+	}
+
 	while (*p) {
 		count = 0;
 		if (sscanf(p, "%d%n", &align_order, &count) == 1 &&
@@ -5063,6 +5070,12 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		r = &dev->resource[i];
 		if (!(r->flags & IORESOURCE_MEM))
 			continue;
+		if (r->flags & IORESOURCE_PCI_FIXED) {
+			dev_info(&dev->dev, "No alignment for fixed BAR%d: %pR\n",
+				i, r);
+			continue;
+		}
+
 		size = resource_size(r);
 		if (size < align) {
 			size = align;
-- 
1.7.9.5

[PATCH v4 2/5] PCI: Ignore enforced alignment to VF BARs

[Yongji Xie]

VF BARs are read-only zeroes according to SRIOV spec,
the normal way(writing BARs) of allocating resources wouldn't
be applied to VFs. The VFs' resources would be allocated
when we enable SR-IOV capability. So we should not try to
reassign alignment after we enable VFs. It's meaningless
and will release the allocated resources which leads to a bug.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/pci.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 2d85a96..b8357d7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5048,6 +5048,15 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	resource_size_t align, size;
 	u16 command;
 
+	/*
+	 * VF BARs are RO zero according to SR-IOV spec 3.4.1.11. Their
+	 * resources would be allocated when we enable them and not be
+	 * re-allocated any more. So we should never try to reassign
+	 * VF's alignment here.
+	 */
+	if (dev->is_virtfn)
+		return;
+
 	/* check if specified PCI is target device to reassign */
 	align = pci_specified_resource_alignment(dev);
 	if (!align)
-- 
1.7.9.5

[PATCH v4 3/5] PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()

[Yongji Xie]

We should not disable memory decoding when we reassign alignment
in pci_reassigndev_resource_alignment(). It's meaningless and
have some side effects. For example, we found it would break
this kind of P2P bridge:

0001:02:02.0 PCI bridge: PLX Technology, Inc. PEX 8718 16-Lane,
5-Port PCI Express Gen 3 (8.0 GT/s) Switch (rev aa)

And it may also potentially break the PCI devices with mmio_always_on
bit set.

Besides, disabling memory decoding is not expected in some fixup
function such as fixup_vga(). The fixup_vga() read PCI_COMMAND_MEMORY
to know whether the devices has been initialized by the firmware or
not. Disabling memory decoding would cause the one initialized by
firmware may not be set as the default VGA device when more than one
graphics adapter is present.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/pci.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b8357d7..caa0894 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5046,7 +5046,6 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	int i;
 	struct resource *r;
 	resource_size_t align, size;
-	u16 command;
 
 	/*
 	 * VF BARs are RO zero according to SR-IOV spec 3.4.1.11. Their
@@ -5069,12 +5068,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		return;
 	}
 
-	dev_info(&dev->dev,
-		"Disabling memory decoding and releasing memory resources.\n");
-	pci_read_config_word(dev, PCI_COMMAND, &command);
-	command &= ~PCI_COMMAND_MEMORY;
-	pci_write_config_word(dev, PCI_COMMAND, command);
-
+	dev_info(&dev->dev, "Releasing memory resources.\n");
 	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
 		r = &dev->resource[i];
 		if (!(r->flags & IORESOURCE_MEM))
-- 
1.7.9.5

[PATCH v4 4/5] PCI: Add a new option for resource_alignment to reassign alignment

[Yongji Xie]

When using resource_alignment kernel parameter, the current
implement reassigns the alignment by changing resources' size
which can potentially break some drivers. For example, the driver
uses the size to locate some register whose length is related
to the size.

This patch adds a new option "noresize" for the parameter to
solve this problem.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 Documentation/kernel-parameters.txt |    9 ++++++---
 drivers/pci/pci.c                   |   37 +++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 46c030a..c64e439 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3023,15 +3023,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				window. The default value is 64 megabytes.
 		resource_alignment=
 				Format:
-				[<order of align>@][<domain>:]<bus>:<slot>.<func>[; ...]
-				[<order of align>@]pci:<vendor>:<device>\
-						[:<subvendor>:<subdevice>][; ...]
+				[<order of align>@][noresize@][<domain>:]
+				<bus>:<slot>.<func>[; ...]
+				[<order of align>@][noresize@]pci:<vendor>:<device>
+				[:<subvendor>:<subdevice>][; ...]
 				Specifies alignment and device to reassign
 				aligned memory resources.
 				If <order of align> is not specified,
 				PAGE_SIZE is used as alignment.
 				PCI-PCI bridge can be specified, if resource
 				windows need to be expanded.
+				noresize: Don't change the resources' sizes when
+				reassigning alignment.
 		ecrc=		Enable/disable PCIe ECRC (transaction layer
 				end-to-end CRC checking).
 				bios: Use BIOS/firmware settings. This is the
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index caa0894..d895be7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4946,11 +4946,13 @@ static DEFINE_SPINLOCK(resource_alignment_lock);
 /**
  * pci_specified_resource_alignment - get resource alignment specified by user.
  * @dev: the PCI device to get
+ * @resize: whether or not to change resources' size when reassigning alignment
  *
  * RETURNS: Resource alignment if it is specified.
  *          Zero if it is not specified.
  */
-static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
+static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
+		bool *resize)
 {
 	int seg, bus, slot, func, align_order, count;
 	unsigned short vendor, device, subsystem_vendor, subsystem_device;
@@ -4974,6 +4976,13 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
 		} else {
 			align_order = -1;
 		}
+
+		if (!strncmp(p, "noresize@", 9)) {
+			*resize = false;
+			p += 9;
+		} else
+			*resize = true;
+
 		if (strncmp(p, "pci:", 4) == 0) {
 			/* PCI vendor/device (subvendor/subdevice) ids are specified */
 			p += 4;
@@ -5045,6 +5054,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 {
 	int i;
 	struct resource *r;
+	bool resize = true;
 	resource_size_t align, size;
 
 	/*
@@ -5057,7 +5067,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		return;
 
 	/* check if specified PCI is target device to reassign */
-	align = pci_specified_resource_alignment(dev);
+	align = pci_specified_resource_alignment(dev, &resize);
 	if (!align)
 		return;
 
@@ -5080,15 +5090,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 		}
 
 		size = resource_size(r);
-		if (size < align) {
-			size = align;
-			dev_info(&dev->dev,
-				"Rounding up size of resource #%d to %#llx.\n",
-				i, (unsigned long long)size);
+		if (resize) {
+			if (size < align) {
+				size = align;
+				dev_info(&dev->dev,
+					"Rounding up size of resource #%d to %#llx.\n",
+					i, (unsigned long long)size);
+			}
+			r->flags |= IORESOURCE_UNSET;
+			r->end = size - 1;
+			r->start = 0;
+		} else {
+			r->flags &= ~IORESOURCE_SIZEALIGN;
+			r->flags |= IORESOURCE_STARTALIGN | IORESOURCE_UNSET;
+			r->start = max(align, size);
+			r->end = r->start + size - 1;
 		}
-		r->flags |= IORESOURCE_UNSET;
-		r->end = size - 1;
-		r->start = 0;
 	}
 	/* Need to disable bridge's resource window,
 	 * to enable the kernel to reassign new resource
-- 
1.7.9.5

[PATCH v4 5/5] PCI: Add a macro to set default alignment for all PCI devices

[Yongji Xie]

When vfio passthroughs a PCI device of which MMIO BARs are
smaller than PAGE_SIZE, guest will not handle the mmio
accesses to the BARs which leads to mmio emulations in host.

This is because vfio will not allow to passthrough one BAR's
mmio page which may be shared with other BARs. Otherwise,
there will be a backdoor that guest can use to access BARs
of other guest.

This patch adds a macro to set default alignment for all
PCI devices. Then we could solve this issue on some platforms
which would easily hit this issue because of their 64K page
such as PowerNV platform by defining this macro as PAGE_SIZE.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/pci.h |    4 ++++
 drivers/pci/pci.c              |    4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index e9bd6cf..5e31bc2 100644
--- a/arch/powerpc/include/asm/pci.h
+++ b/arch/powerpc/include/asm/pci.h
@@ -28,6 +28,10 @@
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		0x10000000
 
+#ifdef CONFIG_PPC_POWERNV
+#define PCIBIOS_DEFAULT_ALIGNMENT	PAGE_SIZE
+#endif
+
 struct pci_dev;
 
 /* Values for the `which' argument to sys_pciconfig_iobase syscall.  */
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d895be7..feae59e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4959,6 +4959,10 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 	resource_size_t align = 0;
 	char *p;
 
+#ifdef PCIBIOS_DEFAULT_ALIGNMENT
+	align = PCIBIOS_DEFAULT_ALIGNMENT;
+	*resize = false;
+#endif
 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
 	if (pci_has_flag(PCI_PROBE_ONLY)) {
-- 
1.7.9.5

Re: [PATCH v4 0/5] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

[Yongji Xie]

Ping?

On 2016/8/12 13:42, Yongji Xie wrote:
> This series introduces a way for PCI resource allocator to force
> MMIO BARs not to share PAGE_SIZE. This would make sense to VFIO
> driver. Because current VFIO implementation disallows to mmap
> sub-page(size < PAGE_SIZE) MMIO BARs which may share the same page
> with other BARs for security reasons. Thus, we have to handle mmio
> access to these BARs in QEMU emulation rather than in guest which
> will cause some performance loss.
>
> In our solution, we try to make use of the existing code path of
> resource_alignment kernel parameter and add a macro to set default
> alignment for it. Thus we can define this macro by default on some
> archs which may easily hit the performance issue because of their
> 64K page.
>
> In this series, patch 1,2,3 fixed bugs of using resource_alignment;
> patch 4 tried to add a new option for resource_alignment to use
> IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 5
> adds a macro to set the default alignment of all MMIO BARs.
>
> Changelog v4:
> - Rebased against v4.8-rc1
> - Drop one irrelevant patch
> - Drop the patch that adding wildcard to resource_alignment to enforce
>    the alignment of all MMIO BARs to be at least PAGE_SIZE
> - Change the format of option "noresize" of resource_alignment
> - Code style improvements
>
> Changelog v3:
> - Ignore enforced alignment to fixed BARs
> - Fix issue that disabling memory decoding when reassigning the alignment
> - Only enable default alignment on PowerNV platform
>
> Changelog v2:
> - Ignore enforced alignment to VF BARs on pci_reassigndev_resource_alignment()
>
> Yongji Xie (5):
>    PCI: Ignore enforced alignment when kernel uses existing firmware setup
>    PCI: Ignore enforced alignment to VF BARs
>    PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()
>    PCI: Add a new option for resource_alignment to reassign alignment
>    PCI: Add a macro to set default alignment for all PCI devices
>
>   Documentation/kernel-parameters.txt |    9 +++--
>   arch/powerpc/include/asm/pci.h      |    4 ++
>   drivers/pci/pci.c                   |   71 ++++++++++++++++++++++++++---------
>   3 files changed, 64 insertions(+), 20 deletions(-)
>

Re: [PATCH v4 3/5] PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()

[Bjorn Helgaas]

On Fri, Aug 12, 2016 at 01:42:24PM +0800, Yongji Xie wrote:
> We should not disable memory decoding when we reassign alignment
> in pci_reassigndev_resource_alignment(). It's meaningless and
> have some side effects. For example, we found it would break
> this kind of P2P bridge:
> 
> 0001:02:02.0 PCI bridge: PLX Technology, Inc. PEX 8718 16-Lane,
> 5-Port PCI Express Gen 3 (8.0 GT/s) Switch (rev aa)

I doubt that turning memory decode off breaks this bridge.  I can
believe that it could cause a problem, but I doubt it would be
specific to this bridge.

I also don't think it's meaningless.  After your patch, we throw away
our knowledge of what the BAR contains when we set "r->start = 0".
But if you leave memory decoding enabled, the device will still
respond at whatever address the BAR contains.  That seems like a
problem.

> And it may also potentially break the PCI devices with mmio_always_on
> bit set.
> 
> Besides, disabling memory decoding is not expected in some fixup
> function such as fixup_vga(). The fixup_vga() read PCI_COMMAND_MEMORY
> to know whether the devices has been initialized by the firmware or
> not. Disabling memory decoding would cause the one initialized by
> firmware may not be set as the default VGA device when more than one
> graphics adapter is present.
> 
> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> ---
>  drivers/pci/pci.c |    8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b8357d7..caa0894 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5046,7 +5046,6 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>  	int i;
>  	struct resource *r;
>  	resource_size_t align, size;
> -	u16 command;
>  
>  	/*
>  	 * VF BARs are RO zero according to SR-IOV spec 3.4.1.11. Their
> @@ -5069,12 +5068,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>  		return;
>  	}
>  
> -	dev_info(&dev->dev,
> -		"Disabling memory decoding and releasing memory resources.\n");
> -	pci_read_config_word(dev, PCI_COMMAND, &command);
> -	command &= ~PCI_COMMAND_MEMORY;
> -	pci_write_config_word(dev, PCI_COMMAND, command);
> -
> +	dev_info(&dev->dev, "Releasing memory resources.\n");
>  	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
>  		r = &dev->resource[i];
>  		if (!(r->flags & IORESOURCE_MEM))
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] PCI: Add a macro to set default alignment for all PCI devices

[Bjorn Helgaas]

On Fri, Aug 12, 2016 at 01:42:26PM +0800, Yongji Xie wrote:
> When vfio passthroughs a PCI device of which MMIO BARs are
> smaller than PAGE_SIZE, guest will not handle the mmio
> accesses to the BARs which leads to mmio emulations in host.
> 
> This is because vfio will not allow to passthrough one BAR's
> mmio page which may be shared with other BARs. Otherwise,
> there will be a backdoor that guest can use to access BARs
> of other guest.
> 
> This patch adds a macro to set default alignment for all
> PCI devices. Then we could solve this issue on some platforms
> which would easily hit this issue because of their 64K page
> such as PowerNV platform by defining this macro as PAGE_SIZE.

Just to clarify, I think the issue happens on any arch, whenever
device BARs are smaller than PAGE_SIZE.  This is obviously more
*likely* when PAGE_SIZE is large, but could still happen even with
4K pages.

> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/pci.h |    4 ++++
>  drivers/pci/pci.c              |    4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index e9bd6cf..5e31bc2 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -28,6 +28,10 @@
>  #define PCIBIOS_MIN_IO		0x1000
>  #define PCIBIOS_MIN_MEM		0x10000000
>  
> +#ifdef CONFIG_PPC_POWERNV
> +#define PCIBIOS_DEFAULT_ALIGNMENT	PAGE_SIZE
> +#endif
> +
>  struct pci_dev;
>  
>  /* Values for the `which' argument to sys_pciconfig_iobase syscall.  */
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d895be7..feae59e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4959,6 +4959,10 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
>  	resource_size_t align = 0;
>  	char *p;
>  
> +#ifdef PCIBIOS_DEFAULT_ALIGNMENT
> +	align = PCIBIOS_DEFAULT_ALIGNMENT;
> +	*resize = false;
> +#endif
>  	spin_lock(&resource_alignment_lock);
>  	p = resource_alignment_param;
>  	if (pci_has_flag(PCI_PROBE_ONLY)) {
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] PCI: Add a macro to set default alignment for all PCI devices

[Yongji Xie]

On 2016/9/7 0:59, Bjorn Helgaas wrote:

> On Fri, Aug 12, 2016 at 01:42:26PM +0800, Yongji Xie wrote:
>> When vfio passthroughs a PCI device of which MMIO BARs are
>> smaller than PAGE_SIZE, guest will not handle the mmio
>> accesses to the BARs which leads to mmio emulations in host.
>>
>> This is because vfio will not allow to passthrough one BAR's
>> mmio page which may be shared with other BARs. Otherwise,
>> there will be a backdoor that guest can use to access BARs
>> of other guest.
>>
>> This patch adds a macro to set default alignment for all
>> PCI devices. Then we could solve this issue on some platforms
>> which would easily hit this issue because of their 64K page
>> such as PowerNV platform by defining this macro as PAGE_SIZE.
> Just to clarify, I think the issue happens on any arch, whenever
> device BARs are smaller than PAGE_SIZE.  This is obviously more
> *likely* when PAGE_SIZE is large, but could still happen even with
> 4K pages.

Yes, that's true.

Thanks,
Yongji

>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/include/asm/pci.h |    4 ++++
>>   drivers/pci/pci.c              |    4 ++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
>> index e9bd6cf..5e31bc2 100644
>> --- a/arch/powerpc/include/asm/pci.h
>> +++ b/arch/powerpc/include/asm/pci.h
>> @@ -28,6 +28,10 @@
>>   #define PCIBIOS_MIN_IO		0x1000
>>   #define PCIBIOS_MIN_MEM		0x10000000
>>   
>> +#ifdef CONFIG_PPC_POWERNV
>> +#define PCIBIOS_DEFAULT_ALIGNMENT	PAGE_SIZE
>> +#endif
>> +
>>   struct pci_dev;
>>   
>>   /* Values for the `which' argument to sys_pciconfig_iobase syscall.  */
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index d895be7..feae59e 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4959,6 +4959,10 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
>>   	resource_size_t align = 0;
>>   	char *p;
>>   
>> +#ifdef PCIBIOS_DEFAULT_ALIGNMENT
>> +	align = PCIBIOS_DEFAULT_ALIGNMENT;
>> +	*resize = false;
>> +#endif
>>   	spin_lock(&resource_alignment_lock);
>>   	p = resource_alignment_param;
>>   	if (pci_has_flag(PCI_PROBE_ONLY)) {
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/5] PCI: Do not disable memory decoding in pci_reassigndev_resource_alignment()

[Yongji Xie]

On 2016/9/7 0:56, Bjorn Helgaas wrote:

> On Fri, Aug 12, 2016 at 01:42:24PM +0800, Yongji Xie wrote:
>> We should not disable memory decoding when we reassign alignment
>> in pci_reassigndev_resource_alignment(). It's meaningless and
>> have some side effects. For example, we found it would break
>> this kind of P2P bridge:
>>
>> 0001:02:02.0 PCI bridge: PLX Technology, Inc. PEX 8718 16-Lane,
>> 5-Port PCI Express Gen 3 (8.0 GT/s) Switch (rev aa)
> I doubt that turning memory decode off breaks this bridge.  I can
> believe that it could cause a problem, but I doubt it would be
> specific to this bridge.

I found that disabling memory decoding would not break
this kind of bridge.

However, it would cause some problems if we have a
VGA device using "vgaarb" driver behind the bridge.

The driver didn't call something like pci_enable_device()
when it is loaded or initialized. So when the driver issued
memory access to the VGA device, the access cannot be
supported by the bridge because its memory decoding is
still disabled.

Maybe we should drop this patch and try to fix the driver.

Thanks,
Yongji

> I also don't think it's meaningless.  After your patch, we throw away
> our knowledge of what the BAR contains when we set "r->start = 0".
> But if you leave memory decoding enabled, the device will still
> respond at whatever address the BAR contains.  That seems like a
> problem.
>
>> And it may also potentially break the PCI devices with mmio_always_on
>> bit set.
>>
>> Besides, disabling memory decoding is not expected in some fixup
>> function such as fixup_vga(). The fixup_vga() read PCI_COMMAND_MEMORY
>> to know whether the devices has been initialized by the firmware or
>> not. Disabling memory decoding would cause the one initialized by
>> firmware may not be set as the default VGA device when more than one
>> graphics adapter is present.
>>
>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
>> ---
>>   drivers/pci/pci.c |    8 +-------
>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b8357d7..caa0894 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5046,7 +5046,6 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>>   	int i;
>>   	struct resource *r;
>>   	resource_size_t align, size;
>> -	u16 command;
>>   
>>   	/*
>>   	 * VF BARs are RO zero according to SR-IOV spec 3.4.1.11. Their
>> @@ -5069,12 +5068,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
>>   		return;
>>   	}
>>   
>> -	dev_info(&dev->dev,
>> -		"Disabling memory decoding and releasing memory resources.\n");
>> -	pci_read_config_word(dev, PCI_COMMAND, &command);
>> -	command &= ~PCI_COMMAND_MEMORY;
>> -	pci_write_config_word(dev, PCI_COMMAND, command);
>> -
>> +	dev_info(&dev->dev, "Releasing memory resources.\n");
>>   	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
>>   		r = &dev->resource[i];
>>   		if (!(r->flags & IORESOURCE_MEM))
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>