Re: [PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs
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 *)&ctxt.pkt.message; 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
> -Original Message- > From: Bjorn Helgaas [mailto:helg...@kernel.org] > Sent: Wednesday, February 3, 2016 1:29 PM > To: Jake Oshins > Cc: gre...@linuxfoundation.org; KY Srinivasan ; linux- > ker...@vger.kernel.org; de...@linuxdriverproject.org; Haiyang Zhang > ; 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 > > > > 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
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 > > 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, > + PCI_BUS_RELATIONS = PCI_MESSAGE_BASE + 0, >