Current vfio-pci implementation disallows to mmap
sub-page(size < PAGE_SIZE) MMIO BARs and MSI-X table. This is because
sub-page BARs' mmio page may be shared with other BARs and MSI-X table
should not be accessed directly from the guest for security reasons.
But these will easily cause some performance issues for mmio accesses
in guest when vfio passthrough sub-page BARs or BARs containing MSI-X
table on PPC64 platform. This is because PAGE_SIZE is 64KB by default
on PPC64 platform and the big page may easily hit the sub-page MMIO
BARs' unmmapping and cause the unmmaping of the mmio page which
MSI-X table locate in, which lead to mmio emulation in host.
For sub-page MMIO BARs' unmmapping, this patchset adds a kernel
parameter for PCI resource allocator to enforce the alignment of all
MMIO BARs to be at least PAGE_SZIE and make it enabled by default on
PPC64 platform so that sub-page BAR's mmio page will not be shared
with other BARs. Then we can mmap sub-page MMIO BARs in vfio-pci driver
with this parameter enabled.
For MSI-X table's unmmapping, we think MSI-X table is safe to access
directly from userspace if PCI host bridge support filtering of MSIs
which can ensure that a given pci device can only shoot the MSIs
assigned for it. So we add a pci_host_bridge attribute to indicate
if this PCI host bridge supports filtering of MSIs. Then we can mmap
MSI-X table with this attribute set.
With this patchset applied, we can get almost 100% improvement on
performance for mmio accesses when we passthrough sub-page BARs to guest
in our test.
The two vfio related patches(patch 2 and patch 5) are based on the
proposed patchset[1].
Changelog v3:
- Rebase on new linux kernel mainline with the patchset[1] applied.
- Add a function to check whether PCI BARs'mmio page is shared with
other BARs.
- Add a host bridge attribute to indicate PCI host bridge support
filtering of MSIs.
- Use the new host bridge attribute to check if MSI-X table can
be mmapped instead of CONFIG_EEH.
- Remove Kconfig option VFIO_PCI_MMAP_MSIX
Changelog v2:
- Rebase on v4.4-rc6 with the patchset[1] applied.
- Use kernel parameter to enforce all MMIO BARs to be page aligned
on PCI core code instead of doing it on PPC64 arch code.
- Remove flags: VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED
VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP
- Add a Kconfig option to support for mmapping MSI-X table.
[1] https://lkml.org/lkml/2015/11/23/748
Yongji Xie (5):
PCI: Add support for enforcing all MMIO BARs to be page aligned
vfio-pci: Allow to mmap sub-page MMIO BARs if the mmio page is exclusive
PCI: Add host bridge attribute to indicate filtering of MSIs is supported
powerpc/powernv/pci-ioda: Enable msi_filtered bit for any IODA host bridge
vfio-pci: Allow to mmap MSI-X table if host bridge supports filtering of MSIs
Documentation/kernel-parameters.txt | 5 +++++
arch/powerpc/include/asm/pci.h | 11 +++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 6 +++++
drivers/pci/host-bridge.c | 6 +++++
drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++
drivers/pci/pci.h | 8 ++++++-
drivers/vfio/pci/vfio_pci.c | 13 ++++++++---
include/linux/pci.h | 7 ++++++
8 files changed, 87 insertions(+), 4 deletions(-)
--
1.7.9.5
When vfio passthrough 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.
To solve this performance issue, this patch adds a kernel
parameter "pci=resource_page_aligned=on" to enforce
the alignment of all MMIO BARs to be at least PAGE_SIZE,
so that one BAR's mmio page would not be shared with other
BARs. We can also disable it through kernel parameter
"pci=resource_page_aligned=off".
For the default value of the parameter, we think it should be
arch-independent, so we add a macro
HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED to change it. And we
define this macro to enable this parameter by default on PPC64
platform which can easily hit this performance issue because
its PAGE_SIZE is 64KB.
Note that the kernel parameter won't works if kernel doesn't do
resources reallocation.
Signed-off-by: Yongji Xie <redacted>
---
Documentation/kernel-parameters.txt | 5 +++++
arch/powerpc/include/asm/pci.h | 11 +++++++++++
drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 8 +++++++-
include/linux/pci.h | 4 ++++
5 files changed, 62 insertions(+), 1 deletion(-)
@@ -2857,6 +2857,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted. PAGE_SIZE is used as alignment. PCI-PCI bridge can be specified, if resource windows need to be expanded.+ resource_page_aligned= Enable/disable enforcing the alignment+ of all PCI devices' memory resources to be+ at least PAGE_SIZE if resources reallocation+ is done by kernel.+ Format: { "on" | "off" } ecrc= Enable/disable PCIe ECRC (transaction layer end-to-end CRC checking). bios: Use BIOS/firmware settings. This is the
@@ -136,6 +136,17 @@ extern pgprot_t pci_phys_mem_access_prot(struct file *file,unsignedlongpfn,unsignedlongsize,pgprot_tprot);+#ifdef CONFIG_PPC64++/* For PPC64, We enforce all PCI MMIO BARs to be page aligned+*bydefault.Thiswouldbehelpfultoimproveperformance+*whenwepassthroughaPCIdeviceofwhichBARsaresmaller+*thanPAGE_SIZE(64KB).Andwecanusekernelparameter+*"pci=resource_page_aligned=off"todisableit.+*/+#define HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED 1++#endif#define HAVE_ARCH_PCI_RESOURCE_TO_USERexternvoidpci_resource_to_user(conststructpci_dev*dev,intbar,
@@ -99,6 +99,9 @@ u8 pci_cache_line_size;*/unsignedintpcibios_max_latency=255;+boolpci_resources_page_aligned=+IS_ENABLED(HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED);+/* If set, the PCIe ARI capability will not be used. */staticboolpcie_ari_disabled;
@@ -1530,6 +1530,10 @@ static inline int pci_get_new_domain_nr(void) { return -ENOSYS; }(pci_resource_end((dev),(bar))-\pci_resource_start((dev),(bar))+1))+externboolpci_resources_page_aligned;++boolpci_resources_share_page(structpci_dev*dev,intresno);+/* Similar to the helpers above, these manipulate per-pci_dev*driver-specificdata.Theyarereallyjustawrapperaround*thegenericdevicestructurefunctionsofthesecalls.
Current vfio-pci implementation disallows to mmap
sub-page(size < PAGE_SIZE) MMIO BARs because these BARs' mmio
page may be shared with other BARs.
But we should allow to mmap these sub-page MMIO BARs if PCI
resource allocator can make sure these BARs' mmio page will
not be shared with other BARs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/vfio/pci/vfio_pci.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
MSI-X tables are not allowed to be mmapped in vfio-pci
driver in case that user get to touch this directly.
This will cause some performance issues when when PCI
adapters have critical registers in the same page as
the MSI-X table.
However, some kind of PCI host bridge such as IODA bridge
on Power support filtering of MSIs, which can ensure that a
given pci device can only shoot the MSIs assigned for it.
So we think it's safe to expose the MSI-X table to userspace
if filtering of MSIs is supported because the exposed MSI-X
table can't be used to do harm to other memory space.
To support this case, this patch adds a pci_host_bridge
attribute to indicate if this PCI host bridge supports
filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/pci/host-bridge.c | 6 ++++++
include/linux/pci.h | 3 +++
2 files changed, 9 insertions(+)
Current vfio-pci implementation disallows to mmap MSI-X
table in case that user get to touch this directly.
But we should allow to mmap these MSI-X tables if the PCI
host bridge supports filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/vfio/pci/vfio_pci.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Sent: 15 January 2016 07:06
MSI-X tables are not allowed to be mmapped in vfio-pci
driver in case that user get to touch this directly.
This will cause some performance issues when when PCI
adapters have critical registers in the same page as
the MSI-X table.
...
If the driver wants to generate an incorrect MSI-X interrupt
it can do so by requesting the device do a normal memory transfer
to the target address area that raises MSI-X interrupts.
IOMMUs supporting interrupt remapping can prevent this case.
So disabling writes to the MSI-X table (and pending bit array)
areas only raises the bar very slightly.
A device may also give the driver write access to the MSI-X
table through other addresses.
This seems to make disallowing the mapping of the MSI-X table
rather pointless.
If we allow the mapping of the MSI-X table, it seems the guest
kernels of some architectures can write invalid data to MSI-X table
when device drivers initialize MSI-X interrupts.
Regards,
Yongji Xie
I've also dumped out the MSI-X table (during development) to
check that the values are being written there correctly.
David
Ping...
Alex, any comment?
Regards,
Yongji Xie
On 2016/1/15 15:06, Yongji Xie wrote:
Current vfio-pci implementation disallows to mmap
sub-page(size < PAGE_SIZE) MMIO BARs and MSI-X table. This is because
sub-page BARs' mmio page may be shared with other BARs and MSI-X table
should not be accessed directly from the guest for security reasons.
But these will easily cause some performance issues for mmio accesses
in guest when vfio passthrough sub-page BARs or BARs containing MSI-X
table on PPC64 platform. This is because PAGE_SIZE is 64KB by default
on PPC64 platform and the big page may easily hit the sub-page MMIO
BARs' unmmapping and cause the unmmaping of the mmio page which
MSI-X table locate in, which lead to mmio emulation in host.
For sub-page MMIO BARs' unmmapping, this patchset adds a kernel
parameter for PCI resource allocator to enforce the alignment of all
MMIO BARs to be at least PAGE_SZIE and make it enabled by default on
PPC64 platform so that sub-page BAR's mmio page will not be shared
with other BARs. Then we can mmap sub-page MMIO BARs in vfio-pci driver
with this parameter enabled.
For MSI-X table's unmmapping, we think MSI-X table is safe to access
directly from userspace if PCI host bridge support filtering of MSIs
which can ensure that a given pci device can only shoot the MSIs
assigned for it. So we add a pci_host_bridge attribute to indicate
if this PCI host bridge supports filtering of MSIs. Then we can mmap
MSI-X table with this attribute set.
With this patchset applied, we can get almost 100% improvement on
performance for mmio accesses when we passthrough sub-page BARs to guest
in our test.
The two vfio related patches(patch 2 and patch 5) are based on the
proposed patchset[1].
Changelog v3:
- Rebase on new linux kernel mainline with the patchset[1] applied.
- Add a function to check whether PCI BARs'mmio page is shared with
other BARs.
- Add a host bridge attribute to indicate PCI host bridge support
filtering of MSIs.
- Use the new host bridge attribute to check if MSI-X table can
be mmapped instead of CONFIG_EEH.
- Remove Kconfig option VFIO_PCI_MMAP_MSIX
Changelog v2:
- Rebase on v4.4-rc6 with the patchset[1] applied.
- Use kernel parameter to enforce all MMIO BARs to be page aligned
on PCI core code instead of doing it on PPC64 arch code.
- Remove flags: VFIO_DEVICE_FLAGS_PCI_PAGE_ALIGNED
VFIO_DEVICE_FLAGS_PCI_MSIX_MMAP
- Add a Kconfig option to support for mmapping MSI-X table.
[1] https://lkml.org/lkml/2015/11/23/748
Yongji Xie (5):
PCI: Add support for enforcing all MMIO BARs to be page aligned
vfio-pci: Allow to mmap sub-page MMIO BARs if the mmio page is exclusive
PCI: Add host bridge attribute to indicate filtering of MSIs is supported
powerpc/powernv/pci-ioda: Enable msi_filtered bit for any IODA host bridge
vfio-pci: Allow to mmap MSI-X table if host bridge supports filtering of MSIs
Documentation/kernel-parameters.txt | 5 +++++
arch/powerpc/include/asm/pci.h | 11 +++++++++
arch/powerpc/platforms/powernv/pci-ioda.c | 6 +++++
drivers/pci/host-bridge.c | 6 +++++
drivers/pci/pci.c | 35 +++++++++++++++++++++++++++++
drivers/pci/pci.h | 8 ++++++-
drivers/vfio/pci/vfio_pci.c | 13 ++++++++---
include/linux/pci.h | 7 ++++++
8 files changed, 87 insertions(+), 4 deletions(-)
From: Alex Williamson <hidden> Date: 2016-01-28 22:46:16
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
When vfio passthrough 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.
To solve this performance issue, this patch adds a kernel
parameter "pci=resource_page_aligned=on" to enforce
the alignment of all MMIO BARs to be at least PAGE_SIZE,
so that one BAR's mmio page would not be shared with other
BARs. We can also disable it through kernel parameter
"pci=resource_page_aligned=off".
For the default value of the parameter, we think it should be
arch-independent, so we add a macro
HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED to change it. And we
define this macro to enable this parameter by default on PPC64
platform which can easily hit this performance issue because
its PAGE_SIZE is 64KB.
Note that the kernel parameter won't works if kernel doesn't do
resources reallocation.
And where do you account for this so that we know whether it's really in
effect?
@@ -2857,6 +2857,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
PAGE_SIZE is used as alignment.
PCI-PCI bridge can be specified, if resource
windows need to be expanded.
+ resource_page_aligned= Enable/disable enforcing the alignment
+ of all PCI devices' memory resources to be
+ at least PAGE_SIZE if resources reallocation
+ is done by kernel.
+ Format: { "on" | "off" }
ecrc= Enable/disable PCIe ECRC (transaction layer
end-to-end CRC checking).
bios: Use BIOS/firmware settings. This is the
unsigned long pfn,
unsigned long size,
pgprot_t prot);
+#ifdef CONFIG_PPC64
+
+/* For PPC64, We enforce all PCI MMIO BARs to be page aligned
+ * by default. This would be helpful to improve performance
+ * when we passthrough a PCI device of which BARs are smaller
+ * than PAGE_SIZE(64KB). And we can use kernel parameter
+ * "pci=resource_page_aligned=off" to disable it.
+ */
+#define HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED 1
+
+#endif
#define HAVE_ARCH_PCI_RESOURCE_TO_USER
extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
Doesn't this seem rather redundant with the option right above it,
resource_alignment=? Why not modify that to support syntax where all
devices get the same alignment?
quoted hunk
} else if (!strncmp(str, "ecrc=", 5)) {
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
(pci_resource_end((dev), (bar)) - \
pci_resource_start((dev), (bar)) + 1))
+extern bool pci_resources_page_aligned;
+
+bool pci_resources_share_page(struct pci_dev *dev, int resno);
+
/* Similar to the helpers above, these manipulate per-pci_dev
* driver-specific data. They are really just a wrapper around
* the generic device structure functions of these calls.
From: Alex Williamson <hidden> Date: 2016-01-28 22:46:34
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted hunk
MSI-X tables are not allowed to be mmapped in vfio-pci
driver in case that user get to touch this directly.
This will cause some performance issues when when PCI
adapters have critical registers in the same page as
the MSI-X table.
However, some kind of PCI host bridge such as IODA bridge
on Power support filtering of MSIs, which can ensure that a
given pci device can only shoot the MSIs assigned for it.
So we think it's safe to expose the MSI-X table to userspace
if filtering of MSIs is supported because the exposed MSI-X
table can't be used to do harm to other memory space.
To support this case, this patch adds a pci_host_bridge
attribute to indicate if this PCI host bridge supports
filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/pci/host-bridge.c | 6 ++++++
include/linux/pci.h | 3 +++
2 files changed, 9 insertions(+)
int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
+bool pci_host_bridge_msi_filtered_enabled(struct pci_dev *pdev);
+
/*
* The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond
* to P2P or CardBus bridge windows) go in a table. Additional ones (for
Don't we already have a flag for this in the IOMMU space?
enum iommu_cap {
IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache coherent DMA
transactions */
---> IOMMU_CAP_INTR_REMAP, /* IOMMU supports interrupt isolation */
IOMMU_CAP_NOEXEC, /* IOMMU_NOEXEC flag */
};
From: Alex Williamson <hidden> Date: 2016-01-28 22:46:44
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted hunk
Current vfio-pci implementation disallows to mmap MSI-X
table in case that user get to touch this directly.
But we should allow to mmap these MSI-X tables if the PCI
host bridge supports filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/vfio/pci/vfio_pci.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
if (phys_len < PAGE_SIZE || req_start + req_len > phys_len)
return -EINVAL;
- if (index == vdev->msix_bar) {
+ if (!pci_host_bridge_msi_filtered_enabled(pdev) &&
+ index == vdev->msix_bar) {
/*
* Disallow mmaps overlapping the MSI-X table; users don't
* get to touch this directly. We could find somewhere
What about read()/write() access, why would we allow mmap() but not
those?
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted
When vfio passthrough 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.
To solve this performance issue, this patch adds a kernel
parameter "pci=resource_page_aligned=on" to enforce
the alignment of all MMIO BARs to be at least PAGE_SIZE,
so that one BAR's mmio page would not be shared with other
BARs. We can also disable it through kernel parameter
"pci=resource_page_aligned=off".
For the default value of the parameter, we think it should be
arch-independent, so we add a macro
HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED to change it. And we
define this macro to enable this parameter by default on PPC64
platform which can easily hit this performance issue because
its PAGE_SIZE is 64KB.
Note that the kernel parameter won't works if kernel doesn't do
resources reallocation.
And where do you account for this so that we know whether it's really in
effect?
We can check the flag PCI_PROBE_ONLY to know whether kernel do
resources reallocation. Then we know if the kernel parameter is really
in effect.
enum {
/* Force re-assigning all resources (ignore firmware
* setup completely)
*/
PCI_REASSIGN_ALL_RSRC = 0x00000001,
/* Re-assign all bus numbers */
PCI_REASSIGN_ALL_BUS = 0x00000002,
/* Do not try to assign, just use existing setup */
---> PCI_PROBE_ONLY = 0x00000004,
And I will add this to commit log.
@@ -2857,6 +2857,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted. PAGE_SIZE is used as alignment. PCI-PCI bridge can be specified, if resource windows need to be expanded.+ resource_page_aligned= Enable/disable enforcing the alignment+ of all PCI devices' memory resources to be+ at least PAGE_SIZE if resources reallocation+ is done by kernel.+ Format: { "on" | "off" } ecrc= Enable/disable PCIe ECRC (transaction layer end-to-end CRC checking). bios: Use BIOS/firmware settings. This is the
@@ -136,6 +136,17 @@ extern pgprot_t pci_phys_mem_access_prot(struct file *file,unsignedlongpfn,unsignedlongsize,pgprot_tprot);+#ifdef CONFIG_PPC64++/* For PPC64, We enforce all PCI MMIO BARs to be page aligned+*bydefault.Thiswouldbehelpfultoimproveperformance+*whenwepassthroughaPCIdeviceofwhichBARsaresmaller+*thanPAGE_SIZE(64KB).Andwecanusekernelparameter+*"pci=resource_page_aligned=off"todisableit.+*/+#define HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED 1++#endif#define HAVE_ARCH_PCI_RESOURCE_TO_USERexternvoidpci_resource_to_user(conststructpci_dev*dev,intbar,
Doesn't this seem rather redundant with the option right above it,
resource_alignment=? Why not modify that to support syntax where all
devices get the same alignment?
The kernel option will be used to do two things.
Firstly, the option can be used to enable all devices to be page aligned.
Secondly, we can use the option to disable it when the Kconfig option
mentioned above enable all devices to be page aligned by default.
We can easily modify this option "resource_alignment=" to do the first
thing. But I didn't find a proper way to modify it to do the second thing.
quoted
} else if (!strncmp(str, "ecrc=", 5)) {
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
Since we already have resource_alignment=, shouldn't we already have the
code in place to re-align?
Yes, this code can do the re-aligning. But we can't reuse the code because
it re-align device's bars by changing their sizes, which can potentially
break
some drivers.
I'm thinking if we can use IORESOURCE_STARTALIGN for this. Thanks.
Regards,
Yongji Xie
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted
MSI-X tables are not allowed to be mmapped in vfio-pci
driver in case that user get to touch this directly.
This will cause some performance issues when when PCI
adapters have critical registers in the same page as
the MSI-X table.
However, some kind of PCI host bridge such as IODA bridge
on Power support filtering of MSIs, which can ensure that a
given pci device can only shoot the MSIs assigned for it.
So we think it's safe to expose the MSI-X table to userspace
if filtering of MSIs is supported because the exposed MSI-X
table can't be used to do harm to other memory space.
To support this case, this patch adds a pci_host_bridge
attribute to indicate if this PCI host bridge supports
filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/pci/host-bridge.c | 6 ++++++
include/linux/pci.h | 3 +++
2 files changed, 9 insertions(+)
Don't we already have a flag for this in the IOMMU space?
enum iommu_cap {
IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache coherent DMA
transactions */
---> IOMMU_CAP_INTR_REMAP, /* IOMMU supports interrupt isolation */
IOMMU_CAP_NOEXEC, /* IOMMU_NOEXEC flag */
};
I saw this flag had been enabled in x86 and ARM arch.
I'm not sure whether we can mmap MSI-X table in those archs. I just
verify it on PPC64 arch.
Regards.
Yongji Xie
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted
Current vfio-pci implementation disallows to mmap MSI-X
table in case that user get to touch this directly.
But we should allow to mmap these MSI-X tables if the PCI
host bridge supports filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/vfio/pci/vfio_pci.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
From: Alex Williamson <hidden> Date: 2016-01-29 19:01:17
On Fri, 2016-01-29 at 18:37 +0800, Yongji Xie wrote:
On 2016/1/29 6:46, Alex Williamson wrote:
quoted
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted
When vfio passthrough 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.
To solve this performance issue, this patch adds a kernel
parameter "pci=resource_page_aligned=on" to enforce
the alignment of all MMIO BARs to be at least PAGE_SIZE,
so that one BAR's mmio page would not be shared with other
BARs. We can also disable it through kernel parameter
"pci=resource_page_aligned=off".
For the default value of the parameter, we think it should be
arch-independent, so we add a macro
HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED to change it. And we
define this macro to enable this parameter by default on PPC64
platform which can easily hit this performance issue because
its PAGE_SIZE is 64KB.
Note that the kernel parameter won't works if kernel doesn't do
resources reallocation.
And where do you account for this so that we know whether it's really in
effect?
We can check the flag PCI_PROBE_ONLY to know whether kernel do
resources reallocation. Then we know if the kernel parameter is really
in effect.
enum {
/* Force re-assigning all resources (ignore firmware
* setup completely)
*/
PCI_REASSIGN_ALL_RSRC = 0x00000001,
/* Re-assign all bus numbers */
PCI_REASSIGN_ALL_BUS = 0x00000002,
/* Do not try to assign, just use existing setup */
---> PCI_PROBE_ONLY = 0x00000004,
And I will add this to commit log.
We need more than a commit log entry for this, what's the purpose of the
pci_resources_share_page() function if we don't know if this is in
effect?
@@ -2857,6 +2857,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
PAGE_SIZE is used as alignment.
PCI-PCI bridge can be specified, if resource
windows need to be expanded.
+ resource_page_aligned= Enable/disable enforcing the alignment
+ of all PCI devices' memory resources to be
+ at least PAGE_SIZE if resources reallocation
+ is done by kernel.
+ Format: { "on" | "off" }
ecrc= Enable/disable PCIe ECRC (transaction layer
end-to-end CRC checking).
bios: Use BIOS/firmware settings. This is the
unsigned long pfn,
unsigned long size,
pgprot_t prot);
+#ifdef CONFIG_PPC64
+
+/* For PPC64, We enforce all PCI MMIO BARs to be page aligned
+ * by default. This would be helpful to improve performance
+ * when we passthrough a PCI device of which BARs are smaller
+ * than PAGE_SIZE(64KB). And we can use kernel parameter
+ * "pci=resource_page_aligned=off" to disable it.
+ */
+#define HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED 1
+
+#endif
#define HAVE_ARCH_PCI_RESOURCE_TO_USER
extern void pci_resource_to_user(const struct pci_dev *dev, int bar,
Doesn't this seem rather redundant with the option right above it,
resource_alignment=? Why not modify that to support syntax where all
devices get the same alignment?
The kernel option will be used to do two things.
Firstly, the option can be used to enable all devices to be page aligned.
Secondly, we can use the option to disable it when the Kconfig option
mentioned above enable all devices to be page aligned by default.
We can easily modify this option "resource_alignment=" to do the first
thing. But I didn't find a proper way to modify it to do the second thing.
You could allow an arch specified default which is overridden by
specifying a resource_alignment= value. Then you need a way to disable
it, which you could simply do by making
pci_set_resource_alignment_param() able to parse something like
resource_alignment=off.
quoted
quoted
} else if (!strncmp(str, "ecrc=", 5)) {
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
#ifdef CONFIG_PCI_IOV
int resno = res - dev->resource;
- if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END)
+ if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END) {
+ if (pci_resources_page_aligned && res->flags & IORESOURCE_MEM)
+ return PAGE_ALIGN(pci_sriov_resource_alignment(dev,
+ resno));
return pci_sriov_resource_alignment(dev, resno);
+ }
#endif
if (dev->class >> 8 == PCI_CLASS_BRIDGE_CARDBUS)
return pci_cardbus_resource_alignment(res);
+ if (pci_resources_page_aligned && res->flags & IORESOURCE_MEM)
+ return PAGE_ALIGN(resource_alignment(res));
return resource_alignment(res);
}
Since we already have resource_alignment=, shouldn't we already have the
code in place to re-align?
Yes, this code can do the re-aligning. But we can't reuse the code because
it re-align device's bars by changing their sizes, which can potentially
break
some drivers.
I'm thinking if we can use IORESOURCE_STARTALIGN for this. Thanks.
Shouldn't we fix resource_alignment= then to make it behave in a more
compatible way then? resource_alignment=64k,resource_resize=off?
Thanks,
Alex
From: Alex Williamson <hidden> Date: 2016-01-29 19:06:09
----- Original Message -----
On 2016/1/29 6:46, Alex Williamson wrote:
quoted
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted
MSI-X tables are not allowed to be mmapped in vfio-pci
driver in case that user get to touch this directly.
This will cause some performance issues when when PCI
adapters have critical registers in the same page as
the MSI-X table.
However, some kind of PCI host bridge such as IODA bridge
on Power support filtering of MSIs, which can ensure that a
given pci device can only shoot the MSIs assigned for it.
So we think it's safe to expose the MSI-X table to userspace
if filtering of MSIs is supported because the exposed MSI-X
table can't be used to do harm to other memory space.
To support this case, this patch adds a pci_host_bridge
attribute to indicate if this PCI host bridge supports
filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/pci/host-bridge.c | 6 ++++++
include/linux/pci.h | 3 +++
2 files changed, 9 insertions(+)
pci_host_bridge *bridge,
int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
+bool pci_host_bridge_msi_filtered_enabled(struct pci_dev *pdev);
+
/*
* The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that
correspond
* to P2P or CardBus bridge windows) go in a table. Additional ones
(for
Don't we already have a flag for this in the IOMMU space?
enum iommu_cap {
IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache
coherent DMA
transactions */
---> IOMMU_CAP_INTR_REMAP, /* IOMMU supports interrupt
isolation */
IOMMU_CAP_NOEXEC, /* IOMMU_NOEXEC flag */
};
I saw this flag had been enabled in x86 and ARM arch.
I'm not sure whether we can mmap MSI-X table in those archs. I just
verify it on PPC64 arch.
Unfortunately that's not a very good excuse for creating an alternate implementation. When x86 implements interrupt remapping, we get fine grained isolation of MSI vectors and we've always taken this flag to mean that the system is isolated from devices that may perform DoS attacks with MSI writes. I'm not entirely sure whether ARM really provides that degree of isolation, but they would be incorrect is exposing the capability if they do not. Thanks,
Alex
On Fri, 2016-01-29 at 18:37 +0800, Yongji Xie wrote:
quoted
On 2016/1/29 6:46, Alex Williamson wrote:
quoted
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted
When vfio passthrough 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.
To solve this performance issue, this patch adds a kernel
parameter "pci=resource_page_aligned=on" to enforce
the alignment of all MMIO BARs to be at least PAGE_SIZE,
so that one BAR's mmio page would not be shared with other
BARs. We can also disable it through kernel parameter
"pci=resource_page_aligned=off".
For the default value of the parameter, we think it should be
arch-independent, so we add a macro
HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED to change it. And we
define this macro to enable this parameter by default on PPC64
platform which can easily hit this performance issue because
its PAGE_SIZE is 64KB.
Note that the kernel parameter won't works if kernel doesn't do
resources reallocation.
And where do you account for this so that we know whether it's really in
effect?
We can check the flag PCI_PROBE_ONLY to know whether kernel do
resources reallocation. Then we know if the kernel parameter is really
in effect.
enum {
/* Force re-assigning all resources (ignore firmware
* setup completely)
*/
PCI_REASSIGN_ALL_RSRC = 0x00000001,
/* Re-assign all bus numbers */
PCI_REASSIGN_ALL_BUS = 0x00000002,
/* Do not try to assign, just use existing setup */
---> PCI_PROBE_ONLY = 0x00000004,
And I will add this to commit log.
We need more than a commit log entry for this, what's the purpose of the
pci_resources_share_page() function if we don't know if this is in
effect?
It seems the parameter will be always in effect if we reuse the
re-aligning code
of parameter "resource_alignment=" in pci_reassigndev_resource_alignment().
@@ -2857,6 +2857,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted. PAGE_SIZE is used as alignment. PCI-PCI bridge can be specified, if resource windows need to be expanded.+ resource_page_aligned= Enable/disable enforcing the alignment+ of all PCI devices' memory resources to be+ at least PAGE_SIZE if resources reallocation+ is done by kernel.+ Format: { "on" | "off" } ecrc= Enable/disable PCIe ECRC (transaction layer end-to-end CRC checking). bios: Use BIOS/firmware settings. This is the
@@ -136,6 +136,17 @@ extern pgprot_t pci_phys_mem_access_prot(struct file *file,unsignedlongpfn,unsignedlongsize,pgprot_tprot);+#ifdef CONFIG_PPC64++/* For PPC64, We enforce all PCI MMIO BARs to be page aligned+*bydefault.Thiswouldbehelpfultoimproveperformance+*whenwepassthroughaPCIdeviceofwhichBARsaresmaller+*thanPAGE_SIZE(64KB).Andwecanusekernelparameter+*"pci=resource_page_aligned=off"todisableit.+*/+#define HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED 1++#endif#define HAVE_ARCH_PCI_RESOURCE_TO_USERexternvoidpci_resource_to_user(conststructpci_dev*dev,intbar,
I don't think this is proper use of IS_ENABLED, which seems to be
targeted at CONFIG_ type options. You could define this as that in an
arch Kconfig.
Is it better that we define this as a pci Kconfig and select it in arch
Kconfig?
If you want to use IS_ENABLE here, I would think so.
Actually, I'm not sure if it's necessary to add a Kconfig option for it.
I prefer to do it like previous version:
#ifdef HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED
bool pci_resources_page_aligned = true;
#else
bool pci_resources_page_aligned;
#endif
quoted
quoted
quoted
+
/* If set, the PCIe ARI capability will not be used. */
static bool pcie_ari_disabled;
Doesn't this seem rather redundant with the option right above it,
resource_alignment=? Why not modify that to support syntax where all
devices get the same alignment?
The kernel option will be used to do two things.
Firstly, the option can be used to enable all devices to be page aligned.
Secondly, we can use the option to disable it when the Kconfig option
mentioned above enable all devices to be page aligned by default.
We can easily modify this option "resource_alignment=" to do the first
thing. But I didn't find a proper way to modify it to do the second thing.
You could allow an arch specified default which is overridden by
specifying a resource_alignment= value. Then you need a way to disable
it, which you could simply do by making
pci_set_resource_alignment_param() able to parse something like
resource_alignment=off.
We just want to enforce the alignment of all MMIO BARs to be page
aligned in this patch. And both the arch specified default value and
pci_resources_page_aligned are something like *on/off* enforcing the
alignment of resources to be page aligned.
So I think it's better to add a parameter whose format is *on/off*.
If we reuse "resource_alignment=", we need an additional translation
from "resource_alignment="(format: [<order of align>@]...) to
pci_resources_page_aligned(format: true/false).
And "resource_alignment=" is always used to specify alignments of devices.
Would it be misunderstanding to add some syntax like
"resource_alignment=off"?
quoted
quoted
quoted
} else if (!strncmp(str, "ecrc=", 5)) {
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
Since we already have resource_alignment=, shouldn't we already have the
code in place to re-align?
Yes, this code can do the re-aligning. But we can't reuse the code because
it re-align device's bars by changing their sizes, which can potentially
break
some drivers.
I'm thinking if we can use IORESOURCE_STARTALIGN for this. Thanks.
Shouldn't we fix resource_alignment= then to make it behave in a more
compatible way then? resource_alignment=64k,resource_resize=off?
Thanks,
Alex
Good point! We can add something like "resource_resize=off" to
"resource_alignment=". Then we can reuse the re-aligning code. Thanks.
Regards,
Yongji Xie
On Fri, 2016-01-15 at 15:06 +0800, Yongji Xie wrote:
quoted
MSI-X tables are not allowed to be mmapped in vfio-pci
driver in case that user get to touch this directly.
This will cause some performance issues when when PCI
adapters have critical registers in the same page as
the MSI-X table.
However, some kind of PCI host bridge such as IODA bridge
on Power support filtering of MSIs, which can ensure that a
given pci device can only shoot the MSIs assigned for it.
So we think it's safe to expose the MSI-X table to userspace
if filtering of MSIs is supported because the exposed MSI-X
table can't be used to do harm to other memory space.
To support this case, this patch adds a pci_host_bridge
attribute to indicate if this PCI host bridge supports
filtering of MSIs.
Signed-off-by: Yongji Xie <redacted>
---
drivers/pci/host-bridge.c | 6 ++++++
include/linux/pci.h | 3 +++
2 files changed, 9 insertions(+)
pci_host_bridge *bridge,
int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge);
+bool pci_host_bridge_msi_filtered_enabled(struct pci_dev *pdev);
+
/*
* The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that
correspond
* to P2P or CardBus bridge windows) go in a table. Additional ones
(for
Don't we already have a flag for this in the IOMMU space?
enum iommu_cap {
IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache
coherent DMA
transactions */
---> IOMMU_CAP_INTR_REMAP, /* IOMMU supports interrupt
isolation */
IOMMU_CAP_NOEXEC, /* IOMMU_NOEXEC flag */
};
I saw this flag had been enabled in x86 and ARM arch.
I'm not sure whether we can mmap MSI-X table in those archs. I just
verify it on PPC64 arch.
Unfortunately that's not a very good excuse for creating an alternate implementation. When x86 implements interrupt remapping, we get fine grained isolation of MSI vectors and we've always taken this flag to mean that the system is isolated from devices that may perform DoS attacks with MSI writes. I'm not entirely sure whether ARM really provides that degree of isolation, but they would be incorrect is exposing the capability if they do not. Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
OK, I will use this flag in next version. Thanks.
Regards,
Yongji Xie