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

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

On Wed, Nov 27, 2013 at 09:36:55AM +0000, Ian Campbell wrote:
> On Tue, 2013-11-26 at 15:08 -0500, Konrad Rzeszutek Wilk wrote:
> > 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())
> 
> I think !xen_has_pv_devices() or some such would be a better name.

<nods>
> > 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)
> 
>          ^ stray space, but I think you wanted an inline here anyway?

Yup.
> 
> Or you could move this to arch/x86/xen/platform-pci-unplug.c and then
> xen_platform_pci_unplug could be unexported, which seems like a good
> thing to do if the logic to using it is as complex as below.

I was thinking about it - but then there is a bit of a problem with
!CONFIG_PVHVM && CONFIG_XEN_BLKFRONT for example. Which means that
platform-pci-unplug.c won't be built, but the xen-blkfront will and
it needs the xen_has_pv_devices()). Hence sticking it in a header.

> 
> > +{
> >  
> > +	if (!xen_domain())
> > +		return true;
> > +
> > +	if (xen_hvm_domain()) {
> > +		if (xen_platform_pci_unplug & (XEN_UNPLUG_UNNECESSARY | XEN_UNPLUG_NEVER))
> > +			return true;
> > +		if (xen_platform_pci_unplug == 0)
> > +			return true;
> 
> There are some cases where xen_unplug_emulated_devices doesn't update
> xen_platform_pci_unplug, specifically when xen_emul_unplug contains
> UNNECESSARY or NEVER. I think the above logic covers that case but it
> might be simpler to always set it? At that point on the first if is
> necessary?

<nods>

Thanks for your review!
> 
> > +	}
> > +	return false;
> > +}
> >  #endif /* _XEN_PLATFORM_PCI_H */
> 
>