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

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

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.
>