Re: Xen PCIE hotplug VTD handling

[Jeremy Fitzhardinge <jeremy@goop.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 
> <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