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
> <mailto:winston.l.wang@intel.com>
>
> Signed-off-by: Allen Kay allen.m.kay@intel.com
> <mailto: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 <linux/workqueue.h> #include
"../pci.h"
> #include "pciehp.h" +#include <asm/xen/hypercall.h>
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel