diff mbox

[v9,2/3] PCI: Add a macro to set default alignment for all PCI devices

Message ID 20170323205342.GB23612@bhelgaas-glaptop.roam.corp.google.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Bjorn Helgaas March 23, 2017, 8:53 p.m. UTC
Hi Yongji,

On Wed, Feb 15, 2017 at 02:45:05PM +0800, Yongji Xie wrote:
> 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.

Please include a pointer to the VFIO code that enforces this.  It's
not obvious to me how it would do this.  This doesn't change the
*size* of the resource, only the alignment.  So if VFIO sees a BAR
like [mem 0x80000000-0x800000ff], it knows the BAR is aligned enough
that it *could* be the only thing on a page, but I don't know how it
could know that there will never be another BAR at 0x80000100.  Even
if there's no other BAR on that page *now*, it would have to know that
no hot-added device will ever be placed on that page.

> 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(+), 0 deletions(-)
> 
> 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 a881c0d..2622e9b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4965,6 +4965,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

I'd prefer to move this #ifdef out of the code path, as in the patch
below.

I don't know how powerpc kernels are built: can you build a kernel
that will run on PowerNV as well as on different powerpc systems?  If
so, is it acceptable to force that kernel to user 64K alignment even
when it's running on non-PowerNV systems?  Or do you need a function
call here instead of a #define?

>  	spin_lock(&resource_alignment_lock);
>  	p = resource_alignment_param;
>  	if (!*p)


commit 60106ae302a264f9be15fa736dae2ea51140b988
Author: Yongji Xie <xyjxie@linux.vnet.ibm.com>
Date:   Wed Feb 15 14:45:05 2017 +0800

    PCI: Add PCIBIOS_DEFAULT_ALIGNMENT for arch-specific alignment control
    
    When VFIO passes through a PCI device to a guest, it does not allow the
    guest direct access to BARs that are smaller than PAGE_SIZE.  This is
    because a page might contain several small BARs for unrelated devices and
    a guest should not be able to access all of them.
    
    VFIO emulates guest accesses to these small BARs, which is functional but
    slow.  On systems with large page sizes, e.g., PowerNV with 64K pages, BARs
    are more likely to share a page and performance is more of a problem.
    
    Add a macro to set default alignment for all PCI devices.  An arch can set
    this to PAGE_SIZE to prevent memory BARs from sharing a page.
    
    [bhelgaas: add default PCIBIOS_DEFAULT_ALIGNMENT definition, changelog]
    Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Comments

Bjorn Helgaas March 23, 2017, 9:57 p.m. UTC | #1
On Thu, Mar 23, 2017 at 03:53:42PM -0500, Bjorn Helgaas wrote:
> Hi Yongji,
> 
> On Wed, Feb 15, 2017 at 02:45:05PM +0800, Yongji Xie wrote:
> > 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.
> 
> Please include a pointer to the VFIO code that enforces this.  It's
> not obvious to me how it would do this.  This doesn't change the
> *size* of the resource, only the alignment.  So if VFIO sees a BAR
> like [mem 0x80000000-0x800000ff], it knows the BAR is aligned enough
> that it *could* be the only thing on a page, but I don't know how it
> could know that there will never be another BAR at 0x80000100.  Even
> if there's no other BAR on that page *now*, it would have to know that
> no hot-added device will ever be placed on that page.

Never mind, I found it.  I updated the changelog like this; please
correct anything I got wrong:

    When VFIO passes through a PCI device to a guest, it does not
    allow the guest to mmap BARs that are smaller than PAGE_SIZE
    unless it can reserve the rest of the page (see
    vfio_pci_probe_mmaps()).  This is because a page might contain
    several small BARs for unrelated devices and a guest should not be
    able to access all of them.

    VFIO emulates guest accesses to non-mappable BARs, which is
    functional but slow.  On systems with large page sizes, e.g.,
    PowerNV with 64K pages, BARs are more likely to share a page and
    performance is more likely to be a problem.

    Add a macro to set default alignment for all PCI devices.  An arch
    can set this to PAGE_SIZE to force the PCI core to place memory
    BARs on their own pages.
Yongji Xie March 25, 2017, 12:36 p.m. UTC | #2
On Thu, Mar 23, 2017 at 03:53:42PM -0500, Bjorn Helgaas wrote:
> Hi Yongji,
>
> On Wed, Feb 15, 2017 at 02:45:05PM +0800, Yongji Xie wrote:
>> 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.
>
> Please include a pointer to the VFIO code that enforces this.  It's
> not obvious to me how it would do this.  This doesn't change the
> *size* of the resource, only the alignment.  So if VFIO sees a BAR
> like [mem 0x80000000-0x800000ff], it knows the BAR is aligned enough
> that it *could* be the only thing on a page, but I don't know how it
> could know that there will never be another BAR at 0x80000100.  Even
> if there's no other BAR on that page *now*, it would have to know that
> no hot-added device will ever be placed on that page.
>
>> 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(+), 0 deletions(-)
>>
>> 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 a881c0d..2622e9b 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4965,6 +4965,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
>
> I'd prefer to move this #ifdef out of the code path, as in the patch
> below.
>
> I don't know how powerpc kernels are built: can you build a kernel
> that will run on PowerNV as well as on different powerpc systems?  If
> so, is it acceptable to force that kernel to user 64K alignment even
> when it's running on non-PowerNV systems?  Or do you need a function
> call here instead of a #define?
>

That's a good point, the macro may be also defined on non-PowerNV systems.
Maybe an arch-specific function is more reasonable here.

Thanks,
Yongji
Michael Ellerman March 27, 2017, 10:17 a.m. UTC | #3
Bjorn Helgaas <helgaas@kernel.org> writes:
>
> I don't know how powerpc kernels are built: can you build a kernel
> that will run on PowerNV as well as on different powerpc systems?

Yes you can. But there are some restrictions.

You can only build a 64-bit OR a 32-bit kernel.

And within 64-bit you can only build a *kernel* for the "server
architecture (~= IBM CPUs)" or the "embedded architecture
(Freescale/NXP)".

So in practice you can build a 64-bit kernel that supports:
 - powernv (bare metal on IBM server chips)
 - pseries (virtualised on IBM server chips or qemu)
 - powermac (Apple G5s)
 - pasemi (long dead but still used by some Amiga folks?)
 - Cell/PS3 (also long dead)

> If so, is it acceptable to force that kernel to user 64K alignment even
> when it's running on non-PowerNV systems?

Probably, but I'm not sure TBH. Benh will know, I'll try and get his
attention.

cheers
Benjamin Herrenschmidt March 27, 2017, 10:25 a.m. UTC | #4
On Mon, 2017-03-27 at 21:17 +1100, Michael Ellerman wrote:
> > If so, is it acceptable to force that kernel to user 64K alignment
> > even
> > when it's running on non-PowerNV systems?
> 
> Probably, but I'm not sure TBH. Benh will know, I'll try and get his
> attention.

Can we make the alignment PAGE_SIZE ? I think that should be ok as long
as it doesn't try to re-allocate BARs for devices that have already
been properly allocated by firmware (ie, PowerMac, if you mess around
with the MacIO chip on these, bad things will happen).

Cheers,
Ben.
Bjorn Helgaas March 30, 2017, 11:13 p.m. UTC | #5
On Mon, Mar 27, 2017 at 09:25:50PM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2017-03-27 at 21:17 +1100, Michael Ellerman wrote:
> > > If so, is it acceptable to force that kernel to user 64K alignment
> > > even
> > > when it's running on non-PowerNV systems?
> > 
> > Probably, but I'm not sure TBH. Benh will know, I'll try and get his
> > attention.
> 
> Can we make the alignment PAGE_SIZE ? I think that should be ok as long
> as it doesn't try to re-allocate BARs for devices that have already
> been properly allocated by firmware (ie, PowerMac, if you mess around
> with the MacIO chip on these, bad things will happen).

I think this *will* change BARs even if they've already been allocated
by firmware, because it affects this path that is used for every
device:

  pci_device_add
    pci_reassigndev_resource_alignment
      pci_specified_resource_alignment
        align = PCIBIOS_DEFAULT_ALIGNMENT

I'm looking for an update that fixes the minor comment issues and
possibly addresses this PowerMac question.

Bjorn
diff mbox

Patch

diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
index 93eded8d3843..dc3c81ab4cc4 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 7904d02ffdb9..e9a079063fd0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4962,7 +4962,7 @@  static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev)
 {
 	int seg, bus, slot, func, align_order, count;
 	unsigned short vendor, device, subsystem_vendor, subsystem_device;
-	resource_size_t align = 0;
+	resource_size_t align = PCIBIOS_DEFAULT_ALIGNMENT;
 	char *p;
 
 	spin_lock(&resource_alignment_lock);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index eb3da1a04e6c..618beb5809d1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1630,6 +1630,10 @@  static inline int pci_get_new_domain_nr(void) { return -ENOSYS; }
 #define pci_root_bus_fwnode(bus)	NULL
 #endif
 
+#ifndef PCIBIOS_DEFAULT_ALIGNMENT
+#define PCIBIOS_DEFAULT_ALIGNMENT	0
+#endif
+
 /* these helpers provide future and backwards compatibility
  * for accessing popular PCI BAR info */
 #define pci_resource_start(dev, bar)	((dev)->resource[(bar)].start)