Re: [BUG] Xen vm kernel crash in get_free_entries.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Nov 28, 2013 at 02:56:53PM +0000, Stefano Stabellini wrote:
> On Tue, 26 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > On Wed, Nov 13, 2013 at 09:40:58AM +0000, Ian Campbell wrote:
> > > On Tue, 2013-11-12 at 10:56 -0500, Konrad Rzeszutek Wilk wrote:
> > > > On Thu, Nov 07, 2013 at 01:47:03PM +0000, Ian Campbell wrote:
> > > > > On Thu, 2013-11-07 at 09:20 +0400, Astarta wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > Let me bring some new life to this discussion.
> > > > > > 
> > > > > > I've investigated a bit and found another way to make  kernels starting 
> > > > > > from 3.8.x to boot on the VMs with platform device_id 0002.
> > > > > > Reverting of xen-grant-table-correctly-initialize-grant-table-version-1 
> > > > > > patch is not necessary.
> > > > > > 
> > > > > > We can simply modify struct pci_device_id platform_pci_tbl[] (in 
> > > > > > drivers/xen/platform-pci.c) to respect 0002 and 0000 device ids.
> > > > > > That makes the kernel (3.8.x and 3.11.6) to boot correctly, disks and 
> > > > > > network are also recognized.
> > > > > 
> > > > > I think this is just working around the problem, by avoiding the
> > > > > situation where the error occurs. You could just as well switch to
> > > > > platform device id < 2.
> > > > 
> > > > I am bit late to this discussion - but shouldn't there be something
> > > > in the kernel to deal with this?
> > > 
> > > Well, ideally the kernel wouldn't crash ;-)
> > 
> > This patch should solve that (untested, but at least compile tested):
> > Please test it.
> > 
> > >From f7d3581aa19a35ea3ff10b965b2b08843e923635 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.
> > 
> > *TODO*
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/block/xen-blkfront.c               |  2 +-
> >  drivers/input/misc/xen-kbdfront.c          |  4 ++++
> >  drivers/net/xen-netfront.c                 |  2 +-
> >  drivers/xen/xenbus/xenbus_probe_frontend.c |  2 +-
> >  include/xen/platform_pci.h                 | 13 +++++++++++++
> >  5 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> > index 432db1b..bcbaf0b 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_err_out())
> >  		return -ENODEV;
> >  
> >  	if (register_blkdev(XENVBD_MAJOR, DEV_NAME)) {
> > diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
> > index e21c181..9d250cf 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_err_out())
> > +		return -ENODEV;
> 
> Why do we error out here? There is nothing to unplug in this case.

B/c otherwise the driver blows up when it acts on the XenBus and tries
to setup a grant. But the grant is initialized by the xen-platform-pci
which is not run.
> 
> 
> >  	return xenbus_register_frontend(&xenkbd_driver);
> >  }
> >  
> > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> > index e59acb1..be2744b 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_err_out())
> >  		return -ENODEV;
> >  
> >  	pr_info("Initialising Xen virtual ethernet driver\n");
> > diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
> > index 129bf84..b1c0f2a 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_err_out())
> >  		return -ENODEV;
> >  
> >  	ready_to_wait_for_devices = 1;
> > diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
> > index 438c256..a5bbd0b 100644
> > --- a/include/xen/platform_pci.h
> > +++ b/include/xen/platform_pci.h
> > @@ -47,5 +47,18 @@ static inline int xen_must_unplug_disks(void) {
> >  }
> >  
> >  extern int xen_platform_pci_unplug;
> > +static  bool xen_err_out(void)
> > +{
> 
> static inline
> 
> 
> > +	if (!xen_domain())
> > +		return true;
> > +
> > +	if (xen_hvm_domain()) {
> > +		if (xen_platform_pci_unplug & (XEN_UNPLUG_UNNECESSARY | XEN_UNPLUG_NEVER))
> 
> This check wasn't present before and I don't think should be present
> now: xen_platform_pci_unplug cannot be set to XEN_UNPLUG_NEVER, see the
> beginning of xen_unplug_emulated_devices. Also XEN_UNPLUG_UNNECESSARY is
> meant to say that the user knows what she is doing and no unplug is
> necessary to safely use PV drivers, see the commit message of
> 1dc7ce99b091a11cce0f34456c1ffcb928f17edd.
> 
> 
> > +			return true;
> > +		if (xen_platform_pci_unplug == 0)
> > +			return true;
> > +	}
> > +	return false;
> > +}
> >  #endif /* _XEN_PLATFORM_PCI_H */
>