diff mbox

[2/6] pci: Scan all functions when probing while running over Jailhouse

Message ID 66bfcfc6dc1832baa3fbd8e4879764d36aa9c1e7.1516601570.git.jan.kiszka@siemens.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Jan Kiszka Jan. 22, 2018, 6:12 a.m. UTC 
From: Jan Kiszka <jan.kiszka@siemens.com>

PCI and PCIBIOS probing only scans devices at function number 0/8/16/...
Subdevices (e.g. multiqueue) have function numbers which are not a
multiple of 8.

The simple hypervisor Jailhouse passes subdevices directly w/o providing
a virtual PCI topology like KVM. As a consequence a PCI passthrough from
Jailhouse to a guest will not be detected by Linux.

Based on patch by Benedikt Spranger, adding Jailhouse probing to avoid
changing the behavior in the absence of the hypervisor.

CC: Benedikt Spranger <b.spranger@linutronix.de>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/pci/legacy.c | 4 +++-
 drivers/pci/probe.c   | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas Feb. 22, 2018, 8:57 p.m. UTC | #1 
On Mon, Jan 22, 2018 at 07:12:46AM +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> PCI and PCIBIOS probing only scans devices at function number 0/8/16/...
> Subdevices (e.g. multiqueue) have function numbers which are not a
> multiple of 8.

Suggested text:

  Per PCIe r4.0, sec 7.5.1.1.9, multi-function devices are required to
  have a function 0.  Therefore, Linux scans for devices at function 0
  (devfn 0/8/16/...) and only scans for other functions if function 0
  has its Multi-Function Device bit set or ARI or SR-IOV indicate
  there are more functions.
  
  The Jailhouse hypervisor may pass individual functions of a
  multi-function device to a guest without passing function 0, which
  means a Linux guest won't find them.

  Change Linux PCI probing so it scans all function numbers when
  running as a guest over Jailhouse.
  
  This is technically prohibited by the spec, so it is possible that
  PCI devices without the Multi-Function Device bit set may have
  unexpected behavior in response to this probe.

> The simple hypervisor Jailhouse passes subdevices directly w/o providing
> a virtual PCI topology like KVM. As a consequence a PCI passthrough from
> Jailhouse to a guest will not be detected by Linux.
> 
> Based on patch by Benedikt Spranger, adding Jailhouse probing to avoid
> changing the behavior in the absence of the hypervisor.
> 
> CC: Benedikt Spranger <b.spranger@linutronix.de>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

With subject change to:

  PCI: Scan all functions when running over Jailhouse

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  arch/x86/pci/legacy.c | 4 +++-
>  drivers/pci/probe.c   | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
> index 1cb01abcb1be..a7b0476b4f44 100644
> --- a/arch/x86/pci/legacy.c
> +++ b/arch/x86/pci/legacy.c
> @@ -5,6 +5,7 @@
>  #include <linux/export.h>
>  #include <linux/pci.h>
>  #include <asm/pci_x86.h>
> +#include <asm/jailhouse_para.h>
>  
>  /*
>   * Discover remaining PCI buses in case there are peer host bridges.
> @@ -34,13 +35,14 @@ int __init pci_legacy_init(void)
>  
>  void pcibios_scan_specific_bus(int busn)
>  {
> +	int stride = jailhouse_paravirt() ? 1 : 8;
>  	int devfn;
>  	u32 l;
>  
>  	if (pci_find_bus(0, busn))
>  		return;
>  
> -	for (devfn = 0; devfn < 256; devfn += 8) {
> +	for (devfn = 0; devfn < 256; devfn += stride) {
>  		if (!raw_pci_read(0, busn, devfn, PCI_VENDOR_ID, 2, &l) &&
>  		    l != 0x0000 && l != 0xffff) {
>  			DBG("Found device at %02x:%02x [%04x]\n", busn, devfn, l);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 14e0ea1ff38b..60ad14c8245f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -17,6 +17,7 @@
>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/hypervisor.h>
>  #include "pci.h"
>  
>  #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
> @@ -2454,6 +2455,7 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  					      unsigned int available_buses)
>  {
>  	unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0;
> +	unsigned int stride = jailhouse_paravirt() ? 1 : 8;
>  	unsigned int start = bus->busn_res.start;
>  	unsigned int devfn, cmax, max = start;
>  	struct pci_dev *dev;
> @@ -2461,7 +2463,7 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  	dev_dbg(&bus->dev, "scanning bus\n");
>  
>  	/* Go find them, Rover! */
> -	for (devfn = 0; devfn < 0x100; devfn += 8)
> +	for (devfn = 0; devfn < 0x100; devfn += stride)
>  		pci_scan_slot(bus, devfn);
>  
>  	/* Reserve buses for SR-IOV capability. */
> -- 
> 2.13.6
>
Andy Shevchenko Feb. 23, 2018, 1:23 p.m. UTC | #2 
On Mon, Jan 22, 2018 at 8:12 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

>  #include <linux/export.h>
>  #include <linux/pci.h>
>  #include <asm/pci_x86.h>
> +#include <asm/jailhouse_para.h>

Keep it in order?


>  #include <linux/acpi.h>
>  #include <linux/irqdomain.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/hypervisor.h>

Ditto.
Jan Kiszka Feb. 27, 2018, 7:22 a.m. UTC | #3 
On 2018-02-23 14:23, Andy Shevchenko wrote:
> On Mon, Jan 22, 2018 at 8:12 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>>  #include <linux/export.h>
>>  #include <linux/pci.h>
>>  #include <asm/pci_x86.h>
>> +#include <asm/jailhouse_para.h>
> 
> Keep it in order?
> 

Done.

> 
>>  #include <linux/acpi.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/hypervisor.h>
> 
> Ditto.
> 

Despite the context suggesting it, this file has no ordering.

Jan
Jan Kiszka Feb. 27, 2018, 7:25 a.m. UTC | #4 
On 2018-02-22 21:57, Bjorn Helgaas wrote:
> On Mon, Jan 22, 2018 at 07:12:46AM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> PCI and PCIBIOS probing only scans devices at function number 0/8/16/...
>> Subdevices (e.g. multiqueue) have function numbers which are not a
>> multiple of 8.
> 
> Suggested text:
> 
>   Per PCIe r4.0, sec 7.5.1.1.9, multi-function devices are required to
>   have a function 0.  Therefore, Linux scans for devices at function 0
>   (devfn 0/8/16/...) and only scans for other functions if function 0
>   has its Multi-Function Device bit set or ARI or SR-IOV indicate
>   there are more functions.
>   
>   The Jailhouse hypervisor may pass individual functions of a
>   multi-function device to a guest without passing function 0, which
>   means a Linux guest won't find them.
> 
>   Change Linux PCI probing so it scans all function numbers when
>   running as a guest over Jailhouse.
>   
>   This is technically prohibited by the spec, so it is possible that
>   PCI devices without the Multi-Function Device bit set may have
>   unexpected behavior in response to this probe.
> 
>> The simple hypervisor Jailhouse passes subdevices directly w/o providing
>> a virtual PCI topology like KVM. As a consequence a PCI passthrough from
>> Jailhouse to a guest will not be detected by Linux.
>>
>> Based on patch by Benedikt Spranger, adding Jailhouse probing to avoid
>> changing the behavior in the absence of the hypervisor.
>>
>> CC: Benedikt Spranger <b.spranger@linutronix.de>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> With subject change to:
> 
>   PCI: Scan all functions when running over Jailhouse
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 

Thanks, all suggestions picked up for next round.

Jan
Andy Shevchenko Feb. 27, 2018, 3:48 p.m. UTC | #5 
On Tue, Feb 27, 2018 at 9:22 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2018-02-23 14:23, Andy Shevchenko wrote:
>> On Mon, Jan 22, 2018 at 8:12 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:

>>>  #include <linux/acpi.h>
>>>  #include <linux/irqdomain.h>
>>>  #include <linux/pm_runtime.h>
>>> +#include <linux/hypervisor.h>
>>
>> Ditto.
>>
>
> Despite the context suggesting it, this file has no ordering.

At least you might not increase disordering by putting the line after acpi.h.
diff mbox

Patch

diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
index 1cb01abcb1be..a7b0476b4f44 100644
--- a/arch/x86/pci/legacy.c
+++ b/arch/x86/pci/legacy.c
@@ -5,6 +5,7 @@ 
 #include <linux/export.h>
 #include <linux/pci.h>
 #include <asm/pci_x86.h>
+#include <asm/jailhouse_para.h>
 
 /*
  * Discover remaining PCI buses in case there are peer host bridges.
@@ -34,13 +35,14 @@  int __init pci_legacy_init(void)
 
 void pcibios_scan_specific_bus(int busn)
 {
+	int stride = jailhouse_paravirt() ? 1 : 8;
 	int devfn;
 	u32 l;
 
 	if (pci_find_bus(0, busn))
 		return;
 
-	for (devfn = 0; devfn < 256; devfn += 8) {
+	for (devfn = 0; devfn < 256; devfn += stride) {
 		if (!raw_pci_read(0, busn, devfn, PCI_VENDOR_ID, 2, &l) &&
 		    l != 0x0000 && l != 0xffff) {
 			DBG("Found device at %02x:%02x [%04x]\n", busn, devfn, l);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 14e0ea1ff38b..60ad14c8245f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -17,6 +17,7 @@ 
 #include <linux/acpi.h>
 #include <linux/irqdomain.h>
 #include <linux/pm_runtime.h>
+#include <linux/hypervisor.h>
 #include "pci.h"
 
 #define CARDBUS_LATENCY_TIMER	176	/* secondary latency timer */
@@ -2454,6 +2455,7 @@  static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 					      unsigned int available_buses)
 {
 	unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0;
+	unsigned int stride = jailhouse_paravirt() ? 1 : 8;
 	unsigned int start = bus->busn_res.start;
 	unsigned int devfn, cmax, max = start;
 	struct pci_dev *dev;
@@ -2461,7 +2463,7 @@  static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 	dev_dbg(&bus->dev, "scanning bus\n");
 
 	/* Go find them, Rover! */
-	for (devfn = 0; devfn < 0x100; devfn += 8)
+	for (devfn = 0; devfn < 0x100; devfn += stride)
 		pci_scan_slot(bus, devfn);
 
 	/* Reserve buses for SR-IOV capability. */