Konrad Rzeszutek Wilk
2013-Dec-03 21:14 UTC
[PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
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 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.
P.S.
Ian Campbell suggested getting rid of
''xen_platform_pci_unplug''
but unfortunatly the xen-blkfront driver is using it, so we
cannot do it.
Reported-by: Sander Eikelenboom <linux@eikelenboom.it
Reported-by: Anthony PERARD <anthony.perard@citrix.com>
Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
---
arch/x86/xen/platform-pci-unplug.c | 18 ++++++++++++++++++
drivers/block/xen-blkfront.c | 2 +-
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 | 13 ++++++++++++-
9 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/arch/x86/xen/platform-pci-unplug.c
b/arch/x86/xen/platform-pci-unplug.c
index 0a78524..087dfeb 100644
--- a/arch/x86/xen/platform-pci-unplug.c
+++ b/arch/x86/xen/platform-pci-unplug.c
@@ -69,6 +69,24 @@ static int check_platform_magic(void)
return 0;
}
+bool xen_has_pv_devices(void)
+{
+ if (!xen_domain())
+ return false;
+
+ if (xen_hvm_domain()) {
+ /* User requested no unplug, so no PV drivers. */
+ if (xen_emul_unplug & XEN_UNPLUG_NEVER)
+ return false;
+ /* And user has xen_platform_pci=0 set in guest config as
+ * driver did not modify the value. */
+ if (!xen_platform_pci_unplug)
+ return false;
+ }
+ return true;
+}
+EXPORT_SYMBOL_GPL(xen_has_pv_devices);
+
void xen_unplug_emulated_devices(void)
{
int r;
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 432db1b..9616b81 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2074,7 +2074,7 @@ static int __init xlblk_init(void)
if (!xen_domain())
return -ENODEV;
- if (xen_hvm_domain() && !xen_platform_pci_unplug)
+ if (!xen_has_pv_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 c8ff4df..62e7d38 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -17,6 +17,7 @@
#include <xen/xenbus.h>
#include <xen/page.h>
#include "tpm.h"
+#include <xen/platform_pci.h>
struct tpm_private {
struct tpm_chip *chip;
@@ -421,6 +422,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 e59acb1..d4b52e9 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2115,7 +2115,7 @@ static int __init netif_init(void)
if (!xen_domain())
return -ENODEV;
- if (xen_hvm_domain() && !xen_platform_pci_unplug)
+ if (!xen_has_pv_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 129bf84..cb385c1 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..87bc59c 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -47,5 +47,16 @@ 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);
+#else
+static inline bool xen_has_pv_devices(void)
+{
+#if defined(CONFIG_XEN)
+ return true;
+#else
+ return false;
+#endif
+}
+#endif
#endif /* _XEN_PLATFORM_PCI_H */
--
1.8.3.1
Matthew Daley
2013-Dec-04 00:03 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, Dec 4, 2013 at 10:14 AM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:> 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 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. > > P.S. > Ian Campbell suggested getting rid of ''xen_platform_pci_unplug'' > but unfortunatly the xen-blkfront driver is using it, so we > cannot do it. > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it > Reported-by: Anthony PERARD <anthony.perard@citrix.com> > Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz>I think you forgot your Signed-off-by line. - Matthew
Ian Campbell
2013-Dec-04 09:37 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Tue, 2013-12-03 at 16:14 -0500, Konrad Rzeszutek Wilk wrote:> 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 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. > > P.S. > Ian Campbell suggested getting rid of ''xen_platform_pci_unplug'' > but unfortunatly the xen-blkfront driver is using it, so we > cannot do it.It might still be nice to expose a suitable semantic interface (i.e. some relevant predicate) rather than the raw value for blkfront to use. But that can be a future thing I think.> Reported-by: Sander Eikelenboom <linux@eikelenboom.it > Reported-by: Anthony PERARD <anthony.perard@citrix.com> > Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz> > --- > arch/x86/xen/platform-pci-unplug.c | 18 ++++++++++++++++++ > drivers/block/xen-blkfront.c | 2 +- > 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 | 13 ++++++++++++- > 9 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c > index 0a78524..087dfeb 100644 > --- a/arch/x86/xen/platform-pci-unplug.c > +++ b/arch/x86/xen/platform-pci-unplug.c > @@ -69,6 +69,24 @@ static int check_platform_magic(void) > return 0; > } > > +bool xen_has_pv_devices(void) > +{ > + if (!xen_domain()) > + return false; > + > + if (xen_hvm_domain()) { > + /* User requested no unplug, so no PV drivers. */ > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > + return false;I think you need if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) return true; don''t you?> + /* And user has xen_platform_pci=0 set in guest config as > + * driver did not modify the value. */ > + if (!xen_platform_pci_unplug) > + return false; > + } > + return true; > +} > +EXPORT_SYMBOL_GPL(xen_has_pv_devices); > + > void xen_unplug_emulated_devices(void) > { > int r;
Stefano Stabellini
2013-Dec-04 10:48 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Tue, 3 Dec 2013, Konrad Rzeszutek Wilk wrote:> diff --git a/arch/x86/xen/platform-pci-unplug.c b/arch/x86/xen/platform-pci-unplug.c > index 0a78524..087dfeb 100644 > --- a/arch/x86/xen/platform-pci-unplug.c > +++ b/arch/x86/xen/platform-pci-unplug.c > @@ -69,6 +69,24 @@ static int check_platform_magic(void) > return 0; > } > > +bool xen_has_pv_devices(void) > +{ > + if (!xen_domain()) > + return false; > + > + if (xen_hvm_domain()) { > + /* User requested no unplug, so no PV drivers. */ > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > + return false;Considering that if (xen_emul_unplug & XEN_UNPLUG_NEVER) we never set xen_platform_pci_unplug, this check is redundant.> + /* And user has xen_platform_pci=0 set in guest config as > + * driver did not modify the value. */ > + if (!xen_platform_pci_unplug) > + return false; > + } > + return true; > +} > +EXPORT_SYMBOL_GPL(xen_has_pv_devices); > + > void xen_unplug_emulated_devices(void) > { > int r;
Stefano Stabellini
2013-Dec-04 10:51 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 4 Dec 2013, Ian Campbell wrote:> > +bool xen_has_pv_devices(void) > > +{ > > + if (!xen_domain()) > > + return false; > > + > > + if (xen_hvm_domain()) { > > + /* User requested no unplug, so no PV drivers. */ > > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > > + return false; > > I think you need > if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) > return true; > don''t you?XEN_UNPLUG_UNNECESSARY was introduced to enable the platform PCI device even if it didn''t respond properly to the unplug protocol. The corresponding parameter is called "unnecessary" because if you pass it to the kernel you mean that it is unnecessary to unplug the emulated devices but you can use the pv devices anyway. So no, we shouldn''t check for XEN_UNPLUG_UNNECESSARY here.> > + /* And user has xen_platform_pci=0 set in guest config as > > + * driver did not modify the value. */ > > + if (!xen_platform_pci_unplug) > > + return false; > > + } > > + return true; > > +} > > +EXPORT_SYMBOL_GPL(xen_has_pv_devices); > > + > > void xen_unplug_emulated_devices(void) > > { > > int r; > >
Ian Campbell
2013-Dec-04 10:59 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 2013-12-04 at 10:51 +0000, Stefano Stabellini wrote:> On Wed, 4 Dec 2013, Ian Campbell wrote: > > > +bool xen_has_pv_devices(void) > > > +{ > > > + if (!xen_domain()) > > > + return false; > > > + > > > + if (xen_hvm_domain()) { > > > + /* User requested no unplug, so no PV drivers. */ > > > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > > > + return false; > > > > I think you need > > if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) > > return true; > > don''t you? > > XEN_UNPLUG_UNNECESSARY was introduced to enable the platform PCI device > even if it didn''t respond properly to the unplug protocol. > The corresponding parameter is called "unnecessary" because if you pass > it to the kernel you mean that it is unnecessary to unplug the emulated > devices but you can use the pv devices anyway. > > So no, we shouldn''t check for XEN_UNPLUG_UNNECESSARY here.Oh, we will eventually fall through to the return true, so it does actually work out OK. I''d still be in favour of handling each option explicitly, for clarity. Which means checking for XEN_UNPLUG_UNNECESSARY.> > > + /* And user has xen_platform_pci=0 set in guest config as > > > + * driver did not modify the value. */ > > > + if (!xen_platform_pci_unplug) > > > + return false;I assume this check doesn''t trigger if unnecessary has been specified?> > > + } > > > + return true; > > > +} > > > +EXPORT_SYMBOL_GPL(xen_has_pv_devices); > > > + > > > void xen_unplug_emulated_devices(void) > > > { > > > int r; > > > >
Stefano Stabellini
2013-Dec-04 11:05 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 4 Dec 2013, Ian Campbell wrote:> On Wed, 2013-12-04 at 10:51 +0000, Stefano Stabellini wrote: > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > > +bool xen_has_pv_devices(void) > > > > +{ > > > > + if (!xen_domain()) > > > > + return false; > > > > + > > > > + if (xen_hvm_domain()) { > > > > + /* User requested no unplug, so no PV drivers. */ > > > > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > > > > + return false; > > > > > > I think you need > > > if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) > > > return true; > > > don''t you? > > > > XEN_UNPLUG_UNNECESSARY was introduced to enable the platform PCI device > > even if it didn''t respond properly to the unplug protocol. > > The corresponding parameter is called "unnecessary" because if you pass > > it to the kernel you mean that it is unnecessary to unplug the emulated > > devices but you can use the pv devices anyway. > > > > So no, we shouldn''t check for XEN_UNPLUG_UNNECESSARY here. > > Oh, we will eventually fall through to the return true, so it does > actually work out OK. > > I''d still be in favour of handling each option explicitly, for clarity. > Which means checking for XEN_UNPLUG_UNNECESSARY.I think is wrong to check for any xen_emul_unpug options in this function. The xen_emul_unpug options should be used to set the right value of xen_platform_pci_unplug. (See my other reply.)> > > > + /* And user has xen_platform_pci=0 set in guest config as > > > > + * driver did not modify the value. */ > > > > + if (!xen_platform_pci_unplug) > > > > + return false; > > I assume this check doesn''t trigger if unnecessary has been specified?right
Ian Campbell
2013-Dec-04 11:08 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 2013-12-04 at 11:05 +0000, Stefano Stabellini wrote:> On Wed, 4 Dec 2013, Ian Campbell wrote: > > On Wed, 2013-12-04 at 10:51 +0000, Stefano Stabellini wrote: > > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > > > +bool xen_has_pv_devices(void) > > > > > +{ > > > > > + if (!xen_domain()) > > > > > + return false; > > > > > + > > > > > + if (xen_hvm_domain()) { > > > > > + /* User requested no unplug, so no PV drivers. */ > > > > > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > > > > > + return false; > > > > > > > > I think you need > > > > if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) > > > > return true; > > > > don''t you? > > > > > > XEN_UNPLUG_UNNECESSARY was introduced to enable the platform PCI device > > > even if it didn''t respond properly to the unplug protocol. > > > The corresponding parameter is called "unnecessary" because if you pass > > > it to the kernel you mean that it is unnecessary to unplug the emulated > > > devices but you can use the pv devices anyway. > > > > > > So no, we shouldn''t check for XEN_UNPLUG_UNNECESSARY here. > > > > Oh, we will eventually fall through to the return true, so it does > > actually work out OK. > > > > I''d still be in favour of handling each option explicitly, for clarity. > > Which means checking for XEN_UNPLUG_UNNECESSARY. > > I think is wrong to check for any xen_emul_unpug options in this function. > The xen_emul_unpug options should be used to set the right value of > xen_platform_pci_unplug. (See my other reply.)Whichever one we check we should still be checking explicitly for the "unnecessary" case, for clarity if nothing else. TBH I think the split between xen_emul_unplug and xen_platform_pci_unplug is a bit artificial. There should be one value which is static to platform-pci-unplug.c and accessor functions should be provided for other code to use. Open coding all those accesses to xen_platform_pci_unplug in every driver is just too error prone. Ian.
Stefano Stabellini
2013-Dec-04 11:18 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 4 Dec 2013, Ian Campbell wrote:> On Wed, 2013-12-04 at 11:05 +0000, Stefano Stabellini wrote: > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > On Wed, 2013-12-04 at 10:51 +0000, Stefano Stabellini wrote: > > > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > > > > +bool xen_has_pv_devices(void) > > > > > > +{ > > > > > > + if (!xen_domain()) > > > > > > + return false; > > > > > > + > > > > > > + if (xen_hvm_domain()) { > > > > > > + /* User requested no unplug, so no PV drivers. */ > > > > > > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > > > > > > + return false; > > > > > > > > > > I think you need > > > > > if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) > > > > > return true; > > > > > don''t you? > > > > > > > > XEN_UNPLUG_UNNECESSARY was introduced to enable the platform PCI device > > > > even if it didn''t respond properly to the unplug protocol. > > > > The corresponding parameter is called "unnecessary" because if you pass > > > > it to the kernel you mean that it is unnecessary to unplug the emulated > > > > devices but you can use the pv devices anyway. > > > > > > > > So no, we shouldn''t check for XEN_UNPLUG_UNNECESSARY here. > > > > > > Oh, we will eventually fall through to the return true, so it does > > > actually work out OK. > > > > > > I''d still be in favour of handling each option explicitly, for clarity. > > > Which means checking for XEN_UNPLUG_UNNECESSARY. > > > > I think is wrong to check for any xen_emul_unpug options in this function. > > The xen_emul_unpug options should be used to set the right value of > > xen_platform_pci_unplug. (See my other reply.) > > Whichever one we check we should still be checking explicitly for the > "unnecessary" case, for clarity if nothing else.Sure, that is OK for me. In that case should we check for the full list of possible options? ide-disks -- unplug primary master IDE devices aux-ide-disks -- unplug non-primary-master IDE devices nics -- unplug network devices all -- unplug all emulated devices (NICs and IDE disks) unnecessary -- unplugging emulated devices is unnecessary even if the host did not respond to the unplug protocol never -- do not unplug even if version check succeeds
Ian Campbell
2013-Dec-04 11:23 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 2013-12-04 at 11:18 +0000, Stefano Stabellini wrote:> On Wed, 4 Dec 2013, Ian Campbell wrote: > > On Wed, 2013-12-04 at 11:05 +0000, Stefano Stabellini wrote: > > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > > On Wed, 2013-12-04 at 10:51 +0000, Stefano Stabellini wrote: > > > > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > > > > > +bool xen_has_pv_devices(void) > > > > > > > +{ > > > > > > > + if (!xen_domain()) > > > > > > > + return false; > > > > > > > + > > > > > > > + if (xen_hvm_domain()) { > > > > > > > + /* User requested no unplug, so no PV drivers. */ > > > > > > > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > > > > > > > + return false; > > > > > > > > > > > > I think you need > > > > > > if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) > > > > > > return true; > > > > > > don''t you? > > > > > > > > > > XEN_UNPLUG_UNNECESSARY was introduced to enable the platform PCI device > > > > > even if it didn''t respond properly to the unplug protocol. > > > > > The corresponding parameter is called "unnecessary" because if you pass > > > > > it to the kernel you mean that it is unnecessary to unplug the emulated > > > > > devices but you can use the pv devices anyway. > > > > > > > > > > So no, we shouldn''t check for XEN_UNPLUG_UNNECESSARY here. > > > > > > > > Oh, we will eventually fall through to the return true, so it does > > > > actually work out OK. > > > > > > > > I''d still be in favour of handling each option explicitly, for clarity. > > > > Which means checking for XEN_UNPLUG_UNNECESSARY. > > > > > > I think is wrong to check for any xen_emul_unpug options in this function. > > > The xen_emul_unpug options should be used to set the right value of > > > xen_platform_pci_unplug. (See my other reply.) > > > > Whichever one we check we should still be checking explicitly for the > > "unnecessary" case, for clarity if nothing else. > > Sure, that is OK for me. > In that case should we check for the full list of possible options?We probably should. That probably means an extra xen_has_pv_{disk,nic}_devices() which is the existing one plus the specific checks?> > ide-disks -- unplug primary master IDE devices > aux-ide-disks -- unplug non-primary-master IDE devices > nics -- unplug network devices > all -- unplug all emulated devices (NICs and IDE disks) > unnecessary -- unplugging emulated devices is > unnecessary even if the host did not respond to > the unplug protocol > never -- do not unplug even if version check succeeds
David Vrabel
2013-Dec-04 13:00 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On 03/12/13 21:14, Konrad Rzeszutek Wilk wrote:> > Ian Campbell suggested getting rid of ''xen_platform_pci_unplug'' > but unfortunatly the xen-blkfront driver is using it, so we > cannot do it.I had a look at what blkfront was using this for and it seems dumb. How did we end up with the frontend driver working around toolstack bugs? If HVM Linux guest didn''t want (e.g.,) PV CDROM, the toolstack shouldn''t have created one. And perhaps more importantly, if you actually want a PV CDROM in a HVM guest, it''s not possible. David
Ian Campbell
2013-Dec-04 13:06 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 2013-12-04 at 13:00 +0000, David Vrabel wrote:> On 03/12/13 21:14, Konrad Rzeszutek Wilk wrote: > > > > Ian Campbell suggested getting rid of ''xen_platform_pci_unplug'' > > but unfortunatly the xen-blkfront driver is using it, so we > > cannot do it. > > I had a look at what blkfront was using this for and it seems dumb. How > did we end up with the frontend driver working around toolstack bugs? > If HVM Linux guest didn''t want (e.g.,) PV CDROM, the toolstack shouldn''t > have created one.Note that this cdrom stuff is actually nothing to do with the unplug variable -- it just happens to be right next to that check. Not that the use of the unplug var there doesn''t also look pretty odd...
Dmitry Torokhov
2013-Dec-04 16:42 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
Hi Konrad, On Tue, Dec 03, 2013 at 04:14:06PM -0500, Konrad Rzeszutek Wilk wrote:> 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 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. > > P.S. > Ian Campbell suggested getting rid of ''xen_platform_pci_unplug'' > but unfortunatly the xen-blkfront driver is using it, so we > cannot do it. > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it > Reported-by: Anthony PERARD <anthony.perard@citrix.com> > Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz>For input: Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> ...> +#if defined(CONFIG_XEN_PVHVM) > +extern bool xen_has_pv_devices(void); > +#else > +static inline bool xen_has_pv_devices(void) > +{ > +#if defined(CONFIG_XEN) > + return true; > +#else > + return false; > +#endifreturn IS_ENABLED(CONFIG_XEN); ? Thanks. -- Dmitry
Konrad Rzeszutek Wilk
2013-Dec-04 16:48 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
> > 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 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. > > > > P.S. > > Ian Campbell suggested getting rid of ''xen_platform_pci_unplug'' > > but unfortunatly the xen-blkfront driver is using it, so we > > cannot do it. > > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it > > Reported-by: Anthony PERARD <anthony.perard@citrix.com> > > Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz> > > For input: > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>Thank you. I need to do some extra changes to the other subsystems but for the fb/kbd itshould be still throught the same function as this patch has exposed. I will repost it and include your Ack if you are OK with that?> > ... > > > +#if defined(CONFIG_XEN_PVHVM) > > +extern bool xen_has_pv_devices(void); > > +#else > > +static inline bool xen_has_pv_devices(void) > > +{ > > +#if defined(CONFIG_XEN) > > + return true; > > +#else > > + return false; > > +#endif > > return IS_ENABLED(CONFIG_XEN); > > ?Oh, awesome! Thanks!
Dmitry Torokhov
2013-Dec-04 17:08 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, Dec 04, 2013 at 11:48:03AM -0500, Konrad Rzeszutek Wilk wrote:> > > 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 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. > > > > > > P.S. > > > Ian Campbell suggested getting rid of ''xen_platform_pci_unplug'' > > > but unfortunatly the xen-blkfront driver is using it, so we > > > cannot do it. > > > > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it > > > Reported-by: Anthony PERARD <anthony.perard@citrix.com> > > > Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz> > > > > For input: > > > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Thank you. I need to do some extra changes to the other subsystems but > for the fb/kbd itshould be still throught the same function as this > patch has exposed. I will repost it and include your Ack if you are OK > with that?Sure. -- Dmitry
Konrad Rzeszutek Wilk
2013-Dec-11 20:26 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, Dec 04, 2013 at 11:23:40AM +0000, Ian Campbell wrote:> On Wed, 2013-12-04 at 11:18 +0000, Stefano Stabellini wrote: > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > On Wed, 2013-12-04 at 11:05 +0000, Stefano Stabellini wrote: > > > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > > > On Wed, 2013-12-04 at 10:51 +0000, Stefano Stabellini wrote: > > > > > > On Wed, 4 Dec 2013, Ian Campbell wrote: > > > > > > > > +bool xen_has_pv_devices(void) > > > > > > > > +{ > > > > > > > > + if (!xen_domain()) > > > > > > > > + return false; > > > > > > > > + > > > > > > > > + if (xen_hvm_domain()) { > > > > > > > > + /* User requested no unplug, so no PV drivers. */ > > > > > > > > + if (xen_emul_unplug & XEN_UNPLUG_NEVER) > > > > > > > > + return false; > > > > > > > > > > > > > > I think you need > > > > > > > if (xen_emul_unpug & XEN_UNPLUG_UNNECESSARY) > > > > > > > return true; > > > > > > > don''t you? > > > > > > > > > > > > XEN_UNPLUG_UNNECESSARY was introduced to enable the platform PCI device > > > > > > even if it didn''t respond properly to the unplug protocol. > > > > > > The corresponding parameter is called "unnecessary" because if you pass > > > > > > it to the kernel you mean that it is unnecessary to unplug the emulated > > > > > > devices but you can use the pv devices anyway. > > > > > > > > > > > > So no, we shouldn''t check for XEN_UNPLUG_UNNECESSARY here. > > > > > > > > > > Oh, we will eventually fall through to the return true, so it does > > > > > actually work out OK. > > > > > > > > > > I''d still be in favour of handling each option explicitly, for clarity. > > > > > Which means checking for XEN_UNPLUG_UNNECESSARY. > > > > > > > > I think is wrong to check for any xen_emul_unpug options in this function. > > > > The xen_emul_unpug options should be used to set the right value of > > > > xen_platform_pci_unplug. (See my other reply.) > > > > > > Whichever one we check we should still be checking explicitly for the > > > "unnecessary" case, for clarity if nothing else. > > > > Sure, that is OK for me. > > In that case should we check for the full list of possible options? > > We probably should. That probably means an extra > xen_has_pv_{disk,nic}_devices() which is the existing one plus the > specific checks?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@oracle.com> 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@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] --- 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(); +} + +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 94c280d..afa9362 100644 --- a/drivers/char/tpm/xen-tpmfront.c +++ b/drivers/char/tpm/xen-tpmfront.c @@ -17,6 +17,7 @@ #include <xen/xenbus.h> #include <xen/page.h> #include "tpm.h" +#include <xen/platform_pci.h> struct tpm_private { struct tpm_chip *chip; @@ -423,6 +424,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..eea2392c 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.7.7.6
Ian Campbell
2013-Dec-12 10:43 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Wed, 2013-12-11 at 15:26 -0500, Konrad Rzeszutek Wilk wrote:> 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"caller" (or "callee"? probably not)> + * to be certain which driver can load. */In the XEN_UNPLUG_UNNECESSARY case won''t we end up here and return false, when in fact this means we have PV devices which we want to use?> + return false; > +} > +EXPORT_SYMBOL_GPL(xen_has_pv_devices); > + > +bool __xen_has_pv_device(int state)^static?> [...]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;I think with all this stuff you could now make xen_platform_pci_unplug private to the .c file?> +#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);The logic and usage for all these looked right so far as my cold fuddled brain could determine. Ian.
Stefano Stabellini
2013-Dec-12 11:30 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
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@oracle.com> > 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@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]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
Konrad Rzeszutek Wilk
2013-Dec-13 01:29 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Thu, Dec 12, 2013 at 10:43:21AM +0000, Ian Campbell wrote:> On Wed, 2013-12-11 at 15:26 -0500, Konrad Rzeszutek Wilk wrote: > > > 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 > > "caller" (or "callee"? probably not) > > > + * to be certain which driver can load. */ > > In the XEN_UNPLUG_UNNECESSARY case won''t we end up here and return > false, when in fact this means we have PV devices which we want to use?Yes. Thanks for spotting that bug.> > > + return false; > > +} > > +EXPORT_SYMBOL_GPL(xen_has_pv_devices); > > + > > +bool __xen_has_pv_device(int state) > > ^static?Yes :-)> > > [...]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; > > I think with all this stuff you could now make xen_platform_pci_unplug > private to the .c file?Yes, I have another patch that does that.> > > +#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); > > The logic and usage for all these looked right so far as my cold fuddled > brain could determine.Yeeey!> > Ian. >
Konrad Rzeszutek Wilk
2013-Dec-13 01:34 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
> > 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.I think that is OK - you don''t have CONFIG_PVHVM so from the platform_pci.h the ''static inline'' ones get picked up which will return true (except the xen_has_pv_block_and_legacy_devices which will return false) for all of those functions (if compiled with CONFIG_XEN). It should compile _and_ work properly. .. snip..> __xen_has_pv_device should be staticYes. Thanks for spotting that!>
Fabio Fantoni
2013-Dec-13 09:58 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
Il 13/12/2013 02:29, Konrad Rzeszutek Wilk ha scritto:> On Thu, Dec 12, 2013 at 10:43:21AM +0000, Ian Campbell wrote: >> On Wed, 2013-12-11 at 15:26 -0500, Konrad Rzeszutek Wilk wrote: >> >>> 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 >> "caller" (or "callee"? probably not) >> >>> + * to be certain which driver can load. */ >> In the XEN_UNPLUG_UNNECESSARY case won''t we end up here and return >> false, when in fact this means we have PV devices which we want to use? > Yes. Thanks for spotting that bug.Thanks for your patch. I tested the first version of this patch and was working, I should wait a third version before do another test or I should test also the second version? Thanks for any reply.>>> + return false; >>> +} >>> +EXPORT_SYMBOL_GPL(xen_has_pv_devices); >>> + >>> +bool __xen_has_pv_device(int state) >> ^static? > Yes :-) >>> [...]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; >> I think with all this stuff you could now make xen_platform_pci_unplug >> private to the .c file? > Yes, I have another patch that does that. >>> +#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); >> The logic and usage for all these looked right so far as my cold fuddled >> brain could determine. > Yeeey! >> Ian. >> > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk
2013-Dec-13 14:39 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Fri, Dec 13, 2013 at 10:58:11AM +0100, Fabio Fantoni wrote:> Il 13/12/2013 02:29, Konrad Rzeszutek Wilk ha scritto: > >On Thu, Dec 12, 2013 at 10:43:21AM +0000, Ian Campbell wrote: > >>On Wed, 2013-12-11 at 15:26 -0500, Konrad Rzeszutek Wilk wrote: > >> > >>>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 > >>"caller" (or "callee"? probably not) > >> > >>>+ * to be certain which driver can load. */ > >>In the XEN_UNPLUG_UNNECESSARY case won''t we end up here and return > >>false, when in fact this means we have PV devices which we want to use? > >Yes. Thanks for spotting that bug. > > Thanks for your patch. > > I tested the first version of this patch and was working, I should > wait a third version before do another test or I should test also > the second version?Lets wait until I get an Reviewed-by from either Stefano or Ian. Wouldn''t want to spend extra cycles to retest the similar patch over and over.> > Thanks for any reply.Thank you for the question!> > >>>+ return false; > >>>+} > >>>+EXPORT_SYMBOL_GPL(xen_has_pv_devices); > >>>+ > >>>+bool __xen_has_pv_device(int state) > >> ^static? > >Yes :-) > >>>[...]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; > >>I think with all this stuff you could now make xen_platform_pci_unplug > >>private to the .c file? > >Yes, I have another patch that does that. > >>>+#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); > >>The logic and usage for all these looked right so far as my cold fuddled > >>brain could determine. > >Yeeey! > >>Ian. > >> > >_______________________________________________ > >Xen-devel mailing list > >Xen-devel@lists.xen.org > >http://lists.xen.org/xen-devel >
Konrad Rzeszutek Wilk
2013-Dec-13 20:18 UTC
Re: [PATCH] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up.
On Thu, Dec 12, 2013 at 11:30:13AM +0000, Stefano Stabellini wrote:> 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... snip..> > 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] > > 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.I did a compile test and they do look to compile. Please see the following patch: From 0619fbc4dd43944b4c6835d24b01a295c7f205b6 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 1/2] xen/pvhvm: If xen_platform_pci=0 is set don''t blow up (v3). 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] --- 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 c4a4c90..f9c43f9 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -1356,7 +1356,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)) @@ -2079,7 +2079,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 c8ff4df..62e7d38 100644 --- a/drivers/char/tpm/xen-tpmfront.c +++ b/drivers/char/tpm/xen-tpmfront.c @@ -17,6 +17,7 @@ #include <xen/xenbus.h> #include <xen/page.h> #include "tpm.h" +#include <xen/platform_pci.h> struct tpm_private { struct tpm_chip *chip; @@ -421,6 +422,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 e59acb1..2ab82fe 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -2115,7 +2115,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 129bf84..cb385c1 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
Possibly Parallel Threads
- [PATCH] xen: initialize platform_pci even if xen_emul_unplug=never
- xen_emul_unplug on xen 4.1, HVM guest 2.6.38
- linux-3.4-rc0 XENBUS: Device with no driver: device/vbd/51713
- [PATCH 4/5] xen: arm: implement remap interfaces needed for privcmd mappings.
- [RFC 00/14] arm: implement ballooning and privcmd foreign mappings based on x86 PVH