* [PATCH v3] if xen_platform_pci=0 is set don't blow up.
@ 2013-12-16 15:04 Konrad Rzeszutek Wilk
2013-12-16 15:04 ` [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3) Konrad Rzeszutek Wilk
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-12-16 15:04 UTC (permalink / raw)
To: axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel,
boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe,
mail, tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen,
tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev
The first patch:
[PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up
I would like to commit to stable as it is fixing an eggregious bug -
where we blow up if the guest config has: "xen_platform_pci=0" setup.
This bug has been in existence for years and it is time to stamp it out.
The second patch is a cleanup - not a stable candidate.
It touches all of the Xen frontend drivers and adds the logic of:
"if user disabled us, don't init" - with variations. As you can
specify exactly which ones you want to init and which ones
not (Linux runtime parameter 'xen_emul_unplug'). But for the majority
of drivers - it is just an on/off switch.
Since it touches a lot of maintainers I figured I would send it
to Linus on Wednesday or Thursday.
Thank you!
arch/x86/xen/platform-pci-unplug.c | 79 ++++++++++++++++++++++++++++--
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 | 25 +++++++++-
9 files changed, 119 insertions(+), 9 deletions(-)
Konrad Rzeszutek Wilk (2):
xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3).
xen/pvhvm: Remove the xen_platform_pci int.
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). 2013-12-16 15:04 [PATCH v3] if xen_platform_pci=0 is set don't blow up Konrad Rzeszutek Wilk @ 2013-12-16 15:04 ` Konrad Rzeszutek Wilk 2013-12-17 9:54 ` [Xen-devel] " Fabio Fantoni 2013-12-16 15:04 ` [PATCH v3 2/2] xen/pvhvm: Remove the xen_platform_pci int Konrad Rzeszutek Wilk 2013-12-17 1:00 ` [PATCH v3] if xen_platform_pci=0 is set don't blow up Bjorn Helgaas 2 siblings, 1 reply; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-12-16 15:04 UTC (permalink / raw) To: axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel, boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe, mail, tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen, tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev Cc: Konrad Rzeszutek Wilk 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. In (v3) make it actually load PV drivers. Reported-by: Sander Eikelenboom <linux@eikelenboom.it Reported-by: Anthony PERARD <anthony.perard@citrix.com> Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [v2: Add extra logic to handle the myrid ways 'xen_emul_unplug' can be used per Ian and Stefano suggestion] [v3: Make the unnecessary case work properly] Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- arch/x86/xen/platform-pci-unplug.c | 74 ++++++++++++++++++++++++++++++ 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, 117 insertions(+), 4 deletions(-) diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c index 0a78524..ab84ac1 100644 --- a/arch/x86/xen/platform-pci-unplug.c +++ b/arch/x86/xen/platform-pci-unplug.c @@ -69,6 +69,80 @@ 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; + + /* This is an odd one - we are going to run legacy + * and PV drivers at the same time. */ + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) + return true; + + /* And the caller 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); + +static 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(); +} + +bool xen_has_pv_nic_devices(void) +{ + return __xen_has_pv_device(XEN_UNPLUG_ALL_NICS | XEN_UNPLUG_ALL); +} +EXPORT_SYMBOL_GPL(xen_has_pv_nic_devices); + +bool xen_has_pv_disk_devices(void) +{ + return __xen_has_pv_device(XEN_UNPLUG_ALL_IDE_DISKS | + XEN_UNPLUG_AUX_IDE_DISKS | XEN_UNPLUG_ALL); +} +EXPORT_SYMBOL_GPL(xen_has_pv_disk_devices); + +/* + * This one is odd - it determines whether you want to run PV _and_ + * legacy (IDE) drivers together. This combination is only possible + * under HVM. + */ +bool xen_has_pv_and_legacy_disk_devices(void) +{ + if (!xen_domain()) + return false; + + /* N.B. This is only ever used in HVM mode */ + if (xen_pv_domain()) + return false; + + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) + return true; + + return false; +} +EXPORT_SYMBOL_GPL(xen_has_pv_and_legacy_disk_devices); + void xen_unplug_emulated_devices(void) { int r; diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index a4660bb..ed88b3c 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -1278,7 +1278,7 @@ static int blkfront_probe(struct xenbus_device *dev, char *type; int len; /* no unplug has been done: do not hook devices != xen vbds */ - if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) { + if (xen_has_pv_and_legacy_disk_devices()) { int major; if (!VDEV_IS_EXTENDED(vdevice)) @@ -2022,7 +2022,7 @@ static int __init xlblk_init(void) if (!xen_domain()) return -ENODEV; - if (xen_hvm_domain() && !xen_platform_pci_unplug) + if (!xen_has_pv_disk_devices()) return -ENODEV; if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) { diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c index 06189e5..9c2cbd1 100644 --- a/drivers/char/tpm/xen-tpmfront.c +++ b/drivers/char/tpm/xen-tpmfront.c @@ -16,6 +16,7 @@ #include <xen/xenbus.h> #include <xen/page.h> #include "tpm.h" +#include <xen/platform_pci.h> struct tpm_private { struct tpm_chip *chip; @@ -422,6 +423,9 @@ static int __init xen_tpmfront_init(void) if (!xen_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + return xenbus_register_frontend(&tpmfront_driver); } module_init(xen_tpmfront_init); diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c index e21c181..fbfdc10 100644 --- a/drivers/input/misc/xen-kbdfront.c +++ b/drivers/input/misc/xen-kbdfront.c @@ -29,6 +29,7 @@ #include <xen/interface/io/fbif.h> #include <xen/interface/io/kbdif.h> #include <xen/xenbus.h> +#include <xen/platform_pci.h> struct xenkbd_info { struct input_dev *kbd; @@ -380,6 +381,9 @@ static int __init xenkbd_init(void) if (xen_initial_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + return xenbus_register_frontend(&xenkbd_driver); } diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 36808bf..eea2392 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -2106,7 +2106,7 @@ static int __init netif_init(void) if (!xen_domain()) return -ENODEV; - if (xen_hvm_domain() && !xen_platform_pci_unplug) + if (!xen_has_pv_nic_devices()) return -ENODEV; pr_info("Initialising Xen virtual ethernet driver\n"); diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index f7197a7..eae7cd9 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -20,6 +20,7 @@ #include <linux/workqueue.h> #include <linux/bitops.h> #include <linux/time.h> +#include <xen/platform_pci.h> #include <asm/xen/swiotlb-xen.h> #define INVALID_GRANT_REF (0) @@ -1138,6 +1139,9 @@ static int __init pcifront_init(void) if (!xen_pv_domain() || xen_initial_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + pci_frontend_registrar(1 /* enable */); return xenbus_register_frontend(&xenpci_driver); diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c index cd005c2..4b2d3ab 100644 --- a/drivers/video/xen-fbfront.c +++ b/drivers/video/xen-fbfront.c @@ -35,6 +35,7 @@ #include <xen/interface/io/fbif.h> #include <xen/interface/io/protocols.h> #include <xen/xenbus.h> +#include <xen/platform_pci.h> struct xenfb_info { unsigned char *fb; @@ -699,6 +700,9 @@ static int __init xenfb_init(void) if (xen_initial_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + return xenbus_register_frontend(&xenfb_driver); } diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c index 34b20bf..6244f9c 100644 --- a/drivers/xen/xenbus/xenbus_probe_frontend.c +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c @@ -496,7 +496,7 @@ subsys_initcall(xenbus_probe_frontend_init); #ifndef MODULE static int __init boot_wait_for_devices(void) { - if (xen_hvm_domain() && !xen_platform_pci_unplug) + if (!xen_has_pv_devices()) return -ENODEV; ready_to_wait_for_devices = 1; diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h index 438c256..b49eeab 100644 --- a/include/xen/platform_pci.h +++ b/include/xen/platform_pci.h @@ -48,4 +48,27 @@ static inline int xen_must_unplug_disks(void) { extern int xen_platform_pci_unplug; +#if defined(CONFIG_XEN_PVHVM) +extern bool xen_has_pv_devices(void); +extern bool xen_has_pv_disk_devices(void); +extern bool xen_has_pv_nic_devices(void); +extern bool xen_has_pv_and_legacy_disk_devices(void); +#else +static inline bool xen_has_pv_devices(void) +{ + return IS_ENABLED(CONFIG_XEN); +} +static inline bool xen_has_pv_disk_devices(void) +{ + return IS_ENABLED(CONFIG_XEN); +} +static inline bool xen_has_pv_nic_devices(void) +{ + return IS_ENABLED(CONFIG_XEN); +} +static inline bool xen_has_pv_and_legacy_disk_devices(void) +{ + return false; +} +#endif #endif /* _XEN_PLATFORM_PCI_H */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). 2013-12-16 15:04 ` [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3) Konrad Rzeszutek Wilk @ 2013-12-17 9:54 ` Fabio Fantoni 2013-12-17 14:51 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 10+ messages in thread From: Fabio Fantoni @ 2013-12-17 9:54 UTC (permalink / raw) To: Konrad Rzeszutek Wilk, axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel, boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe, mail, tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen, tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev Il 16/12/2013 16:04, Konrad Rzeszutek Wilk ha scritto: > 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. > In (v3) make it actually load PV drivers. > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it > Reported-by: Anthony PERARD <anthony.perard@citrix.com> > Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > [v2: Add extra logic to handle the myrid ways 'xen_emul_unplug' > can be used per Ian and Stefano suggestion] > [v3: Make the unnecessary case work properly] > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> I tested this patch with all possible cases that I know, no crash or calltrace found. I don't understand the utility of 'xen_emul_unplug=unnecessary' but probably it is working correctly, it shows both pv and not pv for blocks and nics. About 'xen_emul_unplug=disks' and 'xen_emul_unplug=nics' probably there is something wrong. With 'xen_emul_unplug=nics' it shows pv nic (this should be correct) and about disks it shows the same disk twice, as pv and not pv (xvda and sda) with different device number. With 'xen_emul_unplug=disks' it shows pv block (this should be correct) and it seems to show the pv nic too (this should be wrong). Thanks for any reply. > --- > arch/x86/xen/platform-pci-unplug.c | 74 ++++++++++++++++++++++++++++++ > 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, 117 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c > index 0a78524..ab84ac1 100644 > --- a/arch/x86/xen/platform-pci-unplug.c > +++ b/arch/x86/xen/platform-pci-unplug.c > @@ -69,6 +69,80 @@ 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; > + > + /* This is an odd one - we are going to run legacy > + * and PV drivers at the same time. */ > + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) > + return true; > + > + /* And the caller 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); > + > +static 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(); > +} > + > +bool xen_has_pv_nic_devices(void) > +{ > + return __xen_has_pv_device(XEN_UNPLUG_ALL_NICS | XEN_UNPLUG_ALL); > +} > +EXPORT_SYMBOL_GPL(xen_has_pv_nic_devices); > + > +bool xen_has_pv_disk_devices(void) > +{ > + return __xen_has_pv_device(XEN_UNPLUG_ALL_IDE_DISKS | > + XEN_UNPLUG_AUX_IDE_DISKS | XEN_UNPLUG_ALL); > +} > +EXPORT_SYMBOL_GPL(xen_has_pv_disk_devices); > + > +/* > + * This one is odd - it determines whether you want to run PV _and_ > + * legacy (IDE) drivers together. This combination is only possible > + * under HVM. > + */ > +bool xen_has_pv_and_legacy_disk_devices(void) > +{ > + if (!xen_domain()) > + return false; > + > + /* N.B. This is only ever used in HVM mode */ > + if (xen_pv_domain()) > + return false; > + > + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) > + return true; > + > + return false; > +} > +EXPORT_SYMBOL_GPL(xen_has_pv_and_legacy_disk_devices); > + > void xen_unplug_emulated_devices(void) > { > int r; > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index a4660bb..ed88b3c 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -1278,7 +1278,7 @@ static int blkfront_probe(struct xenbus_device *dev, > char *type; > int len; > /* no unplug has been done: do not hook devices != xen vbds */ > - if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) { > + if (xen_has_pv_and_legacy_disk_devices()) { > int major; > > if (!VDEV_IS_EXTENDED(vdevice)) > @@ -2022,7 +2022,7 @@ static int __init xlblk_init(void) > if (!xen_domain()) > return -ENODEV; > > - if (xen_hvm_domain() && !xen_platform_pci_unplug) > + if (!xen_has_pv_disk_devices()) > return -ENODEV; > > if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) { > diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c > index 06189e5..9c2cbd1 100644 > --- a/drivers/char/tpm/xen-tpmfront.c > +++ b/drivers/char/tpm/xen-tpmfront.c > @@ -16,6 +16,7 @@ > #include <xen/xenbus.h> > #include <xen/page.h> > #include "tpm.h" > +#include <xen/platform_pci.h> > > struct tpm_private { > struct tpm_chip *chip; > @@ -422,6 +423,9 @@ static int __init xen_tpmfront_init(void) > if (!xen_domain()) > return -ENODEV; > > + if (!xen_has_pv_devices()) > + return -ENODEV; > + > return xenbus_register_frontend(&tpmfront_driver); > } > module_init(xen_tpmfront_init); > diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c > index e21c181..fbfdc10 100644 > --- a/drivers/input/misc/xen-kbdfront.c > +++ b/drivers/input/misc/xen-kbdfront.c > @@ -29,6 +29,7 @@ > #include <xen/interface/io/fbif.h> > #include <xen/interface/io/kbdif.h> > #include <xen/xenbus.h> > +#include <xen/platform_pci.h> > > struct xenkbd_info { > struct input_dev *kbd; > @@ -380,6 +381,9 @@ static int __init xenkbd_init(void) > if (xen_initial_domain()) > return -ENODEV; > > + if (!xen_has_pv_devices()) > + return -ENODEV; > + > return xenbus_register_frontend(&xenkbd_driver); > } > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index 36808bf..eea2392 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -2106,7 +2106,7 @@ static int __init netif_init(void) > if (!xen_domain()) > return -ENODEV; > > - if (xen_hvm_domain() && !xen_platform_pci_unplug) > + if (!xen_has_pv_nic_devices()) > return -ENODEV; > > pr_info("Initialising Xen virtual ethernet driver\n"); > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > index f7197a7..eae7cd9 100644 > --- a/drivers/pci/xen-pcifront.c > +++ b/drivers/pci/xen-pcifront.c > @@ -20,6 +20,7 @@ > #include <linux/workqueue.h> > #include <linux/bitops.h> > #include <linux/time.h> > +#include <xen/platform_pci.h> > > #include <asm/xen/swiotlb-xen.h> > #define INVALID_GRANT_REF (0) > @@ -1138,6 +1139,9 @@ static int __init pcifront_init(void) > if (!xen_pv_domain() || xen_initial_domain()) > return -ENODEV; > > + if (!xen_has_pv_devices()) > + return -ENODEV; > + > pci_frontend_registrar(1 /* enable */); > > return xenbus_register_frontend(&xenpci_driver); > diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c > index cd005c2..4b2d3ab 100644 > --- a/drivers/video/xen-fbfront.c > +++ b/drivers/video/xen-fbfront.c > @@ -35,6 +35,7 @@ > #include <xen/interface/io/fbif.h> > #include <xen/interface/io/protocols.h> > #include <xen/xenbus.h> > +#include <xen/platform_pci.h> > > struct xenfb_info { > unsigned char *fb; > @@ -699,6 +700,9 @@ static int __init xenfb_init(void) > if (xen_initial_domain()) > return -ENODEV; > > + if (!xen_has_pv_devices()) > + return -ENODEV; > + > return xenbus_register_frontend(&xenfb_driver); > } > > diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c > index 34b20bf..6244f9c 100644 > --- a/drivers/xen/xenbus/xenbus_probe_frontend.c > +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c > @@ -496,7 +496,7 @@ subsys_initcall(xenbus_probe_frontend_init); > #ifndef MODULE > static int __init boot_wait_for_devices(void) > { > - if (xen_hvm_domain() && !xen_platform_pci_unplug) > + if (!xen_has_pv_devices()) > return -ENODEV; > > ready_to_wait_for_devices = 1; > diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h > index 438c256..b49eeab 100644 > --- a/include/xen/platform_pci.h > +++ b/include/xen/platform_pci.h > @@ -48,4 +48,27 @@ static inline int xen_must_unplug_disks(void) { > > extern int xen_platform_pci_unplug; > > +#if defined(CONFIG_XEN_PVHVM) > +extern bool xen_has_pv_devices(void); > +extern bool xen_has_pv_disk_devices(void); > +extern bool xen_has_pv_nic_devices(void); > +extern bool xen_has_pv_and_legacy_disk_devices(void); > +#else > +static inline bool xen_has_pv_devices(void) > +{ > + return IS_ENABLED(CONFIG_XEN); > +} > +static inline bool xen_has_pv_disk_devices(void) > +{ > + return IS_ENABLED(CONFIG_XEN); > +} > +static inline bool xen_has_pv_nic_devices(void) > +{ > + return IS_ENABLED(CONFIG_XEN); > +} > +static inline bool xen_has_pv_and_legacy_disk_devices(void) > +{ > + return false; > +} > +#endif > #endif /* _XEN_PLATFORM_PCI_H */ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). 2013-12-17 9:54 ` [Xen-devel] " Fabio Fantoni @ 2013-12-17 14:51 ` Konrad Rzeszutek Wilk 2013-12-17 21:23 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-12-17 14:51 UTC (permalink / raw) To: Fabio Fantoni Cc: axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel, boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe, mail, tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen, tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev On Tue, Dec 17, 2013 at 10:54:47AM +0100, Fabio Fantoni wrote: > Il 16/12/2013 16:04, Konrad Rzeszutek Wilk ha scritto: > >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. > > In (v3) make it actually load PV drivers. > > > >Reported-by: Sander Eikelenboom <linux@eikelenboom.it > >Reported-by: Anthony PERARD <anthony.perard@citrix.com> > >Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz> > >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >[v2: Add extra logic to handle the myrid ways 'xen_emul_unplug' > >can be used per Ian and Stefano suggestion] > >[v3: Make the unnecessary case work properly] > >Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > I tested this patch with all possible cases that I know, no crash or > calltrace found. > > I don't understand the utility of 'xen_emul_unplug=unnecessary' but > probably it is working correctly, it shows both pv and not pv for > blocks and nics. Great! > > About 'xen_emul_unplug=disks' and 'xen_emul_unplug=nics' probably > there is something wrong. > With 'xen_emul_unplug=nics' it shows pv nic (this should be correct) > and about disks it shows the same disk twice, as pv and not pv (xvda > and sda) with different device number. Good. > With 'xen_emul_unplug=disks' it shows pv block (this should be > correct) and it seems to show the pv nic too (this should be wrong). Correct. You should see only the PV disk and all other PV devices should not function. Let me dig in this and see whether there is another bug. Thank you! > > Thanks for any reply. > > >--- > > arch/x86/xen/platform-pci-unplug.c | 74 ++++++++++++++++++++++++++++++ > > 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, 117 insertions(+), 4 deletions(-) > > > >diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c > >index 0a78524..ab84ac1 100644 > >--- a/arch/x86/xen/platform-pci-unplug.c > >+++ b/arch/x86/xen/platform-pci-unplug.c > >@@ -69,6 +69,80 @@ 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; > >+ > >+ /* This is an odd one - we are going to run legacy > >+ * and PV drivers at the same time. */ > >+ if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) > >+ return true; > >+ > >+ /* And the caller 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); > >+ > >+static 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(); > >+} > >+ > >+bool xen_has_pv_nic_devices(void) > >+{ > >+ return __xen_has_pv_device(XEN_UNPLUG_ALL_NICS | XEN_UNPLUG_ALL); > >+} > >+EXPORT_SYMBOL_GPL(xen_has_pv_nic_devices); > >+ > >+bool xen_has_pv_disk_devices(void) > >+{ > >+ return __xen_has_pv_device(XEN_UNPLUG_ALL_IDE_DISKS | > >+ XEN_UNPLUG_AUX_IDE_DISKS | XEN_UNPLUG_ALL); > >+} > >+EXPORT_SYMBOL_GPL(xen_has_pv_disk_devices); > >+ > >+/* > >+ * This one is odd - it determines whether you want to run PV _and_ > >+ * legacy (IDE) drivers together. This combination is only possible > >+ * under HVM. > >+ */ > >+bool xen_has_pv_and_legacy_disk_devices(void) > >+{ > >+ if (!xen_domain()) > >+ return false; > >+ > >+ /* N.B. This is only ever used in HVM mode */ > >+ if (xen_pv_domain()) > >+ return false; > >+ > >+ if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) > >+ return true; > >+ > >+ return false; > >+} > >+EXPORT_SYMBOL_GPL(xen_has_pv_and_legacy_disk_devices); > >+ > > void xen_unplug_emulated_devices(void) > > { > > int r; > >diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > >index a4660bb..ed88b3c 100644 > >--- a/drivers/block/xen-blkfront.c > >+++ b/drivers/block/xen-blkfront.c > >@@ -1278,7 +1278,7 @@ static int blkfront_probe(struct xenbus_device *dev, > > char *type; > > int len; > > /* no unplug has been done: do not hook devices != xen vbds */ > >- if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) { > >+ if (xen_has_pv_and_legacy_disk_devices()) { > > int major; > > if (!VDEV_IS_EXTENDED(vdevice)) > >@@ -2022,7 +2022,7 @@ static int __init xlblk_init(void) > > if (!xen_domain()) > > return -ENODEV; > >- if (xen_hvm_domain() && !xen_platform_pci_unplug) > >+ if (!xen_has_pv_disk_devices()) > > return -ENODEV; > > if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) { > >diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c > >index 06189e5..9c2cbd1 100644 > >--- a/drivers/char/tpm/xen-tpmfront.c > >+++ b/drivers/char/tpm/xen-tpmfront.c > >@@ -16,6 +16,7 @@ > > #include <xen/xenbus.h> > > #include <xen/page.h> > > #include "tpm.h" > >+#include <xen/platform_pci.h> > > struct tpm_private { > > struct tpm_chip *chip; > >@@ -422,6 +423,9 @@ static int __init xen_tpmfront_init(void) > > if (!xen_domain()) > > return -ENODEV; > >+ if (!xen_has_pv_devices()) > >+ return -ENODEV; > >+ > > return xenbus_register_frontend(&tpmfront_driver); > > } > > module_init(xen_tpmfront_init); > >diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c > >index e21c181..fbfdc10 100644 > >--- a/drivers/input/misc/xen-kbdfront.c > >+++ b/drivers/input/misc/xen-kbdfront.c > >@@ -29,6 +29,7 @@ > > #include <xen/interface/io/fbif.h> > > #include <xen/interface/io/kbdif.h> > > #include <xen/xenbus.h> > >+#include <xen/platform_pci.h> > > struct xenkbd_info { > > struct input_dev *kbd; > >@@ -380,6 +381,9 @@ static int __init xenkbd_init(void) > > if (xen_initial_domain()) > > return -ENODEV; > >+ if (!xen_has_pv_devices()) > >+ return -ENODEV; > >+ > > return xenbus_register_frontend(&xenkbd_driver); > > } > >diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > >index 36808bf..eea2392 100644 > >--- a/drivers/net/xen-netfront.c > >+++ b/drivers/net/xen-netfront.c > >@@ -2106,7 +2106,7 @@ static int __init netif_init(void) > > if (!xen_domain()) > > return -ENODEV; > >- if (xen_hvm_domain() && !xen_platform_pci_unplug) > >+ if (!xen_has_pv_nic_devices()) > > return -ENODEV; > > pr_info("Initialising Xen virtual ethernet driver\n"); > >diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > >index f7197a7..eae7cd9 100644 > >--- a/drivers/pci/xen-pcifront.c > >+++ b/drivers/pci/xen-pcifront.c > >@@ -20,6 +20,7 @@ > > #include <linux/workqueue.h> > > #include <linux/bitops.h> > > #include <linux/time.h> > >+#include <xen/platform_pci.h> > > #include <asm/xen/swiotlb-xen.h> > > #define INVALID_GRANT_REF (0) > >@@ -1138,6 +1139,9 @@ static int __init pcifront_init(void) > > if (!xen_pv_domain() || xen_initial_domain()) > > return -ENODEV; > >+ if (!xen_has_pv_devices()) > >+ return -ENODEV; > >+ > > pci_frontend_registrar(1 /* enable */); > > return xenbus_register_frontend(&xenpci_driver); > >diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c > >index cd005c2..4b2d3ab 100644 > >--- a/drivers/video/xen-fbfront.c > >+++ b/drivers/video/xen-fbfront.c > >@@ -35,6 +35,7 @@ > > #include <xen/interface/io/fbif.h> > > #include <xen/interface/io/protocols.h> > > #include <xen/xenbus.h> > >+#include <xen/platform_pci.h> > > struct xenfb_info { > > unsigned char *fb; > >@@ -699,6 +700,9 @@ static int __init xenfb_init(void) > > if (xen_initial_domain()) > > return -ENODEV; > >+ if (!xen_has_pv_devices()) > >+ return -ENODEV; > >+ > > return xenbus_register_frontend(&xenfb_driver); > > } > >diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c > >index 34b20bf..6244f9c 100644 > >--- a/drivers/xen/xenbus/xenbus_probe_frontend.c > >+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c > >@@ -496,7 +496,7 @@ subsys_initcall(xenbus_probe_frontend_init); > > #ifndef MODULE > > static int __init boot_wait_for_devices(void) > > { > >- if (xen_hvm_domain() && !xen_platform_pci_unplug) > >+ if (!xen_has_pv_devices()) > > return -ENODEV; > > ready_to_wait_for_devices = 1; > >diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h > >index 438c256..b49eeab 100644 > >--- a/include/xen/platform_pci.h > >+++ b/include/xen/platform_pci.h > >@@ -48,4 +48,27 @@ static inline int xen_must_unplug_disks(void) { > > extern int xen_platform_pci_unplug; > >+#if defined(CONFIG_XEN_PVHVM) > >+extern bool xen_has_pv_devices(void); > >+extern bool xen_has_pv_disk_devices(void); > >+extern bool xen_has_pv_nic_devices(void); > >+extern bool xen_has_pv_and_legacy_disk_devices(void); > >+#else > >+static inline bool xen_has_pv_devices(void) > >+{ > >+ return IS_ENABLED(CONFIG_XEN); > >+} > >+static inline bool xen_has_pv_disk_devices(void) > >+{ > >+ return IS_ENABLED(CONFIG_XEN); > >+} > >+static inline bool xen_has_pv_nic_devices(void) > >+{ > >+ return IS_ENABLED(CONFIG_XEN); > >+} > >+static inline bool xen_has_pv_and_legacy_disk_devices(void) > >+{ > >+ return false; > >+} > >+#endif > > #endif /* _XEN_PLATFORM_PCI_H */ > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). 2013-12-17 14:51 ` Konrad Rzeszutek Wilk @ 2013-12-17 21:23 ` Konrad Rzeszutek Wilk 2013-12-31 14:32 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-12-17 21:23 UTC (permalink / raw) To: Fabio Fantoni Cc: axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel, boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe, mail, tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen, tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev On Tue, Dec 17, 2013 at 09:51:50AM -0500, Konrad Rzeszutek Wilk wrote: > On Tue, Dec 17, 2013 at 10:54:47AM +0100, Fabio Fantoni wrote: > > Il 16/12/2013 16:04, Konrad Rzeszutek Wilk ha scritto: > > >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. > > > In (v3) make it actually load PV drivers. > > > > > >Reported-by: Sander Eikelenboom <linux@eikelenboom.it > > >Reported-by: Anthony PERARD <anthony.perard@citrix.com> > > >Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz> > > >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > >[v2: Add extra logic to handle the myrid ways 'xen_emul_unplug' > > >can be used per Ian and Stefano suggestion] > > >[v3: Make the unnecessary case work properly] > > >Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > I tested this patch with all possible cases that I know, no crash or > > calltrace found. > > > > I don't understand the utility of 'xen_emul_unplug=unnecessary' but > > probably it is working correctly, it shows both pv and not pv for > > blocks and nics. > > Great! > > > > About 'xen_emul_unplug=disks' and 'xen_emul_unplug=nics' probably > > there is something wrong. > > With 'xen_emul_unplug=nics' it shows pv nic (this should be correct) > > and about disks it shows the same disk twice, as pv and not pv (xvda > > and sda) with different device number. > > Good. > > With 'xen_emul_unplug=disks' it shows pv block (this should be > > correct) and it seems to show the pv nic too (this should be wrong). > > Correct. You should see only the PV disk and all other PV devices > should not function. Let me dig in this and see whether there is > another bug. Thank you! That is because 'disks' is incorrect. It should have been 'ide-disks' [ 0.000000] unrecognised option 'disks' in parameter 'xen_emul_unplug' With the 'ide-disks' it should work. I will update the description to mention 'ide-disks' instead of 'disks'. Thank you for finding this! ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). 2013-12-17 21:23 ` Konrad Rzeszutek Wilk @ 2013-12-31 14:32 ` Konrad Rzeszutek Wilk 2014-01-02 15:09 ` David Vrabel 0 siblings, 1 reply; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-12-31 14:32 UTC (permalink / raw) To: Fabio Fantoni Cc: axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel, boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe, mail, tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen, tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev > That is because 'disks' is incorrect. It should have been 'ide-disks' > > [ 0.000000] unrecognised option 'disks' in parameter 'xen_emul_unplug' > > With the 'ide-disks' it should work. I will update the description to > mention 'ide-disks' instead of 'disks'. Thank you for finding this! > I've v4 with said update and will push it to Linus shortly. Thanks! P.S. Here is v4: >From 275a81e7496d3532e5b4752703c50a7c8355a6c7 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Date: Tue, 26 Nov 2013 15:05:40 -0500 Subject: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v4). 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=ide-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. In (v3) make it actually load PV drivers. Reported-by: Sander Eikelenboom <linux@eikelenboom.it Reported-by: Anthony PERARD <anthony.perard@citrix.com> Reported-and-Tested-by: Fabio Fantoni <fabio.fantoni@m2r.biz> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [v2: Add extra logic to handle the myrid ways 'xen_emul_unplug' can be used per Ian and Stefano suggestion] [v3: Make the unnecessary case work properly] [v4: s/disks/ide-disks/ spotted by Fabio] Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> [for PCI parts] --- arch/x86/xen/platform-pci-unplug.c | 74 ++++++++++++++++++++++++++++++ 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, 117 insertions(+), 4 deletions(-) diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c index 0a78524..ab84ac1 100644 --- a/arch/x86/xen/platform-pci-unplug.c +++ b/arch/x86/xen/platform-pci-unplug.c @@ -69,6 +69,80 @@ 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; + + /* This is an odd one - we are going to run legacy + * and PV drivers at the same time. */ + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) + return true; + + /* And the caller 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); + +static 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(); +} + +bool xen_has_pv_nic_devices(void) +{ + return __xen_has_pv_device(XEN_UNPLUG_ALL_NICS | XEN_UNPLUG_ALL); +} +EXPORT_SYMBOL_GPL(xen_has_pv_nic_devices); + +bool xen_has_pv_disk_devices(void) +{ + return __xen_has_pv_device(XEN_UNPLUG_ALL_IDE_DISKS | + XEN_UNPLUG_AUX_IDE_DISKS | XEN_UNPLUG_ALL); +} +EXPORT_SYMBOL_GPL(xen_has_pv_disk_devices); + +/* + * This one is odd - it determines whether you want to run PV _and_ + * legacy (IDE) drivers together. This combination is only possible + * under HVM. + */ +bool xen_has_pv_and_legacy_disk_devices(void) +{ + if (!xen_domain()) + return false; + + /* N.B. This is only ever used in HVM mode */ + if (xen_pv_domain()) + return false; + + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) + return true; + + return false; +} +EXPORT_SYMBOL_GPL(xen_has_pv_and_legacy_disk_devices); + void xen_unplug_emulated_devices(void) { int r; diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index a4660bb..ed88b3c 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -1278,7 +1278,7 @@ static int blkfront_probe(struct xenbus_device *dev, char *type; int len; /* no unplug has been done: do not hook devices != xen vbds */ - if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) { + if (xen_has_pv_and_legacy_disk_devices()) { int major; if (!VDEV_IS_EXTENDED(vdevice)) @@ -2022,7 +2022,7 @@ static int __init xlblk_init(void) if (!xen_domain()) return -ENODEV; - if (xen_hvm_domain() && !xen_platform_pci_unplug) + if (!xen_has_pv_disk_devices()) return -ENODEV; if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) { diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c index 06189e5..9c2cbd1 100644 --- a/drivers/char/tpm/xen-tpmfront.c +++ b/drivers/char/tpm/xen-tpmfront.c @@ -16,6 +16,7 @@ #include <xen/xenbus.h> #include <xen/page.h> #include "tpm.h" +#include <xen/platform_pci.h> struct tpm_private { struct tpm_chip *chip; @@ -422,6 +423,9 @@ static int __init xen_tpmfront_init(void) if (!xen_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + return xenbus_register_frontend(&tpmfront_driver); } module_init(xen_tpmfront_init); diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c index e21c181..fbfdc10 100644 --- a/drivers/input/misc/xen-kbdfront.c +++ b/drivers/input/misc/xen-kbdfront.c @@ -29,6 +29,7 @@ #include <xen/interface/io/fbif.h> #include <xen/interface/io/kbdif.h> #include <xen/xenbus.h> +#include <xen/platform_pci.h> struct xenkbd_info { struct input_dev *kbd; @@ -380,6 +381,9 @@ static int __init xenkbd_init(void) if (xen_initial_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + return xenbus_register_frontend(&xenkbd_driver); } diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 36808bf..eea2392 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -2106,7 +2106,7 @@ static int __init netif_init(void) if (!xen_domain()) return -ENODEV; - if (xen_hvm_domain() && !xen_platform_pci_unplug) + if (!xen_has_pv_nic_devices()) return -ENODEV; pr_info("Initialising Xen virtual ethernet driver\n"); diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index f7197a7..eae7cd9 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -20,6 +20,7 @@ #include <linux/workqueue.h> #include <linux/bitops.h> #include <linux/time.h> +#include <xen/platform_pci.h> #include <asm/xen/swiotlb-xen.h> #define INVALID_GRANT_REF (0) @@ -1138,6 +1139,9 @@ static int __init pcifront_init(void) if (!xen_pv_domain() || xen_initial_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + pci_frontend_registrar(1 /* enable */); return xenbus_register_frontend(&xenpci_driver); diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c index cd005c2..4b2d3ab 100644 --- a/drivers/video/xen-fbfront.c +++ b/drivers/video/xen-fbfront.c @@ -35,6 +35,7 @@ #include <xen/interface/io/fbif.h> #include <xen/interface/io/protocols.h> #include <xen/xenbus.h> +#include <xen/platform_pci.h> struct xenfb_info { unsigned char *fb; @@ -699,6 +700,9 @@ static int __init xenfb_init(void) if (xen_initial_domain()) return -ENODEV; + if (!xen_has_pv_devices()) + return -ENODEV; + return xenbus_register_frontend(&xenfb_driver); } diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c index 34b20bf..6244f9c 100644 --- a/drivers/xen/xenbus/xenbus_probe_frontend.c +++ b/drivers/xen/xenbus/xenbus_probe_frontend.c @@ -496,7 +496,7 @@ subsys_initcall(xenbus_probe_frontend_init); #ifndef MODULE static int __init boot_wait_for_devices(void) { - if (xen_hvm_domain() && !xen_platform_pci_unplug) + if (!xen_has_pv_devices()) return -ENODEV; ready_to_wait_for_devices = 1; diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h index 438c256..b49eeab 100644 --- a/include/xen/platform_pci.h +++ b/include/xen/platform_pci.h @@ -48,4 +48,27 @@ static inline int xen_must_unplug_disks(void) { extern int xen_platform_pci_unplug; +#if defined(CONFIG_XEN_PVHVM) +extern bool xen_has_pv_devices(void); +extern bool xen_has_pv_disk_devices(void); +extern bool xen_has_pv_nic_devices(void); +extern bool xen_has_pv_and_legacy_disk_devices(void); +#else +static inline bool xen_has_pv_devices(void) +{ + return IS_ENABLED(CONFIG_XEN); +} +static inline bool xen_has_pv_disk_devices(void) +{ + return IS_ENABLED(CONFIG_XEN); +} +static inline bool xen_has_pv_nic_devices(void) +{ + return IS_ENABLED(CONFIG_XEN); +} +static inline bool xen_has_pv_and_legacy_disk_devices(void) +{ + return false; +} +#endif #endif /* _XEN_PLATFORM_PCI_H */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). 2013-12-31 14:32 ` Konrad Rzeszutek Wilk @ 2014-01-02 15:09 ` David Vrabel 2014-01-02 19:16 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 10+ messages in thread From: David Vrabel @ 2014-01-02 15:09 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Fabio Fantoni, axboe, leosilva, linux-fbdev, dmitry.torokhov, ian.campbell, stefano.stabellini, linux-pci, netdev, ashley, tpmdd, linux-kernel, mail, tpmdd-devel, tomi.valkeinen, david.vrabel, linux-input, bhelgaas, xen-devel, boris.ostrovsky, plagnioj, peterhuewe On 31/12/13 14:32, Konrad Rzeszutek Wilk wrote: >> That is because 'disks' is incorrect. It should have been 'ide-disks' >> >> [ 0.000000] unrecognised option 'disks' in parameter 'xen_emul_unplug' >> >> With the 'ide-disks' it should work. I will update the description to >> mention 'ide-disks' instead of 'disks'. Thank you for finding this! >> > > I've v4 with said update and will push it to Linus shortly. > > Thanks! > > P.S. > Here is v4: > >>From 275a81e7496d3532e5b4752703c50a7c8355a6c7 Mon Sep 17 00:00:00 2001 > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Date: Tue, 26 Nov 2013 15:05:40 -0500 > Subject: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v4). > > 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=ide-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. > In (v3) make it actually load PV drivers. [...] > --- a/arch/x86/xen/platform-pci-unplug.c > +++ b/arch/x86/xen/platform-pci-unplug.c > @@ -69,6 +69,80 @@ 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; > + > + /* This is an odd one - we are going to run legacy > + * and PV drivers at the same time. */ > + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) > + return true; > + > + /* And the caller has to follow with xen_pv_{disk,nic}_devices > + * to be certain which driver can load. */ > + return false; This may result in: xen_has_pv_devices() == false xen_has_pv_disk_devices() == true which looks odd to me. Surely xen_has_pv_*_devices() is a subset of xen_has_pv_devices()? David ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). 2014-01-02 15:09 ` David Vrabel @ 2014-01-02 19:16 ` Konrad Rzeszutek Wilk 0 siblings, 0 replies; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2014-01-02 19:16 UTC (permalink / raw) To: David Vrabel Cc: Fabio Fantoni, axboe, leosilva, linux-fbdev, dmitry.torokhov, ian.campbell, stefano.stabellini, linux-pci, netdev, ashley, tpmdd, linux-kernel, mail, tpmdd-devel, tomi.valkeinen, linux-input, bhelgaas, xen-devel, boris.ostrovsky, plagnioj, peterhuewe On Thu, Jan 02, 2014 at 03:09:44PM +0000, David Vrabel wrote: > On 31/12/13 14:32, Konrad Rzeszutek Wilk wrote: > >> That is because 'disks' is incorrect. It should have been 'ide-disks' > >> > >> [ 0.000000] unrecognised option 'disks' in parameter 'xen_emul_unplug' > >> > >> With the 'ide-disks' it should work. I will update the description to > >> mention 'ide-disks' instead of 'disks'. Thank you for finding this! > >> > > > > I've v4 with said update and will push it to Linus shortly. > > > > Thanks! > > > > P.S. > > Here is v4: > > > >>From 275a81e7496d3532e5b4752703c50a7c8355a6c7 Mon Sep 17 00:00:00 2001 > > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Date: Tue, 26 Nov 2013 15:05:40 -0500 > > Subject: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v4). > > > > 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=ide-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. > > In (v3) make it actually load PV drivers. > [...] > > --- a/arch/x86/xen/platform-pci-unplug.c > > +++ b/arch/x86/xen/platform-pci-unplug.c > > @@ -69,6 +69,80 @@ 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; > > + > > + /* This is an odd one - we are going to run legacy > > + * and PV drivers at the same time. */ > > + if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) > > + return true; > > + > > + /* And the caller has to follow with xen_pv_{disk,nic}_devices > > + * to be certain which driver can load. */ > > + return false; > > This may result in: > > xen_has_pv_devices() == false > xen_has_pv_disk_devices() == true Yes. > > which looks odd to me. Surely xen_has_pv_*_devices() is a subset of > xen_has_pv_devices()? I wish, this thing drives me nuts and I couldn't come up with a sensible way to make this work for those special ones that have their own xen_emul_unplug parameter without special casing the 'xen_has_pv_devices'. Perhaps it should be renamed to 'xen_has_pv_generic_devices' ? > > David ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] xen/pvhvm: Remove the xen_platform_pci int. 2013-12-16 15:04 [PATCH v3] if xen_platform_pci=0 is set don't blow up Konrad Rzeszutek Wilk 2013-12-16 15:04 ` [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3) Konrad Rzeszutek Wilk @ 2013-12-16 15:04 ` Konrad Rzeszutek Wilk 2013-12-17 1:00 ` [PATCH v3] if xen_platform_pci=0 is set don't blow up Bjorn Helgaas 2 siblings, 0 replies; 10+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-12-16 15:04 UTC (permalink / raw) To: axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel, boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe, mail, tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen, tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev Cc: Konrad Rzeszutek Wilk Since we have xen_has_pv_devices,xen_has_pv_disk_devices, xen_has_pv_nic_devices, and xen_has_pv_and_legacy_disk_devices to figure out the different 'unplug' behaviors - lets use those instead of this single int. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- arch/x86/xen/platform-pci-unplug.c | 5 ++--- include/xen/platform_pci.h | 2 -- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c index ab84ac1..a826171 100644 --- a/arch/x86/xen/platform-pci-unplug.c +++ b/arch/x86/xen/platform-pci-unplug.c @@ -30,10 +30,9 @@ #define XEN_PLATFORM_ERR_PROTOCOL -2 #define XEN_PLATFORM_ERR_BLACKLIST -3 -/* store the value of xen_emul_unplug after the unplug is done */ -int xen_platform_pci_unplug; -EXPORT_SYMBOL_GPL(xen_platform_pci_unplug); #ifdef CONFIG_XEN_PVHVM +/* store the value of xen_emul_unplug after the unplug is done */ +static int xen_platform_pci_unplug; static int xen_emul_unplug; static int check_platform_magic(void) diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h index b49eeab..5c52b55 100644 --- a/include/xen/platform_pci.h +++ b/include/xen/platform_pci.h @@ -46,8 +46,6 @@ static inline int xen_must_unplug_disks(void) { #endif } -extern int xen_platform_pci_unplug; - #if defined(CONFIG_XEN_PVHVM) extern bool xen_has_pv_devices(void); extern bool xen_has_pv_disk_devices(void); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] if xen_platform_pci=0 is set don't blow up. 2013-12-16 15:04 [PATCH v3] if xen_platform_pci=0 is set don't blow up Konrad Rzeszutek Wilk 2013-12-16 15:04 ` [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3) Konrad Rzeszutek Wilk 2013-12-16 15:04 ` [PATCH v3 2/2] xen/pvhvm: Remove the xen_platform_pci int Konrad Rzeszutek Wilk @ 2013-12-17 1:00 ` Bjorn Helgaas 2 siblings, 0 replies; 10+ messages in thread From: Bjorn Helgaas @ 2013-12-17 1:00 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Jens Axboe, Stefano Stabellini, ian.campbell, xen-devel, linux-kernel, boris.ostrovsky, David Vrabel, leosilva, ashley, Peter Hüwe, mail, tpmdd, Dmitry Torokhov, plagnioj, tomi.valkeinen, tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev On Mon, Dec 16, 2013 at 8:04 AM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > The first patch: > [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up > > I would like to commit to stable as it is fixing an eggregious bug - > where we blow up if the guest config has: "xen_platform_pci=0" setup. > This bug has been in existence for years and it is time to stamp it out. > > The second patch is a cleanup - not a stable candidate. > > It touches all of the Xen frontend drivers and adds the logic of: > "if user disabled us, don't init" - with variations. As you can > specify exactly which ones you want to init and which ones > not (Linux runtime parameter 'xen_emul_unplug'). But for the majority > of drivers - it is just an on/off switch. > > Since it touches a lot of maintainers I figured I would send it > to Linus on Wednesday or Thursday. > > Thank you! > > arch/x86/xen/platform-pci-unplug.c | 79 ++++++++++++++++++++++++++++-- > 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 | 25 +++++++++- > 9 files changed, 119 insertions(+), 9 deletions(-) If you want it: Acked-by: Bjorn Helgaas <bhelgaas@google.com> for the PCI parts. > > Konrad Rzeszutek Wilk (2): > xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3). > xen/pvhvm: Remove the xen_platform_pci int. > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-01-02 19:16 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-12-16 15:04 [PATCH v3] if xen_platform_pci=0 is set don't blow up Konrad Rzeszutek Wilk 2013-12-16 15:04 ` [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3) Konrad Rzeszutek Wilk 2013-12-17 9:54 ` [Xen-devel] " Fabio Fantoni 2013-12-17 14:51 ` Konrad Rzeszutek Wilk 2013-12-17 21:23 ` Konrad Rzeszutek Wilk 2013-12-31 14:32 ` Konrad Rzeszutek Wilk 2014-01-02 15:09 ` David Vrabel 2014-01-02 19:16 ` Konrad Rzeszutek Wilk 2013-12-16 15:04 ` [PATCH v3 2/2] xen/pvhvm: Remove the xen_platform_pci int Konrad Rzeszutek Wilk 2013-12-17 1:00 ` [PATCH v3] if xen_platform_pci=0 is set don't blow up Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).