Messages in this thread | | | Subject | Re: [RFC PATCH v2 1/3] PCI: Add support for enforcing all MMIO BARs to be page aligned | From | Yongji Xie <> | Date | Tue, 5 Jan 2016 19:04:40 +0800 |
| |
On 2016/1/5 4:47, Alex Williamson wrote: > On Thu, 2015-12-31 at 16:50 +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 alignments 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". > Shouldn't this somehow be associated with the realloc option? I don't > think PCI code will attempt to reprogram anything unless it needs to > otherwise.
So you mean we need to ignore firmware setup and force re-assigning all resources if we want to use the option "pci=resource_page_aligned=on"?
>> For the default value of this parameter, we think it should be >> arch-independent, so we add a macro PCI_RESOURCE_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. >> >> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >> --- >> Documentation/kernel-parameters.txt | 4 ++++ >> arch/powerpc/include/asm/pci.h | 11 +++++++++++ >> drivers/pci/pci.c | 17 +++++++++++++++++ >> drivers/pci/pci.h | 7 ++++++- >> include/linux/pci.h | 2 ++ >> 5 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt >> index 742f69d..a53aaee 100644 >> --- a/Documentation/kernel-parameters.txt >> +++ b/Documentation/kernel-parameters.txt >> @@ -2857,6 +2857,10 @@ 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. >> + Format: { "on" | "off" } >> ecrc= Enable/disable PCIe ECRC (transaction layer >> end-to-end CRC checking). >> bios: Use BIOS/firmware settings. This is the >> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h >> index 3453bd8..27bff59 100644 >> --- a/arch/powerpc/include/asm/pci.h >> +++ b/arch/powerpc/include/asm/pci.h >> @@ -136,6 +136,17 @@ extern pgprot_t pci_phys_mem_access_prot(struct file *file, >> 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 bootcmd >> + * "pci=resource_page_aligned=off" to disable it. >> + */ >> +#define PCI_ENABLE_RESOURCE_PAGE_ALIGNED >> + >> +#endif > This should be done with something like HAVE_PCI_DEFAULT_RESOURCE_PAGE_ > ALIGNED in arch/powerpc/include/asm
OK, I will fix it in next version.
>> #define HAVE_ARCH_PCI_RESOURCE_TO_USER >> extern void pci_resource_to_user(const struct pci_dev *dev, int bar, >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 314db8c..9f14ba5 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -99,6 +99,13 @@ u8 pci_cache_line_size; >> */ >> unsigned int pcibios_max_latency = 255; >> >> +#ifdef PCI_ENABLE_RESOURCE_PAGE_ALIGNED >> +bool pci_resource_page_aligned = true; >> +#else >> +bool pci_resource_page_aligned; >> +#endif >> +EXPORT_SYMBOL(pci_resource_page_aligned); > Couldn't this be done in a single line with IS_ENABLED() macro?
I'm not sure whether IS_ENABLED() macro should be used there because it is always used for CONFIG_ options.
> Should this symbol be GPL-only?
Yes, it will be fixed in next version.
>> + >> /* If set, the PCIe ARI capability will not be used. */ >> static bool pcie_ari_disabled; >> >> @@ -4746,6 +4753,14 @@ static ssize_t pci_resource_alignment_store(struct bus_type *bus, >> BUS_ATTR(resource_alignment, 0644, pci_resource_alignment_show, >> pci_resource_alignment_store); >> >> +static void pci_resource_get_page_aligned(char *str) >> +{ >> + if (!strncmp(str, "off", 3)) >> + pci_resource_page_aligned = false; >> + else if (!strncmp(str, "on", 2)) >> + pci_resource_page_aligned = true; >> +} >> + >> static int __init pci_resource_alignment_sysfs_init(void) >> { >> return bus_create_file(&pci_bus_type, >> @@ -4859,6 +4874,8 @@ static int __init pci_setup(char *str) >> } else if (!strncmp(str, "resource_alignment=", 19)) { >> pci_set_resource_alignment_param(str + 19, >> strlen(str + 19)); >> + } else if (!strncmp(str, "resource_page_aligned=", 22)) { >> + pci_resource_get_page_aligned(str + 22); >> } else if (!strncmp(str, "ecrc=", 5)) { >> pcie_ecrc_get_policy(str + 5); >> } else if (!strncmp(str, "hpiosize=", 9)) { >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index d390fc1..e16e48c 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -312,11 +312,16 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev, >> #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_resource_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_resource_page_aligned && res->flags & IORESOURCE_MEM) >> + return PAGE_ALIGN(resource_alignment(res)); >> return resource_alignment(res); >> } > Why are we calling resource_alignment() and then doing another > alignment on top of that? Shouldn't this alignment be done there?
We doing the alignment in pci_resource_alignment() because this is a PCI specific feature. And I don't think pci_resource_page_aligned should be used in resource_alignment().
> I still don't really understand if this is enough to produce a > reallocation if the alignment is not sufficient, vfio would need to > know if pci_resource_page_aligned has any loopholes.
Do you mean doing this in pci_resource_alignment() may not be enough to make sure the alignments of all PCI resources are page aligned? Thanks.
Regards Yongji Xie
>> >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 6ae25aa..0ca57f1 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -1517,6 +1517,8 @@ static inline int pci_get_new_domain_nr(void) { return -ENOSYS; } >> >> #include >> >> +extern bool pci_resource_page_aligned; >> + >> /* these helpers provide future and backwards compatibility >> * for accessing popular PCI BAR info */ >> #define pci_resource_start(dev, bar) ((dev)->resource[(bar)].start)
| |