Re: [Xen-devel] [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don't blow up.

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 11 Dec 2013, Konrad Rzeszutek Wilk wrote:
> I decided to make it a bit simple and only implement the ones we
> check for. Please see the patch below.
> 
> Also, I've trimmed the most of the CC list to not spam the folks --
> naturally if Ian and Stefano think this is the way to go then I will
> repost it.
> 
> >From bec67562eaf9f35aba233e6297c7a90cdc89d64f Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date: Tue, 26 Nov 2013 15:05:40 -0500
> Subject: [PATCH 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up
>  (v2).
> 
> The user has the option of disabling the platform driver:
> 00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01)
> 
> which is used to unplug the emulated drivers (IDE, Realtek 8169, etc)
> and allow the PV drivers to take over. If the user wishes
> to disable that they can set:
> 
>   xen_platform_pci=0
>   (in the guest config file)
> 
> or
>   xen_emul_unplug=never
>   (on the Linux command line)
> 
> except it does not work properly. The PV drivers still try to
> load and since the Xen platform driver is not run - and it
> has not initialized the grant tables, most of the PV drivers
> stumble upon:
> 
> input: Xen Virtual Keyboard as /devices/virtual/input/input5
> input: Xen Virtual Pointer as /devices/virtual/input/input6M
> ------------[ cut here ]------------
> kernel BUG at /home/konrad/ssd/konrad/linux/drivers/xen/grant-table.c:1206!
> invalid opcode: 0000 [#1] SMP
> Modules linked in: xen_kbdfront(+) xenfs xen_privcmd
> CPU: 6 PID: 1389 Comm: modprobe Not tainted 
> 3.13.0-rc1upstream-00021-ga6c892b-dirty #1
> Hardware name: Xen HVM domU, BIOS 4.4-unstable 11/26/2013
> RIP: 0010:[<ffffffff813ddc40>]  [<ffffffff813ddc40>] 
> get_free_entries+0x2e0/0x300
> Call Trace:
>  [<ffffffff8150d9a3>] ? evdev_connect+0x1e3/0x240
>  [<ffffffff813ddd0e>] gnttab_grant_foreign_access+0x2e/0x70
>  [<ffffffffa0010081>] xenkbd_connect_backend+0x41/0x290 [xen_kbdfront]
>  [<ffffffffa0010a12>] xenkbd_probe+0x2f2/0x324 [xen_kbdfront]
>  [<ffffffff813e5757>] xenbus_dev_probe+0x77/0x130
>  [<ffffffff813e7217>] xenbus_frontend_dev_probe+0x47/0x50
>  [<ffffffff8145e9a9>] driver_probe_device+0x89/0x230
>  [<ffffffff8145ebeb>] __driver_attach+0x9b/0xa0
>  [<ffffffff8145eb50>] ? driver_probe_device+0x230/0x230
>  [<ffffffff8145eb50>] ? driver_probe_device+0x230/0x230
>  [<ffffffff8145cf1c>] bus_for_each_dev+0x8c/0xb0
>  [<ffffffff8145e7d9>] driver_attach+0x19/0x20
>  [<ffffffff8145e260>] bus_add_driver+0x1a0/0x220
>  [<ffffffff8145f1ff>] driver_register+0x5f/0xf0
>  [<ffffffff813e55c5>] xenbus_register_driver_common+0x15/0x20
>  [<ffffffff813e76b3>] xenbus_register_frontend+0x23/0x40
>  [<ffffffffa0015000>] ? 0xffffffffa0014fff
>  [<ffffffffa001502b>] xenkbd_init+0x2b/0x1000 [xen_kbdfront]
>  [<ffffffff81002049>] do_one_initcall+0x49/0x170
> 
> .. snip..
> 
> which is hardly nice. This patch fixes this by having each
> PV driver check for:
>  - if running in PV, then it is fine to execute (as that is their
>    native environment).
>  - if running in HVM, check if user wanted 'xen_emul_unplug=never',
>    in which case bail out and don't load any PV drivers.
>  - if running in HVM, and if PCI device 5853:0001 (xen_platform_pci)
>    does not exist, then bail out and not load PV drivers.
>  - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=disks',
>    then bail out for all PV devices _except_ the block one.
>    Ditto for the network one ('nics').
>  - (v2) if running in HVM, and if the user wanted 
> 'xen_emul_unplug=unnecessary'
>    then load block PV driver, and also setup the legacy IDE paths.
> 
> Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx
> Reported-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Reported-by: Fabio Fantoni <fabio.fantoni@xxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> [v2: Add extra logic to handle the myrid ways 'xen_emul_unplug'
> can be used per Ian and Stefano suggestion]

I think that the patch is good, the main issue is that it is missing the
ARM and ARM64 implementations of xen_has_pv_devices & co.



>  arch/x86/xen/platform-pci-unplug.c         |   69 
> ++++++++++++++++++++++++++++
>  drivers/block/xen-blkfront.c               |    4 +-
>  drivers/char/tpm/xen-tpmfront.c            |    4 ++
>  drivers/input/misc/xen-kbdfront.c          |    4 ++
>  drivers/net/xen-netfront.c                 |    2 +-
>  drivers/pci/xen-pcifront.c                 |    4 ++
>  drivers/video/xen-fbfront.c                |    4 ++
>  drivers/xen/xenbus/xenbus_probe_frontend.c |    2 +-
>  include/xen/platform_pci.h                 |   23 +++++++++
>  9 files changed, 112 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/xen/platform-pci-unplug.c 
> b/arch/x86/xen/platform-pci-unplug.c
> index 0a78524..2fb9088 100644
> --- a/arch/x86/xen/platform-pci-unplug.c
> +++ b/arch/x86/xen/platform-pci-unplug.c
> @@ -69,6 +69,75 @@ static int check_platform_magic(void)
>       return 0;
>  }
>  
> +bool xen_has_pv_devices()
> +{
> +     if (!xen_domain())
> +             return false;
> +
> +     /* PV domains always have them. */
> +     if (xen_pv_domain())
> +             return true;
> +
> +     /* And user has xen_platform_pci=0 set in guest config as
> +      * driver did not modify the value. */
> +     if (xen_platform_pci_unplug == 0)
> +             return false;
> +
> +     if (xen_platform_pci_unplug & XEN_UNPLUG_NEVER)
> +             return false;
> +
> +     if (xen_platform_pci_unplug & XEN_UNPLUG_ALL)
> +             return true;
> +
> +     /* And the calleer has to follow with xen_pv_{disk,nic}_devices
> +      * to be certain which driver can load. */
> +     return false;
> +}
> +EXPORT_SYMBOL_GPL(xen_has_pv_devices);
> +
> +bool __xen_has_pv_device(int state)
> +{
> +     /* HVM domains might or might not */
> +     if (xen_hvm_domain() && (xen_platform_pci_unplug & state))
> +             return true;
> +
> +     return xen_has_pv_devices();
> +}

__xen_has_pv_device should be static


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel