Re: [PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

2016-02-07 Thread kbuild test robot 
Hi Jake,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.5-rc2 next-20160205]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improving the system]

url:
https://github.com/0day-ci/linux/commits/jakeo-microsoft-com/PCI-hv-New-paravirtual-PCI-front-end-driver/20160203-014543
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-x013-02041016 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/pci/host/hv_pcifront.c: In function 'hv_compose_msi_msg':
>> drivers/pci/host/hv_pcifront.c:830:4: error: 'apic' undeclared (first use in 
>> this function)
  (apic->irq_delivery_mode == dest_LowestPrio) ? 1 : 0;
   ^
   drivers/pci/host/hv_pcifront.c:830:4: note: each undeclared identifier is 
reported only once for each function it appears in

vim +/apic +830 drivers/pci/host/hv_pcifront.c

   824  int_pkt = (struct pci_create_interrupt *)
   825  int_pkt->message_type.message_type = 
PCI_CREATE_INTERRUPT_MESSAGE;
   826  int_pkt->wslot.slot = hpdev->desc.win_slot.slot;
   827  int_pkt->int_desc.vector = cfg->vector;
   828  int_pkt->int_desc.vector_count = 1;
   829  int_pkt->int_desc.delivery_mode =
 > 830  (apic->irq_delivery_mode == dest_LowestPrio) ? 1 : 0;
   831  
   832  /*
   833   * This bit doesn't have to work on machines with more than 64

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

2016-02-03 Thread Bjorn Helgaas 
Hi Jake,

On Tue, Feb 02, 2016 at 05:41:43PM +, ja...@microsoft.com wrote:
> From: Jake Oshins 
> 
> This patch introduces a new driver which exposes a root PCI bus whenever a
> PCI Express device is passed through to a guest VM under Hyper-V. The
> device can be single- or multi-function. The interrupts for the devices
> are managed by an IRQ domain, implemented within the driver.
> 
> Signed-off-by: Jake Oshins 
> ---
>  MAINTAINERS|1 +
>  drivers/pci/Kconfig|7 +
>  drivers/pci/host/Makefile  |1 +
>  drivers/pci/host/hv_pcifront.c | 2248 
> 

This is grossly similar to the other host controller drivers in
drivers/pci/host, so maybe we could name it similarly, too, e.g.,
pci-hyperv.c and CONFIG_PCI_HYPERV?  Or maybe even
CONFIG_PCI_HYPERV_GUEST?

>  4 files changed, 2257 insertions(+)
>  create mode 100644 drivers/pci/host/hv_pcifront.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5161fb9..53f0959 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5185,6 +5185,7 @@ F:  arch/x86/kernel/cpu/mshyperv.c
>  F:   drivers/hid/hid-hyperv.c
>  F:   drivers/hv/
>  F:   drivers/input/serio/hyperv-keyboard.c
> +F:   drivers/pci/host/hv_pcifront.c
>  F:   drivers/net/hyperv/
>  F:   drivers/scsi/storvsc_drv.c
>  F:   drivers/video/fbdev/hyperv_fb.c
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 73de4ef..573b8d6 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -118,4 +118,11 @@ config PCI_LABEL
>   def_bool y if (DMI || ACPI)
>   select NLS
>  
> +config HYPERV_VPCI
> +tristate "Hyper-V PCI Frontend"
> +depends on PCI && X86 && HYPERV && PCI_MSI && PCI_MSI_IRQ_DOMAIN && 
> X86_64
> +help
> +  The PCI device frontend driver allows the kernel to import 
> arbitrary
> +  PCI devices from a PCI backend to support PCI driver domains.
> +
>  source "drivers/pci/host/Kconfig"
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 7b2f20c..6102aa9 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -2,6 +2,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
>  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
>  obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
> +obj-$(CONFIG_HYPERV_VPCI) += hv_pcifront.o
>  obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
>  obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
>  obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
> diff --git a/drivers/pci/host/hv_pcifront.c b/drivers/pci/host/hv_pcifront.c
> new file mode 100644
> index 000..d2ed1be
> --- /dev/null
> +++ b/drivers/pci/host/hv_pcifront.c
> @@ -0,0 +1,2248 @@
> +/*
> + * Copyright (c) Microsoft Corporation.
> + *
> + * Author:
> + *   Jake Oshins 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT.  See the GNU General Public License for more
> + * details.
> + *
> + */

A comment here would help a lot.  Something to the effect of "This
driver runs in a Linux kernel running as a Hyper-V guest on top of a
Windows host.  When the host exports a PCI device to the guest via
Vmbus, this driver creates a virtual PCI root bus for that device to
live on and facilitates config access, resource management for BARs,
and MSI configuration."  Or whatever this actually does.

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Please use dev_info(), dev_err(), etc., everywhere possible below.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Protocol versions. The low word is the minor version, the high word the 
> major
> + * version.
> + */
> +
> +#define PCI_MAKE_VERSION(major, minor) ((__u32)(((major) << 16) | (major)))
> +#define PCI_MAJOR_VERSION(version) ((__u32)(version) >> 16)
> +#define PCI_MINOR_VERSION(version) ((__u32)(version) & 0xff)

I don't know what the rules are for using "u32" vs "__u32".  There are
around 10x as many instances of u32 as there are of __u32 in the
kernel.  I see that you do use "u32" sometimes below, so you probably have
a reason for using __u32 here.

> +enum {
> + PCI_PROTOCOL_VERSION_1_1 = PCI_MAKE_VERSION(1, 1),
> + PCI_PROTOCOL_VERSION_CURRENT = PCI_PROTOCOL_VERSION_1_1
> +};
> +
> +#define PCI_CONFIG_MMIO_LENGTH   0x2000
> +#define MAX_SUPPORTED_MSI_MESSAGES 0x400
> +
> +/*
> + * Message Types
> + */
> +
> +enum pci_message_type {
> + /*
> +  * Version 1.1
> +  */
> + PCI_MESSAGE_BASE= 0x4249,
>

RE: [PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

2016-02-03 Thread Jake Oshins 
> -Original Message-
> From: Bjorn Helgaas [mailto:helg...@kernel.org]
> Sent: Wednesday, February 3, 2016 1:29 PM
> To: Jake Oshins <ja...@microsoft.com>
> Cc: gre...@linuxfoundation.org; KY Srinivasan <k...@microsoft.com>; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; Haiyang Zhang
> <haiya...@microsoft.com>; marc.zyng...@arm.com;
> bhelg...@google.com; linux-...@vger.kernel.org
> Subject: Re: [PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for
> Hyper-V VMs
> 
> Hi Jake,
> 
> On Tue, Feb 02, 2016 at 05:41:43PM +, ja...@microsoft.com wrote:
> > From: Jake Oshins <ja...@microsoft.com>
> >
> > This patch introduces a new driver which exposes a root PCI bus whenever
> a
> > PCI Express device is passed through to a guest VM under Hyper-V. The
> > device can be single- or multi-function. The interrupts for the devices
> > are managed by an IRQ domain, implemented within the driver.

[lots of stuff snipped out]

> > +/**
> > + * hv_pci_free_bridge_windows() - Release memory regions for the
> > + * bus
> > + * @hbus:  Root PCI bus, as understood by this driver
> > + */
> > +static void hv_pci_free_bridge_windows(struct hv_pcibus_device *hbus)
> > +{
> > +   if (hbus->low_mmio_space && hbus->low_mmio_res) {
> > +   hbus->low_mmio_res->flags |= IORESOURCE_BUSY;
> 
> Drivers should not normally set or clear IORESOURCE_BUSY themselves.
> 
> > +   release_mem_region(hbus->low_mmio_res->start,
> > +  resource_size(hbus->low_mmio_res));
> 
> Usually there's a request_mem_region() to correspond with a
> release_mem_region(), and that takes care of IORESOURCE_BUSY.
> 
> What's unique about this driver, and can you make it less unique?
> 

Thanks for the detailed review.  I'll make all the changes that you're 
suggesting and resend.

As for the comment above, and all the others related to IORESOURCE_BUSY, this 
amounts to making the resource claim look like a bridge window, so that callers 
of request_mem_region() in the drivers for the child devices actually can 
succeed.  There's no function in the kernel today that amounts to 
"request_mem_region_for_bridge_window()" or at least none that I understood.  
The current plug and play code in Linux is pretty hard coded for various bus 
types, i.e. ACPI and PCI.  I took a tack at one point where I tried to offer 
changes to it, so that it understood the concept of "grandchild of ACPI or PCI" 
which is what would make this code simpler.  Rafael Wysocki basically told me 
that the pnp layer is deprecated and that new changes to it wouldn't be 
entertained, and that I should find some way of using what exists.  This was 
the result.  If you have another suggestion, I'm happy to try it.  In any case, 
I'll explain what's happening more thoroughly in comments.

Thanks,
Jake Oshins

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

2016-02-02 Thread jakeo 
From: Jake Oshins 

This patch introduces a new driver which exposes a root PCI bus whenever a
PCI Express device is passed through to a guest VM under Hyper-V. The
device can be single- or multi-function. The interrupts for the devices
are managed by an IRQ domain, implemented within the driver.

Signed-off-by: Jake Oshins 
---
 MAINTAINERS|1 +
 drivers/pci/Kconfig|7 +
 drivers/pci/host/Makefile  |1 +
 drivers/pci/host/hv_pcifront.c | 2248 
 4 files changed, 2257 insertions(+)
 create mode 100644 drivers/pci/host/hv_pcifront.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5161fb9..53f0959 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5185,6 +5185,7 @@ F:arch/x86/kernel/cpu/mshyperv.c
 F: drivers/hid/hid-hyperv.c
 F: drivers/hv/
 F: drivers/input/serio/hyperv-keyboard.c
+F: drivers/pci/host/hv_pcifront.c
 F: drivers/net/hyperv/
 F: drivers/scsi/storvsc_drv.c
 F: drivers/video/fbdev/hyperv_fb.c
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 73de4ef..573b8d6 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -118,4 +118,11 @@ config PCI_LABEL
def_bool y if (DMI || ACPI)
select NLS
 
+config HYPERV_VPCI
+tristate "Hyper-V PCI Frontend"
+depends on PCI && X86 && HYPERV && PCI_MSI && PCI_MSI_IRQ_DOMAIN && 
X86_64
+help
+  The PCI device frontend driver allows the kernel to import arbitrary
+  PCI devices from a PCI backend to support PCI driver domains.
+
 source "drivers/pci/host/Kconfig"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 7b2f20c..6102aa9 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
 obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
+obj-$(CONFIG_HYPERV_VPCI) += hv_pcifront.o
 obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
 obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
 obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
diff --git a/drivers/pci/host/hv_pcifront.c b/drivers/pci/host/hv_pcifront.c
new file mode 100644
index 000..d2ed1be
--- /dev/null
+++ b/drivers/pci/host/hv_pcifront.c
@@ -0,0 +1,2248 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ *
+ * Author:
+ *   Jake Oshins 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT.  See the GNU General Public License for more
+ * details.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * Protocol versions. The low word is the minor version, the high word the 
major
+ * version.
+ */
+
+#define PCI_MAKE_VERSION(major, minor) ((__u32)(((major) << 16) | (major)))
+#define PCI_MAJOR_VERSION(version) ((__u32)(version) >> 16)
+#define PCI_MINOR_VERSION(version) ((__u32)(version) & 0xff)
+
+enum {
+   PCI_PROTOCOL_VERSION_1_1 = PCI_MAKE_VERSION(1, 1),
+   PCI_PROTOCOL_VERSION_CURRENT = PCI_PROTOCOL_VERSION_1_1
+};
+
+#define PCI_CONFIG_MMIO_LENGTH 0x2000
+#define MAX_SUPPORTED_MSI_MESSAGES 0x400
+
+/*
+ * Message Types
+ */
+
+enum pci_message_type {
+   /*
+* Version 1.1
+*/
+   PCI_MESSAGE_BASE= 0x4249,
+   PCI_BUS_RELATIONS   = PCI_MESSAGE_BASE + 0,
+   PCI_QUERY_BUS_RELATIONS = PCI_MESSAGE_BASE + 1,
+   PCI_POWER_STATE_CHANGE  = PCI_MESSAGE_BASE + 4,
+   PCI_QUERY_RESOURCE_REQUIREMENTS = PCI_MESSAGE_BASE + 5,
+   PCI_QUERY_RESOURCE_RESOURCES= PCI_MESSAGE_BASE + 6,
+   PCI_BUS_D0ENTRY = PCI_MESSAGE_BASE + 7,
+   PCI_BUS_D0EXIT  = PCI_MESSAGE_BASE + 8,
+   PCI_READ_BLOCK  = PCI_MESSAGE_BASE + 9,
+   PCI_WRITE_BLOCK = PCI_MESSAGE_BASE + 0xA,
+   PCI_EJECT   = PCI_MESSAGE_BASE + 0xB,
+   PCI_QUERY_STOP  = PCI_MESSAGE_BASE + 0xC,
+   PCI_REENABLE= PCI_MESSAGE_BASE + 0xD,
+   PCI_QUERY_STOP_FAILED   = PCI_MESSAGE_BASE + 0xE,
+   PCI_EJECTION_COMPLETE   = PCI_MESSAGE_BASE + 0xF,
+   PCI_RESOURCES_ASSIGNED  = PCI_MESSAGE_BASE + 0x10,
+   PCI_RESOURCES_RELEASED  = PCI_MESSAGE_BASE + 0x11,
+   PCI_INVALIDATE_BLOCK= PCI_MESSAGE_BASE + 0x12,
+   PCI_QUERY_PROTOCOL_VERSION  = PCI_MESSAGE_BASE + 0x13,
+