From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: Xen PCIE hotplug VTD handling Date: Tue, 15 Dec 2009 14:11:19 -0800 Message-ID: <4B280987.1020700@goop.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Wang, Winston L" Cc: "xen-devel@lists.xensource.com" , "Kay, Allen M" List-Id: xen-devel@lists.xenproject.org On 12/15/2009 02:02 PM, Wang, Winston L wrote: > > Hi , > > Attached please review the patch handling PCIE hotplug VTD support > > by adjusting VTD remapping entry after pcie hot add and removal. > This patch needs serious rethinking. You can't just plonk Xen code into the middle of a generic driver. You need to find some other way to get callbacks so you can do the appropriate Xen hypercalls, iff the kernel is actually running under Xen. > Signed-off-by: Winston Wang winston.l.wang@intel.com > > > Signed-off-by: Allen Kay allen.m.kay@intel.com > > > diff -p -r -u > /usr/src/xen-unstable.hg/linux-2.6-pvops.git.org/drivers/pci/hotplug/pciehp_ctrl.c > /usr/src/xen-unstable.hg/linux-2.6-pvops.git/drivers/pci/hotplug/pciehp_ctrl.c > --- > /usr/src/xen-unstable.hg/linux-2.6-pvops.git.org/drivers/pci/hotplug/pciehp_ctrl.c > 2009-12-14 06:19:20.000000000 -0500 +++ > /usr/src/xen-unstable.hg/linux-2.6-pvops.git/drivers/pci/hotplug/pciehp_ctrl.c > 2009-12-14 08:06:42.000000000 -0500 The standard way to generate patches is with just the kernel source directory starting the paths : "linux-base/..." and "linux-mychanges/...". > @@ -34,6 +34,7 @@ #include #include "../pci.h" > #include "pciehp.h" +#include Sticking Xen-specific changes directly into non-Xen files is not a good idea. Particularly since this is not an architecture-specific file, so this will fail to compile on non-x86 systems. > static void interrupt_event_handler(struct work_struct *work); > > @@ -207,9 +208,16 @@ static void set_slot_off(struct controll > static int board_added(struct slot *p_slot) > { > int retval = 0; > + int r = 0; > + > struct controller *ctrl = p_slot->ctrl; > struct pci_bus *parent = ctrl->pci_dev->subordinate; > > + struct physdev_manage_pci manage_pci = { > + .bus = p_slot->bus, > + .devfn = p_slot->device, > + }; > + > ctrl_dbg(ctrl, "%s: slot device, slot offset, hp slot = %d, %d, %d\n", > __func__, p_slot->device, ctrl->slot_device_offset, > p_slot->hp_slot); > @@ -254,7 +262,15 @@ static int board_added(struct slot *p_sl > if (PWR_LED(ctrl)) > p_slot->hpc_ops->green_led_on(p_slot); > > - return 0; > + > + /* > + * Add xen VTD device > + */ > + > + r = HYPERVISOR_physdev_op(PHYSDEVOP_manage_pci_add,&manage_pci); > + ctrl_dbg(ctrl, "HYPERVISOR_physdev_op add return\n",r); > + > + return 0; You can't stick a naked hypercall in here. It will crash if you're not running under Xen. A pvops kernel can choose at runtime whether it is running under Xen, KVM, bare hardware, etc. Also, you haven't even made this depend on CONFIG_XEN, so it will attempt to do the hypercall completely unconditionally, even if building a non-Xen kernel. > > err_exit: > set_slot_off(ctrl, p_slot); > @@ -268,7 +284,13 @@ err_exit: > static int remove_board(struct slot *p_slot) > { > int retval = 0; > - struct controller *ctrl = p_slot->ctrl; > + int r = 0; > + > + struct physdev_manage_pci manage_pci = { > + .bus = p_slot->bus, > + .devfn = p_slot->device,}; > + > + struct controller *ctrl = p_slot->ctrl; > > retval = pciehp_unconfigure_device(p_slot); > if (retval) > @@ -296,6 +318,13 @@ static int remove_board(struct slot *p_s > /* turn off Green LED */ > p_slot->hpc_ops->green_led_off(p_slot); > > + /* > + * Remove xen VTD device > + */ > + > + r = HYPERVISOR_physdev_op(PHYSDEVOP_manage_pci_remove,&manage_pci); > + ctrl_dbg(ctrl,"HYPERVISOR_physdev_op remove return\n",r); > + Same comments apply here. J