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

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

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
>