[RFC PATCH v7 0/3] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

[RFC PATCH v7 0/3] 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 fixed bugs of using resource_alignment;
patch 3 adds a macro to set the default alignment of all MMIO BARs.

Changelog v7:
- Rebased against v4.9-rc2
- Drop two merged patches
- Rework the patch which fix a bug that resources's size is changed when
  using resource_alignment
- Add a patch that fix a bug for IOV BARs when using resource_alignment

Changelog v6: 
- Remove the option "noresize@" of resource_alignment

Changelog v5: 
- Rebased against v4.8-rc6
- Drop the patch that forbidding disable memory decoding in
  pci_reassigndev_resource_alignment()

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 (3):
  PCI: Ignore requested alignment for IOV BARs
  PCI: Restore resource's size if we expand it by using resource_alignment
  PCI: Add a macro to set default alignment for all PCI devices

 arch/powerpc/include/asm/pci.h |    4 ++++
 drivers/pci/pci.c              |    6 +++++-
 drivers/pci/setup-bus.c        |   19 +++++++++++++++++++
 include/linux/pci.h            |    1 +
 4 files changed, 29 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[RFC PATCH v7 1/3] PCI: Ignore requested alignment for IOV BARs

[Yongji Xie]

We would call pci_reassigndev_resource_alignment() before
pci_init_capabilities(). So the requested alignment would
never work for IOV BARs. And it's also meaningless to do
so.

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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ba34907..9a30832 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5118,7 +5118,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	command &= ~PCI_COMMAND_MEMORY;
 	pci_write_config_word(dev, PCI_COMMAND, command);
 
-	for (i = 0; i < PCI_BRIDGE_RESOURCES; i++) {
+	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
 		r = &dev->resource[i];
 		if (!(r->flags & IORESOURCE_MEM))
 			continue;
-- 
1.7.9.5

[RFC PATCH v7 2/3] PCI: Restore resource's size if we expand it by using resource_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 saves the increased sizes in pci_reassigndev_resource_alignment()
and restores to original sizes after PCI resource sizing/assigning are
done.

Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
---
 drivers/pci/pci.c       |    1 +
 drivers/pci/setup-bus.c |   19 +++++++++++++++++++
 include/linux/pci.h     |    1 +
 3 files changed, 21 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9a30832..2933c81 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5130,6 +5130,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 
 		size = resource_size(r);
 		if (size < align) {
+			dev->bars_addsize[i] = align - size;
 			size = align;
 			dev_info(&dev->dev,
 				"Rounding up size of resource #%d to %#llx.\n",
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index f30ca75..12f0e2b 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1387,6 +1387,24 @@ static void pdev_assign_fixed_resources(struct pci_dev *dev)
 	}
 }
 
+static inline void pdev_restore_resource_sizes(struct pci_dev *dev)
+{
+	struct resource *r;
+	int i;
+
+	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
+		r = &dev->resource[i];
+		if (r->flags & IORESOURCE_UNSET)
+			continue;
+
+		if (!dev->bars_addsize[i])
+			continue;
+
+		r->end -= dev->bars_addsize[i];
+		dev->bars_addsize[i] = 0;
+	}
+}
+
 void __pci_bus_assign_resources(const struct pci_bus *bus,
 				struct list_head *realloc_head,
 				struct list_head *fail_head)
@@ -1398,6 +1416,7 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		pdev_assign_fixed_resources(dev);
+		pdev_restore_resource_sizes(dev);
 
 		b = dev->subordinate;
 		if (!b)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0e49f70..a60e8c5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -334,6 +334,7 @@ struct pci_dev {
 	unsigned int	irq;
 	struct cpumask	*irq_affinity;
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
+	resource_size_t bars_addsize[PCI_ROM_RESOURCE + 1];
 
 	bool match_driver;		/* Skip attaching driver */
 	/* These fields are used by common fixups */
-- 
1.7.9.5

[RFC PATCH v7 3/3] 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              |    3 +++
 2 files changed, 7 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 2933c81..c7d9057 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4999,6 +4999,9 @@ 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;
+#endif
 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
 	if (!*p)
-- 
1.7.9.5

Re: [RFC PATCH v7 0/3] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

[Yongji Xie]

Hi Bjorn,


Kindly ping. What do you think of the way to fix the bug that 
resources's size is changed

when using resource_alignment. Thanks.


On 2016/10/26 14:53, 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 fixed bugs of using resource_alignment;
> patch 3 adds a macro to set the default alignment of all MMIO BARs.
>
> Changelog v7:
> - Rebased against v4.9-rc2
> - Drop two merged patches
> - Rework the patch which fix a bug that resources's size is changed when
>    using resource_alignment
> - Add a patch that fix a bug for IOV BARs when using resource_alignment
>
> Changelog v6:
> - Remove the option "noresize@" of resource_alignment
>
> Changelog v5:
> - Rebased against v4.8-rc6
> - Drop the patch that forbidding disable memory decoding in
>    pci_reassigndev_resource_alignment()
>
> 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 (3):
>    PCI: Ignore requested alignment for IOV BARs
>    PCI: Restore resource's size if we expand it by using resource_alignment
>    PCI: Add a macro to set default alignment for all PCI devices
>
>   arch/powerpc/include/asm/pci.h |    4 ++++
>   drivers/pci/pci.c              |    6 +++++-
>   drivers/pci/setup-bus.c        |   19 +++++++++++++++++++
>   include/linux/pci.h            |    1 +
>   4 files changed, 29 insertions(+), 1 deletion(-)
>

Re: [RFC PATCH v7 0/3] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

[Alexey Kardashevskiy]

On 15/11/16 13:53, Yongji Xie wrote:
> Hi Bjorn,
> 
> 
> Kindly ping. What do you think of the way to fix the bug that resources's
> size is changed
> 
> when using resource_alignment. Thanks.


Could anyone please comment on this? Thanks!



> 
> 
> On 2016/10/26 14:53, 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 fixed bugs of using resource_alignment;
>> patch 3 adds a macro to set the default alignment of all MMIO BARs.
>>
>> Changelog v7:
>> - Rebased against v4.9-rc2
>> - Drop two merged patches
>> - Rework the patch which fix a bug that resources's size is changed when
>>    using resource_alignment
>> - Add a patch that fix a bug for IOV BARs when using resource_alignment
>>
>> Changelog v6:
>> - Remove the option "noresize@" of resource_alignment
>>
>> Changelog v5:
>> - Rebased against v4.8-rc6
>> - Drop the patch that forbidding disable memory decoding in
>>    pci_reassigndev_resource_alignment()
>>
>> 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 (3):
>>    PCI: Ignore requested alignment for IOV BARs
>>    PCI: Restore resource's size if we expand it by using resource_alignment
>>    PCI: Add a macro to set default alignment for all PCI devices
>>
>>   arch/powerpc/include/asm/pci.h |    4 ++++
>>   drivers/pci/pci.c              |    6 +++++-
>>   drivers/pci/setup-bus.c        |   19 +++++++++++++++++++
>>   include/linux/pci.h            |    1 +
>>   4 files changed, 29 insertions(+), 1 deletion(-)
>>
> 


-- 
Alexey

Re: [RFC PATCH v7 0/3] PCI: Introduce a way to enforce all MMIO BARs not to share PAGE_SIZE

[Alexey Kardashevskiy]

On 01/12/16 13:32, Alexey Kardashevskiy wrote:
> On 15/11/16 13:53, Yongji Xie wrote:
>> Hi Bjorn,
>>
>>
>> Kindly ping. What do you think of the way to fix the bug that resources's
>> size is changed
>>
>> when using resource_alignment. Thanks.
> 
> 
> Could anyone please comment on this? Thanks!


Anyone, please?


> 
> 
> 
>>
>>
>> On 2016/10/26 14:53, 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 fixed bugs of using resource_alignment;
>>> patch 3 adds a macro to set the default alignment of all MMIO BARs.
>>>
>>> Changelog v7:
>>> - Rebased against v4.9-rc2
>>> - Drop two merged patches
>>> - Rework the patch which fix a bug that resources's size is changed when
>>>    using resource_alignment
>>> - Add a patch that fix a bug for IOV BARs when using resource_alignment
>>>
>>> Changelog v6:
>>> - Remove the option "noresize@" of resource_alignment
>>>
>>> Changelog v5:
>>> - Rebased against v4.8-rc6
>>> - Drop the patch that forbidding disable memory decoding in
>>>    pci_reassigndev_resource_alignment()
>>>
>>> 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 (3):
>>>    PCI: Ignore requested alignment for IOV BARs
>>>    PCI: Restore resource's size if we expand it by using resource_alignment
>>>    PCI: Add a macro to set default alignment for all PCI devices
>>>
>>>   arch/powerpc/include/asm/pci.h |    4 ++++
>>>   drivers/pci/pci.c              |    6 +++++-
>>>   drivers/pci/setup-bus.c        |   19 +++++++++++++++++++
>>>   include/linux/pci.h            |    1 +
>>>   4 files changed, 29 insertions(+), 1 deletion(-)
>>>
>>
> 
> 


-- 
Alexey