Re: [RFC PATCH v3 1/5] PCI: Add support for enforcing all MMIO BARs to be page aligned

[Alex Williamson]

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?

> Signed-off-by: Yongji Xie <xyjxie@xxxxxxxxxxxxxxxxxx>
> ---
> Â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(-)
>Â
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 742f69d..3f2a7c9 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -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
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index 3453bd8..2d2b3ef 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 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,
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 314db8c..7b21238 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -99,6 +99,9 @@ u8 pci_cache_line_size;
> Â */
> Âunsigned int pcibios_max_latency = 255;
> Â
> +bool pci_resources_page_aligned =
> +	IS_ENABLED(HAVE_PCI_DEFAULT_RESOURCES_PAGE_ALIGNED);

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.

> +
> Â/* If set, the PCIe ARI capability will not be used. */
> Âstatic bool pcie_ari_disabled;
> Â
> @@ -4746,6 +4749,35 @@ 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_resources_get_page_aligned(char *str)
> +{
> +	if (!strncmp(str, "off", 3))
> +		pci_resources_page_aligned = false;
> +	else if (!strncmp(str, "on", 2))
> +		pci_resources_page_aligned = true;
> +}

"get"?

> +
> +/*
> + * This function checks whether PCI BARs' mmio page will be shared
> + * with other BARs.
> + */
> +bool pci_resources_share_page(struct pci_dev *dev, int resno)
> +{
> +	struct resource *res = dev->resource + resno;
> +
> +	if (resource_size(res) >= PAGE_SIZE)
> +		return false;
> +	if (pci_resources_page_aligned && !(res->start & ~PAGE_MASK) &&
> +	ÂÂÂÂres->flags & IORESOURCE_MEM) {
> +		if (res->sibling)
> +			return (res->sibling->start & ~PAGE_MASK);
> +		else
> +			return false;
> +	}
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(pci_resources_share_page);
> +
> Âstatic int __init pci_resource_alignment_sysfs_init(void)
> Â{
> Â	return bus_create_file(&pci_bus_type,
> @@ -4859,6 +4891,9 @@ 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_resources_get_page_aligned(str + 22);

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?


> Â			} 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..b9b333d 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -312,11 +312,17 @@ 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_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?

> Â
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6ae25aa..b640d65 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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))
> Â
> +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.