This series aims to add an option for PCI resource allocator to
force 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.
To achieve that, we would like to make use of the existing
resource_alignment kernel parameter and force a minimum alignment
of PAGE_SIZE. It's flexible. And we can enable it 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,5 tried to add a new option for resource_alignment to use
IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 6
modified resource_alignment to support syntax which can be used to
enforce the alignment of all MMIO BARs to be at least PAGE_SIZE;
patch 7 enabled this feature by default on PowerNV platform.
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 (7):
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: Do not use IORESOURCE_STARTALIGN to identify bridge resources
PCI: Add support for enforcing all MMIO BARs to be page aligned
PCI: Add a macro to set default alignment for all PCI devices
Documentation/kernel-parameters.txt | 7 +-
arch/powerpc/include/asm/pci.h | 4 ++
drivers/pci/pci.c | 123 +++++++++++++++++++++++++++--------
drivers/pci/setup-bus.c | 9 ++-
4 files changed, 111 insertions(+), 32 deletions(-)
--
1.7.9.5
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 <redacted>
---
drivers/pci/pci.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
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 <redacted>
---
Documentation/kernel-parameters.txt | 5 ++++-
drivers/pci/pci.c | 35 +++++++++++++++++++++++++----------
2 files changed, 29 insertions(+), 11 deletions(-)
@@ -2997,13 +2997,16 @@ 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>@][<domain>:]<bus>:<slot>.<func>+ [:noresize][; ...] 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
@@ -4819,6 +4826,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev){inti;structresource*r;+boolresize=true;resource_size_talign,size;/* We should never try to reassign VF's alignment */
@@ -4826,7 +4834,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;
@@ -4848,15 +4856,22 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)continue;}size=resource_size(r);-if(size<align){-size=align;-dev_info(&dev->dev,-"Rounding up size of resource #%d to %#llx.\n",-i,(unsignedlonglong)size);+if(resize){+if(size<align){+size=align;+dev_info(&dev->dev,+"Rounding up size of resource #%d to %#llx.\n",+i,(unsignedlonglong)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,*toenablethekerneltoreassignnewresource
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. Otherwise,
there will be a backdoor that guest can use to access BARs
of other guest.
To solve this issue, this patch modifies resource_alignment
to support syntax where multiple devices get the same
alignment. So we can use something like
"pci=resource_alignment=*:*:*.*:noresize" 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.
Signed-off-by: Yongji Xie <redacted>
---
Documentation/kernel-parameters.txt | 2 ++
drivers/pci/pci.c | 60 ++++++++++++++++++++++++++++-------
2 files changed, 51 insertions(+), 11 deletions(-)
@@ -3003,6 +3003,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted. aligned memory resources. If <order of align> is not specified, PAGE_SIZE is used as alignment.+ <domain>, <bus>, <slot> and <func> can be set to+ "*" which means match all values. PCI-PCI bridge can be specified, if resource windows need to be expanded. noresize: Don't change the resources' sizes when
@@ -4807,10 +4841,14 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,}if(*p!=';'&&*p!=','){/* End of param or invalid format */+invalid=true;break;}p++;}+if(invalid)+printk(KERN_ERR"PCI: Can't parse resource_alignment parameter:%s\n",+p);spin_unlock(&resource_alignment_lock);returnalign;}
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 <redacted>
---
drivers/pci/pci.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -4822,6 +4822,10 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)resource_size_talign,size;u16command;+/* We should never try to reassign VF's alignment */+if(dev->is_virtfn)+return;+/* check if specified PCI is target device to reassign */align=pci_specified_resource_alignment(dev);if(!align)
We should not disable memory decoding when we reassign alignment
in pci_reassigndev_resource_alignment(). It's meaningless and
have some side effect. For example, some fixup functions such as
quirk_e100_interrupt() read PCI_COMMAND_MEMORY to know whether
the devices has been initialized by the firmware or not. If we
disable memory decoding here, these functions will get a wrong
information that the devices was not initialized by the firmware
which may cause a wrong fixup. Besides, disabling memory decoding
may also break some devices that need to have memory decoding
always-on during probing.
Signed-off-by: Yongji Xie <redacted>
---
drivers/pci/pci.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
@@ -4820,7 +4820,6 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)inti;structresource*r;resource_size_talign,size;-u16command;/* We should never try to reassign VF's alignment */if(dev->is_virtfn)
Now we can use something like "pci=resource_alignment=*:*:*.*:noresize"
to enforce the alignment of all MMIO BARs to be at least PAGE_SIZE so
that we can passthrough sub-page(size < PAGE_SIZE) BARs to guest in
VFIO module.
But sometimes we may want to enable this feature by default on
some platforms such as PowerNV platform which would easily see those
sub-page BARs because of its 64K page. To achieve that, we add a
macro PCIBIOS_DEFAULT_ALIGNMENT to set default alignment for all
PCI devices and define it on PowerNV platform.
Signed-off-by: Yongji Xie <redacted>
---
arch/powerpc/include/asm/pci.h | 4 ++++
drivers/pci/pci.c | 4 ++++
2 files changed, 8 insertions(+)
Now we use the IORESOURCE_STARTALIGN to identify bridge resources
in __assign_resources_sorted(). That's quite fragile. We may also
set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
for example, using the option "noresize" of parameter
"pci=resource_alignment".
In this patch, we try to use a more robust way to identify
bridge resources.
Signed-off-by: Yongji Xie <redacted>
---
drivers/pci/setup-bus.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
On Thu, Jun 30, 2016 at 06:53:07PM +0800, Yongji Xie wrote:
quoted hunk
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 <redacted>
---
drivers/pci/pci.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
spin_lock(&resource_alignment_lock);
p = resource_alignment_param;
+ if (pci_has_flag(PCI_PROBE_ONLY)) {
+ if (*p)
+ printk_once(KERN_INFO "PCI: Ignore resource_alignment parameter: %s with PCI_PROBE_ONLY set\n",
+ p);
We don't have to print @resource_alignment as it's ignored completely.
The content included in it doesn't mean anything:
pr_info_once("PCI: resource_alignment ignored with PCI_PROBE_ONLY\n");
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);
The message would be like below to match PCI code style. I'm thinking it probably
uses dev_dbg() instead dev_info(), but not sure for 100%.
dev_info(&dev->dev, "BAR %d: fixed %pR, no alignment\n", i, r);
On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
quoted hunk
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 <redacted>
---
drivers/pci/pci.c | 4 ++++
1 file changed, 4 insertions(+)
resource_size_t align, size;
u16 command;
+ /* We should never try to reassign VF's alignment */
+ if (dev->is_virtfn)
+ return;
+
Yongji, I think it's correct to ignore VF's BARs. Another concern is:
it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
example here: one PF has 16 VFs; each VF has only one 1KB. It means
the only PF IOV BAR is 16KB. I don't see how it works after expanding
it to 64KB which is the page size. It might be not a problem on PowerNV
platform, but potentially a issue on x86?
/* check if specified PCI is target device to reassign */
align = pci_specified_resource_alignment(dev);
if (!align)
On Thu, Jun 30, 2016 at 06:53:09PM +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 effect. For example, some fixup functions such as
quirk_e100_interrupt() read PCI_COMMAND_MEMORY to know whether
the devices has been initialized by the firmware or not. If we
disable memory decoding here, these functions will get a wrong
information that the devices was not initialized by the firmware
which may cause a wrong fixup. Besides, disabling memory decoding
may also break some devices that need to have memory decoding
always-on during probing.
It seems the changelog isn't correct enough if it's talking about
below check in code:
if (!(command & PCI_COMMAND_MEMORY) || !pci_resource_start(dev, 0))
return;
On Thu, Jun 30, 2016 at 06:53:11PM +0800, Yongji Xie wrote:
Now we use the IORESOURCE_STARTALIGN to identify bridge resources
in __assign_resources_sorted(). That's quite fragile. We may also
set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
for example, using the option "noresize" of parameter
"pci=resource_alignment".
In this patch, we try to use a more robust way to identify
bridge resources.
Signed-off-by: Yongji Xie <redacted>
Reviewed-by: Gavin Shan <redacted>
Yongji, I think this doesn't have to be part of this series, meaning
it can be sent or merged separately.
struct pci_dev_resource *dev_res, *tmp_res, *dev_res2;
unsigned long fail_type;
resource_size_t add_align, align;
+ int index;
/* Check if optional add_size is there */
if (!realloc_head || list_empty(realloc_head))
On Thu, Jun 30, 2016 at 06:53:10PM +0800, Yongji Xie wrote:
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 <redacted>
@@ -2997,13 +2997,16 @@ 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>@][<domain>:]<bus>:<slot>.<func>
+ [:noresize][; ...]
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
/**
* 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;
resource_size_t align = 0;
continue;
}
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;
f->flags |= (IORESOURCE_STARTALIGN | IORESOURCE_UNSET);
Yongji, one quick question - when IORESOURCE_UNSET is cleared? If it's not
cleared, is there any unexpected side effects? Thanks.
+ 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
On Thu, Jun 30, 2016 at 06:53:07PM +0800, Yongji Xie wrote:
quoted
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 <redacted>
---
drivers/pci/pci.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
spin_lock(&resource_alignment_lock);
p = resource_alignment_param;
+ if (pci_has_flag(PCI_PROBE_ONLY)) {
+ if (*p)
+ printk_once(KERN_INFO "PCI: Ignore resource_alignment parameter: %s with PCI_PROBE_ONLY set\n",
+ p);
We don't have to print @resource_alignment as it's ignored completely.
The content included in it doesn't mean anything:
pr_info_once("PCI: resource_alignment ignored with PCI_PROBE_ONLY\n");
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);
The message would be like below to match PCI code style. I'm thinking it probably
uses dev_dbg() instead dev_info(), but not sure for 100%.
dev_info(&dev->dev, "BAR %d: fixed %pR, no alignment\n", i, r);
On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
quoted
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 <redacted>
---
drivers/pci/pci.c | 4 ++++
1 file changed, 4 insertions(+)
resource_size_t align, size;
u16 command;
+ /* We should never try to reassign VF's alignment */
+ if (dev->is_virtfn)
+ return;
+
Yongji, I think it's correct to ignore VF's BARs. Another concern is:
it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
example here: one PF has 16 VFs; each VF has only one 1KB. It means
the only PF IOV BAR is 16KB. I don't see how it works after expanding
it to 64KB which is the page size. It might be not a problem on PowerNV
platform, but potentially a issue on x86?
Seems like the alignment would not be applied to IOV BARs because
pci_reassigndev_resource_alignment() will be called before
sriov_init().
Thanks,
Yongji
On Fri, Jul 01, 2016 at 01:27:17PM +0800, Yongji Xie wrote:
quoted
On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
quoted
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 <redacted>
---
drivers/pci/pci.c | 4 ++++
1 file changed, 4 insertions(+)
resource_size_t align, size;
u16 command;
+ /* We should never try to reassign VF's alignment */
+ if (dev->is_virtfn)
+ return;
+
Yongji, I think it's correct to ignore VF's BARs. Another concern is:
it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
example here: one PF has 16 VFs; each VF has only one 1KB. It means
the only PF IOV BAR is 16KB. I don't see how it works after expanding
it to 64KB which is the page size. It might be not a problem on PowerNV
platform, but potentially a issue on x86?
Seems like the alignment would not be applied to IOV BARs because
pci_reassigndev_resource_alignment() will be called before
sriov_init().
Correct, thanks for the claim. I guess the alignment applied to PF IOV
BARs should be ignored as well? Anyway, the IOV BARs are retireved from
SRIOV capability. It deserves a comment if you plan to take the change.
Actually, the comment here (for ignoring alignment to VF BARs) can be
improved a bit as well, it'd better why the alignment cannot be applied.
Thanks,
Gavin
On Fri, Jul 01, 2016 at 01:27:17PM +0800, Yongji Xie wrote:
quoted
quoted
On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
quoted
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 <redacted>
---
drivers/pci/pci.c | 4 ++++
1 file changed, 4 insertions(+)
resource_size_t align, size;
u16 command;
+ /* We should never try to reassign VF's alignment */
+ if (dev->is_virtfn)
+ return;
+
Yongji, I think it's correct to ignore VF's BARs. Another concern is:
it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
example here: one PF has 16 VFs; each VF has only one 1KB. It means
the only PF IOV BAR is 16KB. I don't see how it works after expanding
it to 64KB which is the page size. It might be not a problem on PowerNV
platform, but potentially a issue on x86?
Seems like the alignment would not be applied to IOV BARs because
pci_reassigndev_resource_alignment() will be called before
sriov_init().
Correct, thanks for the claim. I guess the alignment applied to PF IOV
BARs should be ignored as well? Anyway, the IOV BARs are retireved from
SRIOV capability. It deserves a comment if you plan to take the change.
Actually, the comment here (for ignoring alignment to VF BARs) can be
improved a bit as well, it'd better why the alignment cannot be applied.
Do you mean we should ignore PF IOV BARs like this:
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;
Thanks,
Yongji
On Thu, Jun 30, 2016 at 06:53:10PM +0800, Yongji Xie wrote:
quoted
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 <redacted>
@@ -2997,13 +2997,16 @@ 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>@][<domain>:]<bus>:<slot>.<func>
+ [:noresize][; ...]
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
/**
* 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;
resource_size_t align = 0;
continue;
}
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;
f->flags |= (IORESOURCE_STARTALIGN | IORESOURCE_UNSET);
Yongji, one quick question - when IORESOURCE_UNSET is cleared? If it's not
cleared, is there any unexpected side effects? Thanks.
PCI core will clear this flag in pci_assign_resource() when the BAR
has been assigned properly. If it's not cleared, the driver will fail in
binding the device because pci_enable_resources() will check this flag.
Thanks,
Yongji
On Thu, Jun 30, 2016 at 06:53:11PM +0800, Yongji Xie wrote:
quoted
Now we use the IORESOURCE_STARTALIGN to identify bridge resources
in __assign_resources_sorted(). That's quite fragile. We may also
set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
for example, using the option "noresize" of parameter
"pci=resource_alignment".
In this patch, we try to use a more robust way to identify
bridge resources.
Signed-off-by: Yongji Xie <redacted>
Reviewed-by: Gavin Shan <redacted>
Yongji, I think this doesn't have to be part of this series, meaning
it can be sent or merged separately.
Seems like I give a wrong example in my changelog. The
parameter "pci=resource_alignment" would not set flag
IORESOURCE_STARTALIGN for SR-IOV resources as I replied
to you in previous patch. So this patch has nothing to do
with this series now, I will remove it as you suggested.
Thanks,
Yongji
On Thu, Jun 30, 2016 at 06:53:09PM +0800, Yongji Xie wrote:
quoted
We should not disable memory decoding when we reassign alignment
in pci_reassigndev_resource_alignment(). It's meaningless and
have some side effect. For example, some fixup functions such as
quirk_e100_interrupt() read PCI_COMMAND_MEMORY to know whether
the devices has been initialized by the firmware or not. If we
disable memory decoding here, these functions will get a wrong
information that the devices was not initialized by the firmware
which may cause a wrong fixup. Besides, disabling memory decoding
may also break some devices that need to have memory decoding
always-on during probing.
It seems the changelog isn't correct enough if it's talking about
below check in code:
if (!(command & PCI_COMMAND_MEMORY) || !pci_resource_start(dev, 0))
return;
Could you give me more details? It seems that we couldn't disable
e100 interrupts which enabled by some firmware if we disabling
memory decoding here.
Is there a problem you found with PCI_COMMAND removed? If so, could you
please share more details, thanks.
Disabling memory decoding may break some devices which
have flag mmio_always_on. Besides the mmio_always_on case,
I also found this would break some P2P bridge such as
PEX 8718 controller.
Thanks,
Yongji
On Fri, Jul 01, 2016 at 02:40:16PM +0800, Yongji Xie wrote:
quoted hunk
Hi Gavin,
On 2016/7/1 14:05, Gavin Shan wrote:
quoted
On Fri, Jul 01, 2016 at 01:27:17PM +0800, Yongji Xie wrote:
quoted
quoted
On Thu, Jun 30, 2016 at 06:53:08PM +0800, Yongji Xie wrote:
quoted
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 <redacted>
---
drivers/pci/pci.c | 4 ++++
1 file changed, 4 insertions(+)
resource_size_t align, size;
u16 command;
+ /* We should never try to reassign VF's alignment */
+ if (dev->is_virtfn)
+ return;
+
Yongji, I think it's correct to ignore VF's BARs. Another concern is:
it's safe to apply alignment to PF's IOV BARs? Lets have an extreme
example here: one PF has 16 VFs; each VF has only one 1KB. It means
the only PF IOV BAR is 16KB. I don't see how it works after expanding
it to 64KB which is the page size. It might be not a problem on PowerNV
platform, but potentially a issue on x86?
Seems like the alignment would not be applied to IOV BARs because
pci_reassigndev_resource_alignment() will be called before
sriov_init().
Correct, thanks for the claim. I guess the alignment applied to PF IOV
BARs should be ignored as well? Anyway, the IOV BARs are retireved from
SRIOV capability. It deserves a comment if you plan to take the change.
Actually, the comment here (for ignoring alignment to VF BARs) can be
improved a bit as well, it'd better why the alignment cannot be applied.
Do you mean we should ignore PF IOV BARs like this:
*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;
Yeah, I think it's what I expected. Please add a comment to explain
why PCI bridge windows and PF's IOV BARs are not involed.
Thanks,
Gavin
On Fri, Jul 01, 2016 at 03:04:10PM +0800, Yongji Xie wrote:
Hi Gavin,
On 2016/7/1 10:34, Gavin Shan wrote:
quoted
On Thu, Jun 30, 2016 at 06:53:11PM +0800, Yongji Xie wrote:
quoted
Now we use the IORESOURCE_STARTALIGN to identify bridge resources
in __assign_resources_sorted(). That's quite fragile. We may also
set flag IORESOURCE_STARTALIGN for SR-IOV resources in some cases,
for example, using the option "noresize" of parameter
"pci=resource_alignment".
In this patch, we try to use a more robust way to identify
bridge resources.
Signed-off-by: Yongji Xie <redacted>
Reviewed-by: Gavin Shan <redacted>
Yongji, I think this doesn't have to be part of this series, meaning
it can be sent or merged separately.
Seems like I give a wrong example in my changelog. The
parameter "pci=resource_alignment" would not set flag
IORESOURCE_STARTALIGN for SR-IOV resources as I replied
to you in previous patch. So this patch has nothing to do
with this series now, I will remove it as you suggested.
Thanks, please refine the changelog as it's not related to the
enforced alignment as you claimed when resending it separately.
Thanks,
Gavin
Hi Bjorn,
Any comment on V3?
Thanks,
Yongji
On 2016/6/30 18:53, Yongji Xie wrote:
This series aims to add an option for PCI resource allocator to
force 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.
To achieve that, we would like to make use of the existing
resource_alignment kernel parameter and force a minimum alignment
of PAGE_SIZE. It's flexible. And we can enable it 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,5 tried to add a new option for resource_alignment to use
IORESOURCE_STARTALIGN to specify the alignment of PCI BARs; patch 6
modified resource_alignment to support syntax which can be used to
enforce the alignment of all MMIO BARs to be at least PAGE_SIZE;
patch 7 enabled this feature by default on PowerNV platform.
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 (7):
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: Do not use IORESOURCE_STARTALIGN to identify bridge resources
PCI: Add support for enforcing all MMIO BARs to be page aligned
PCI: Add a macro to set default alignment for all PCI devices
Documentation/kernel-parameters.txt | 7 +-
arch/powerpc/include/asm/pci.h | 4 ++
drivers/pci/pci.c | 123 +++++++++++++++++++++++++++--------
drivers/pci/setup-bus.c | 9 ++-
4 files changed, 111 insertions(+), 32 deletions(-)