thr3ads.net - Xen devel - [Xen-devel] [GIT/PATCH 0/3] pvhvm emul unplug fixes [Aug 2010]

If this information is useful, please help other people find it:
Share via:

Ian Campbell

2010-Aug-19 09:58 UTC

[Xen-devel] [GIT/PATCH 0/3] pvhvm emul unplug fixes

Jeremy, Stefano,

These patches make it possible to inhibit the unplug on the kernel
command line (mostly "Just In Case") and rename xen_emul_unplug=ignore
to the (IMHO) clearer xen_emul_unplug=unnecessary. The third patch is
just a cleanup to platform_pci.h.

I think the first two should be targeting 2.6.36 since they change
command line arguments. The last one could easily wait until 2.6.37.

If you agree with this and want me to I can take care of sending them
upstream (direct to Linus I guess?).

The following changes since commit da5cabf80e2433131bf0ed8993abc0f7ea618c73:
  Linus Torvalds (1):
        Linux 2.6.36-rc1

are available in the git repository at:

  git://xenbits.xensource.com/people/ianc/linux-2.6.git for-upstream/pvhvm

Ian Campbell (3):
      xen: pvhvm: allow user to request no emulated device unplug
      xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary
      xen: pvhvm: make it clearer that XEN_UNPLUG_* individual bits

 Documentation/kernel-parameters.txt |    6 ++++--
 arch/x86/xen/platform-pci-unplug.c  |   25 +++++++++++++++++--------
 drivers/block/xen-blkfront.c        |    2 +-
 include/xen/platform_pci.h          |   13 ++++++++-----
 4 files changed, 30 insertions(+), 16 deletions(-)



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-19 09:59 UTC

head link

[Xen-devel] [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

this allows the user to disable pvhvm and revert to emulated devices
in case of a system misconfiguration (e.g. initramfs with only
emulated drivers in it).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Documentation/kernel-parameters.txt |    1 +
 arch/x86/xen/platform-pci-unplug.c  |   12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 2c85c06..8bbe83b 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2631,6 +2631,7 @@ and is between 256 and 4096 characters. It is defined in
the file
 			all -- unplug all emulated devices (NICs and IDE disks)
 			ignore -- continue loading the Xen platform PCI driver even
 				if the version check failed
+			never -- do not unplug even if version check succeeds
 
 	xirc2ps_cs=	[NET,PCMCIA]
 			Format:
diff --git a/arch/x86/xen/platform-pci-unplug.c
b/arch/x86/xen/platform-pci-unplug.c
index 554c002..c82ce1e 100644
--- a/arch/x86/xen/platform-pci-unplug.c
+++ b/arch/x86/xen/platform-pci-unplug.c
@@ -33,7 +33,7 @@
 int xen_platform_pci_unplug;
 EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
 #ifdef CONFIG_XEN_PVHVM
-static int xen_emul_unplug;
+static int xen_emul_unplug = -1;
 
 static int __init check_platform_magic(void)
 {
@@ -72,18 +72,23 @@ void __init xen_unplug_emulated_devices(void)
 {
 	int r;
 
+	/* user explicitly requested no unplug */
+	if (xen_emul_unplug == 0)
+		return;
 	/* check the version of the xen platform PCI device */
 	r = check_platform_magic();
 	/* If the version matches enable the Xen platform PCI driver.
 	 * Also enable the Xen platform PCI driver if the version is really old
 	 * and the user told us to ignore it. */
 	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
+			(xen_emul_unplug != -1) &&
 			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
 		return;
 	/* Set the default value of xen_emul_unplug depending on whether or
 	 * not the Xen PV frontends and the Xen platform PCI driver have
 	 * been compiled for this kernel (modules or built-in are both OK). */
-	if (!xen_emul_unplug) {
+	if (xen_emul_unplug == -1) {
+		xen_emul_unplug = 0;
 		if (xen_must_unplug_nics()) {
 			printk(KERN_INFO "Netfront and the Xen platform PCI driver have "
 					"been compiled for this kernel: unplug emulated NICs.\n");
@@ -109,6 +114,7 @@ static int __init parse_xen_emul_unplug(char *arg)
 	char *p, *q;
 	int l;
 
+	xen_emul_unplug = 0;
 	for (p = arg; p; p = q) {
 		q = strchr(p, '','');
 		if (q) {
@@ -127,6 +133,8 @@ static int __init parse_xen_emul_unplug(char *arg)
 			xen_emul_unplug |= XEN_UNPLUG_ALL_NICS;
 		else if (!strncmp(p, "ignore", l))
 			xen_emul_unplug |= XEN_UNPLUG_IGNORE;
+		else if (!strncmp(p, "never", l))
+			/* Nothing to do. xen_emul_unplug = 0 above */;
 		else
 			printk(KERN_WARNING "unrecognised option ''%s'' "
 				 "in parameter ''xen_emul_unplug''\n", p);
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-19 09:59 UTC

head link

[Xen-devel] [PATCH 2/3] xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary

It is not immediately clear what this option causes to become
ignored. The actual meaning is that it is not necessary to unplug the
emulated devices to safely use the PV ones, even if the platform does
not support the unplug protocol. (pressumably the user will only add
this option if they have ensured that their domain configuration is
safe).

I think xen_emul_unplug=unnecessary better captures this.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Documentation/kernel-parameters.txt |    5 +++--
 arch/x86/xen/platform-pci-unplug.c  |   13 +++++++------
 drivers/block/xen-blkfront.c        |    2 +-
 include/xen/platform_pci.h          |    2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 8bbe83b..338f469 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2629,8 +2629,9 @@ and is between 256 and 4096 characters. It is defined in
the file
 			aux-ide-disks -- unplug non-primary-master IDE devices
 			nics -- unplug network devices
 			all -- unplug all emulated devices (NICs and IDE disks)
-			ignore -- continue loading the Xen platform PCI driver even
-				if the version check failed
+			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
 
 	xirc2ps_cs=	[NET,PCMCIA]
diff --git a/arch/x86/xen/platform-pci-unplug.c
b/arch/x86/xen/platform-pci-unplug.c
index c82ce1e..d696e37 100644
--- a/arch/x86/xen/platform-pci-unplug.c
+++ b/arch/x86/xen/platform-pci-unplug.c
@@ -78,11 +78,12 @@ void __init xen_unplug_emulated_devices(void)
 	/* check the version of the xen platform PCI device */
 	r = check_platform_magic();
 	/* If the version matches enable the Xen platform PCI driver.
-	 * Also enable the Xen platform PCI driver if the version is really old
-	 * and the user told us to ignore it. */
+	 * Also enable the Xen platform PCI driver if the host does
+	 * not support the unplug protocol (XEN_PLATFORM_ERR_MAGIC)
+	 * but the user told us that unplugging is unnecessary. */
 	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
 			(xen_emul_unplug != -1) &&
-			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
+			(xen_emul_unplug & XEN_UNPLUG_UNNECESSARY)))
 		return;
 	/* Set the default value of xen_emul_unplug depending on whether or
 	 * not the Xen PV frontends and the Xen platform PCI driver have
@@ -104,7 +105,7 @@ void __init xen_unplug_emulated_devices(void)
 		}
 	}
 	/* Now unplug the emulated devices */
-	if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
+	if (!(xen_emul_unplug & XEN_UNPLUG_UNNECESSARY))
 		outw(xen_emul_unplug, XEN_IOPORT_UNPLUG);
 	xen_platform_pci_unplug = xen_emul_unplug;
 }
@@ -131,8 +132,8 @@ static int __init parse_xen_emul_unplug(char *arg)
 			xen_emul_unplug |= XEN_UNPLUG_AUX_IDE_DISKS;
 		else if (!strncmp(p, "nics", l))
 			xen_emul_unplug |= XEN_UNPLUG_ALL_NICS;
-		else if (!strncmp(p, "ignore", l))
-			xen_emul_unplug |= XEN_UNPLUG_IGNORE;
+		else if (!strncmp(p, "unnecessary", l))
+			xen_emul_unplug |= XEN_UNPLUG_UNNECESSARY;
 		else if (!strncmp(p, "never", l))
 			/* Nothing to do. xen_emul_unplug = 0 above */;
 		else
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index ac1b682..ab735a6 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -834,7 +834,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_IGNORE) {
+		if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) {
 			int major;
 
 			if (!VDEV_IS_EXTENDED(vdevice))
diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
index ce9d671..b0dca21 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -20,7 +20,7 @@
 #define XEN_UNPLUG_ALL_NICS 2
 #define XEN_UNPLUG_AUX_IDE_DISKS 4
 #define XEN_UNPLUG_ALL 7
-#define XEN_UNPLUG_IGNORE 8
+#define XEN_UNPLUG_UNNECESSARY 8
 
 static inline int xen_must_unplug_nics(void) {
 #if (defined(CONFIG_XEN_NETDEV_FRONTEND) || \
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-19 09:59 UTC

head link

[Xen-devel] [PATCH 3/3] xen: pvhvm: make it clearer that XEN_UNPLUG_* individual bits

Make this clear by defining in terms of (1<<N)

XEN_UNPLUG_UNNECESSARY is only used within the kernel and is not
defined as a bit on the unplug IO port. Therefore use a bit which is
outside the potentially valid range of the 16 bit IO port.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 include/xen/platform_pci.h |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
index b0dca21..dba26dd 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -16,11 +16,14 @@
 #define XEN_IOPORT_PROTOVER	(XEN_IOPORT_BASE + 2) /* 1 byte access (R) */
 #define XEN_IOPORT_PRODNUM	(XEN_IOPORT_BASE + 2) /* 2 byte access (W) */
 
-#define XEN_UNPLUG_ALL_IDE_DISKS 1
-#define XEN_UNPLUG_ALL_NICS 2
-#define XEN_UNPLUG_AUX_IDE_DISKS 4
-#define XEN_UNPLUG_ALL 7
-#define XEN_UNPLUG_UNNECESSARY 8
+#define XEN_UNPLUG_ALL_IDE_DISKS	(1<<0)
+#define XEN_UNPLUG_ALL_NICS		(1<<1)
+#define XEN_UNPLUG_AUX_IDE_DISKS	(1<<2)
+#define XEN_UNPLUG_ALL			(XEN_UNPLUG_ALL_IDE_DISKS|\
+					 XEN_UNPLUG_ALL_NICS|\
+					 XEN_UNPLUG_AUX_IDE_DISKS)
+
+#define XEN_UNPLUG_UNNECESSARY 		(1<<16)
 
 static inline int xen_must_unplug_nics(void) {
 #if (defined(CONFIG_XEN_NETDEV_FRONTEND) || \
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2010-Aug-19 10:37 UTC

head link

[Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On Thu, 19 Aug 2010, Ian Campbell wrote:> this allows the user to disable pvhvm and revert to emulated devices
> in case of a system misconfiguration (e.g. initramfs with only
> emulated drivers in it).
> 
I think this option might be a good thing to have, expecially to debug
qemu-xen emulated devices.
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  Documentation/kernel-parameters.txt |    1 +
>  arch/x86/xen/platform-pci-unplug.c  |   12 ++++++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
> index 2c85c06..8bbe83b 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2631,6 +2631,7 @@ and is between 256 and 4096 characters. It is defined
in the file
>  			all -- unplug all emulated devices (NICs and IDE disks)
>  			ignore -- continue loading the Xen platform PCI driver even
>  				if the version check failed
> +			never -- do not unplug even if version check succeeds
>  
>  	xirc2ps_cs=	[NET,PCMCIA]
>  			Format:
> diff --git a/arch/x86/xen/platform-pci-unplug.c
b/arch/x86/xen/platform-pci-unplug.c
> index 554c002..c82ce1e 100644
> --- a/arch/x86/xen/platform-pci-unplug.c
> +++ b/arch/x86/xen/platform-pci-unplug.c
> @@ -33,7 +33,7 @@
>  int xen_platform_pci_unplug;
>  EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>  #ifdef CONFIG_XEN_PVHVM
> -static int xen_emul_unplug;
> +static int xen_emul_unplug = -1;
>  
>  static int __init check_platform_magic(void)
>  {
> @@ -72,18 +72,23 @@ void __init xen_unplug_emulated_devices(void)
>  {
>  	int r;
>  
> +	/* user explicitly requested no unplug */
> +	if (xen_emul_unplug == 0)
> +		return;
>  	/* check the version of the xen platform PCI device */
>  	r = check_platform_magic();
>  	/* If the version matches enable the Xen platform PCI driver.
>  	 * Also enable the Xen platform PCI driver if the version is really old
>  	 * and the user told us to ignore it. */
>  	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
> +			(xen_emul_unplug != -1) &&
>  			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
I wouldn''t add xen_emul_unplug != -1 because it should be clear that
xen_emul_unplug & XEN_UNPLUG_IGNORE always implies xen_emul_unplug != -1.

The other two patches look fine to me.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-19 10:50 UTC

head link

[Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On Thu, 2010-08-19 at 11:37 +0100, Stefano Stabellini
wrote:> On Thu, 19 Aug 2010, Ian Campbell wrote:
> >  	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
> > +			(xen_emul_unplug != -1) &&
> >  			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
> 
> I wouldn''t add xen_emul_unplug != -1 because it should be clear
that
> xen_emul_unplug & XEN_UNPLUG_IGNORE always implies xen_emul_unplug !=
-1.
That''s not correct since -1 is all 1s. So you can get a false positive
for "xen_emul_unplug & XEN_UNPLUG_IGNORE" if xen_emul_unplug ==
-1.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-19 10:52 UTC

head link

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On Thu, 2010-08-19 at 11:50 +0100, Ian Campbell wrote:> On Thu, 2010-08-19 at 11:37 +0100, Stefano Stabellini wrote:
> > On Thu, 19 Aug 2010, Ian Campbell wrote:
> > >  	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
> > > +			(xen_emul_unplug != -1) &&
> > >  			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
> > 
> > I wouldn''t add xen_emul_unplug != -1 because it should be
clear that
> > xen_emul_unplug & XEN_UNPLUG_IGNORE always implies xen_emul_unplug
!= -1.
> 
> That''s not correct since -1 is all 1s. So you can get a false
positive
> for "xen_emul_unplug & XEN_UNPLUG_IGNORE" if xen_emul_unplug
== -1.
IOW if we were to rewrite the test to use less boolean logic the patch
might look like:

	if (r) {
		if (r != XEN_PLATFORM_ERR_MAGIC)
			return;
+		if (xen_emul_unplug == -1)
+			return;
		if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
			return;
	}

Perhaps this refactoring is worthwhile in any case? It certainly makes
my head hurt less ;-)

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2010-Aug-19 10:54 UTC

head link

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On Thu, 19 Aug 2010, Ian Campbell wrote:> On Thu, 2010-08-19 at 11:50 +0100, Ian Campbell wrote:
> > On Thu, 2010-08-19 at 11:37 +0100, Stefano Stabellini wrote:
> > > On Thu, 19 Aug 2010, Ian Campbell wrote:
> > > >  	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
> > > > +			(xen_emul_unplug != -1) &&
> > > >  			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
> > > 
> > > I wouldn''t add xen_emul_unplug != -1 because it should
be clear that
> > > xen_emul_unplug & XEN_UNPLUG_IGNORE always implies
xen_emul_unplug != -1.
> > 
> > That''s not correct since -1 is all 1s. So you can get a false
positive
> > for "xen_emul_unplug & XEN_UNPLUG_IGNORE" if
xen_emul_unplug == -1.
> 
> IOW if we were to rewrite the test to use less boolean logic the patch
> might look like:
> 
> 	if (r) {
> 		if (r != XEN_PLATFORM_ERR_MAGIC)
> 			return;
> +		if (xen_emul_unplug == -1)
> +			return;
> 		if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
> 			return;
> 	}
> 
> Perhaps this refactoring is worthwhile in any case? It certainly makes
> my head hurt less ;-)
>  
Yeah, it is probably worth it anyway :)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jeremy Fitzhardinge

2010-Aug-19 16:10 UTC

head link

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On 08/19/2010 03:54 AM, Stefano Stabellini wrote:> On Thu, 19 Aug 2010, Ian Campbell wrote:
>> On Thu, 2010-08-19 at 11:50 +0100, Ian Campbell wrote:
>>> On Thu, 2010-08-19 at 11:37 +0100, Stefano Stabellini wrote:
>>>> On Thu, 19 Aug 2010, Ian Campbell wrote:
>>>>>  	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
>>>>> +			(xen_emul_unplug != -1) &&
>>>>>  			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
>>>> I wouldn''t add xen_emul_unplug != -1 because it should
be clear that
>>>> xen_emul_unplug & XEN_UNPLUG_IGNORE always implies
xen_emul_unplug != -1.
>>> That''s not correct since -1 is all 1s. So you can get a
false positive
>>> for "xen_emul_unplug & XEN_UNPLUG_IGNORE" if
xen_emul_unplug == -1.
>> IOW if we were to rewrite the test to use less boolean logic the patch
>> might look like:
>>
>> 	if (r) {
>> 		if (r != XEN_PLATFORM_ERR_MAGIC)
>> 			return;
>> +		if (xen_emul_unplug == -1)
>> +			return;
>> 		if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
>> 			return;
>> 	}
>>
>> Perhaps this refactoring is worthwhile in any case? It certainly makes
>> my head hurt less ;-)
>>
>  
> Yeah, it is probably worth it anyway :)
Treating a variable as an integer and a bitfield seems like a bad idea.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gianni Tedesco

2010-Aug-19 16:34 UTC

head link

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On Thu, 2010-08-19 at 17:10 +0100, Jeremy Fitzhardinge
wrote:> On 08/19/2010 03:54 AM, Stefano Stabellini wrote:
> > On Thu, 19 Aug 2010, Ian Campbell wrote:
> >> On Thu, 2010-08-19 at 11:50 +0100, Ian Campbell wrote:
> >>> On Thu, 2010-08-19 at 11:37 +0100, Stefano Stabellini wrote:
> >>>> On Thu, 19 Aug 2010, Ian Campbell wrote:
> >>>>>  	if (r && !(r == XEN_PLATFORM_ERR_MAGIC
&&
> >>>>> +			(xen_emul_unplug != -1) &&
> >>>>>  			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
> >>>> I wouldn''t add xen_emul_unplug != -1 because it
should be clear that
> >>>> xen_emul_unplug & XEN_UNPLUG_IGNORE always implies
xen_emul_unplug != -1.
> >>> That''s not correct since -1 is all 1s. So you can get
a false positive
> >>> for "xen_emul_unplug & XEN_UNPLUG_IGNORE" if
xen_emul_unplug == -1.
> >> IOW if we were to rewrite the test to use less boolean logic the
patch
> >> might look like:
> >>
> >> 	if (r) {
> >> 		if (r != XEN_PLATFORM_ERR_MAGIC)
> >> 			return;
> >> +		if (xen_emul_unplug == -1)
> >> +			return;
> >> 		if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
> >> 			return;
> >> 	}
> >>
> >> Perhaps this refactoring is worthwhile in any case? It certainly
makes
> >> my head hurt less ;-)
> >>
> >  
> > Yeah, it is probably worth it anyway :)
> 
> Treating a variable as an integer and a bitfield seems like a bad idea.
Should be fine? I''d just #define XEN_UNPLUG_ALL_THE_BITS ~0U for the
sake of cosmetics...


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jeremy Fitzhardinge

2010-Aug-19 16:44 UTC

head link

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On 08/19/2010 09:34 AM, Gianni Tedesco wrote:> On Thu, 2010-08-19 at 17:10 +0100, Jeremy Fitzhardinge wrote:
>> On 08/19/2010 03:54 AM, Stefano Stabellini wrote:
>>> On Thu, 19 Aug 2010, Ian Campbell wrote:
>>>> On Thu, 2010-08-19 at 11:50 +0100, Ian Campbell wrote:
>>>>> On Thu, 2010-08-19 at 11:37 +0100, Stefano Stabellini
wrote:
>>>>>> On Thu, 19 Aug 2010, Ian Campbell wrote:
>>>>>>>  	if (r && !(r == XEN_PLATFORM_ERR_MAGIC
&&
>>>>>>> +			(xen_emul_unplug != -1) &&
>>>>>>>  			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
>>>>>> I wouldn''t add xen_emul_unplug != -1 because
it should be clear that
>>>>>> xen_emul_unplug & XEN_UNPLUG_IGNORE always implies
xen_emul_unplug != -1.
>>>>> That''s not correct since -1 is all 1s. So you can
get a false positive
>>>>> for "xen_emul_unplug & XEN_UNPLUG_IGNORE" if
xen_emul_unplug == -1.
>>>> IOW if we were to rewrite the test to use less boolean logic
the patch
>>>> might look like:
>>>>
>>>> 	if (r) {
>>>> 		if (r != XEN_PLATFORM_ERR_MAGIC)
>>>> 			return;
>>>> +		if (xen_emul_unplug == -1)
>>>> +			return;
>>>> 		if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
>>>> 			return;
>>>> 	}
>>>>
>>>> Perhaps this refactoring is worthwhile in any case? It
certainly makes
>>>> my head hurt less ;-)
>>>>
>>>  
>>> Yeah, it is probably worth it anyway :)
>> Treating a variable as an integer and a bitfield seems like a bad idea.
> Should be fine? I''d just #define XEN_UNPLUG_ALL_THE_BITS ~0U for
the
> sake of cosmetics...
In a bitfield the bits are typically independent, whereas in an integer
you interpret all the bits to get a value.  Making "all bits set" mean
something other than all the bitfield bits are set is just asking for
something bad to happen to you.

Why not define a bit to mean whatever "-1" currently means?

    J


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-19 16:52 UTC

head link

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

On Thu, 2010-08-19 at 17:44 +0100, Jeremy Fitzhardinge
wrote:> On 08/19/2010 09:34 AM, Gianni Tedesco wrote:
> > On Thu, 2010-08-19 at 17:10 +0100, Jeremy Fitzhardinge wrote:
> >> On 08/19/2010 03:54 AM, Stefano Stabellini wrote:
> >>> On Thu, 19 Aug 2010, Ian Campbell wrote:
> >>>> On Thu, 2010-08-19 at 11:50 +0100, Ian Campbell wrote:
> >>>>> On Thu, 2010-08-19 at 11:37 +0100, Stefano Stabellini
wrote:
> >>>>>> On Thu, 19 Aug 2010, Ian Campbell wrote:
> >>>>>>>  	if (r && !(r ==
XEN_PLATFORM_ERR_MAGIC &&
> >>>>>>> +			(xen_emul_unplug != -1) &&
> >>>>>>>  			(xen_emul_unplug &
XEN_UNPLUG_IGNORE)))
> >>>>>> I wouldn''t add xen_emul_unplug != -1
because it should be clear that
> >>>>>> xen_emul_unplug & XEN_UNPLUG_IGNORE always
implies xen_emul_unplug != -1.
> >>>>> That''s not correct since -1 is all 1s. So you
can get a false positive
> >>>>> for "xen_emul_unplug &
XEN_UNPLUG_IGNORE" if xen_emul_unplug == -1.
> >>>> IOW if we were to rewrite the test to use less boolean
logic the patch
> >>>> might look like:
> >>>>
> >>>> 	if (r) {
> >>>> 		if (r != XEN_PLATFORM_ERR_MAGIC)
> >>>> 			return;
> >>>> +		if (xen_emul_unplug == -1)
> >>>> +			return;
> >>>> 		if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
> >>>> 			return;
> >>>> 	}
> >>>>
> >>>> Perhaps this refactoring is worthwhile in any case? It
certainly makes
> >>>> my head hurt less ;-)
> >>>>
> >>>  
> >>> Yeah, it is probably worth it anyway :)
> >> Treating a variable as an integer and a bitfield seems like a bad
idea.
> > Should be fine? I''d just #define XEN_UNPLUG_ALL_THE_BITS ~0U
for the
> > sake of cosmetics...
> 
> In a bitfield the bits are typically independent, whereas in an integer
> you interpret all the bits to get a value.  Making "all bits set"
mean
> something other than all the bitfield bits are set is just asking for
> something bad to happen to you.
> 
> Why not define a bit to mean whatever "-1" currently means?
I''ll add an explicit XEN_UNPLUG_NEVER instead.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-20 08:13 UTC

head link

[Xen-devel] [GIT/PATCH v2 0/3] pvhvm emul unplug fixes

On Thu, 2010-08-19 at 17:52 +0100, Ian Campbell wrote:> I''ll add an explicit XEN_UNPLUG_NEVER instead. 
Here we go.

Assuming everyone is happy with this round I''ll send the first two to
upstream (see the for-upstream/pvhvm branch at the same location). Or
maybe I should just send all three to Linus? since the third really is
rather trivial.

The following changes since commit da5cabf80e2433131bf0ed8993abc0f7ea618c73:
  Linus Torvalds (1):
        Linux 2.6.36-rc1

are available in the git repository at:

  git://xenbits.xensource.com/people/ianc/linux-2.6.git for-jeremy/pvhvm

Ian Campbell (3):
      xen: pvhvm: allow user to request no emulated device unplug
      xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary
      xen: pvhvm: make it clearer that XEN_UNPLUG_* define bits in a bitfield

 Documentation/kernel-parameters.txt |    6 ++++--
 arch/x86/xen/platform-pci-unplug.c  |   18 ++++++++++++------
 drivers/block/xen-blkfront.c        |    2 +-
 include/xen/platform_pci.h          |   14 +++++++++-----
 4 files changed, 26 insertions(+), 14 deletions(-)



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-20 08:14 UTC

head link

[Xen-devel] [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

this allows the user to disable pvhvm and revert to emulated devices
in case of a system misconfiguration (e.g. initramfs with only
emulated drivers in it).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Documentation/kernel-parameters.txt |    1 +
 arch/x86/xen/platform-pci-unplug.c  |    5 +++++
 include/xen/platform_pci.h          |    1 +
 3 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 2c85c06..8bbe83b 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2631,6 +2631,7 @@ and is between 256 and 4096 characters. It is defined in
the file
 			all -- unplug all emulated devices (NICs and IDE disks)
 			ignore -- continue loading the Xen platform PCI driver even
 				if the version check failed
+			never -- do not unplug even if version check succeeds
 
 	xirc2ps_cs=	[NET,PCMCIA]
 			Format:
diff --git a/arch/x86/xen/platform-pci-unplug.c
b/arch/x86/xen/platform-pci-unplug.c
index 554c002..070dfa0 100644
--- a/arch/x86/xen/platform-pci-unplug.c
+++ b/arch/x86/xen/platform-pci-unplug.c
@@ -72,6 +72,9 @@ void __init xen_unplug_emulated_devices(void)
 {
 	int r;
 
+	/* user explicitly requested no unplug */
+	if (xen_emul_unplug & XEN_UNPLUG_NEVER)
+		return;
 	/* check the version of the xen platform PCI device */
 	r = check_platform_magic();
 	/* If the version matches enable the Xen platform PCI driver.
@@ -127,6 +130,8 @@ static int __init parse_xen_emul_unplug(char *arg)
 			xen_emul_unplug |= XEN_UNPLUG_ALL_NICS;
 		else if (!strncmp(p, "ignore", l))
 			xen_emul_unplug |= XEN_UNPLUG_IGNORE;
+		else if (!strncmp(p, "never", l))
+			xen_emul_unplug |= XEN_UNPLUG_NEVER;
 		else
 			printk(KERN_WARNING "unrecognised option ''%s'' "
 				 "in parameter ''xen_emul_unplug''\n", p);
diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
index ce9d671..123b775 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -21,6 +21,7 @@
 #define XEN_UNPLUG_AUX_IDE_DISKS 4
 #define XEN_UNPLUG_ALL 7
 #define XEN_UNPLUG_IGNORE 8
+#define XEN_UNPLUG_NEVER 16
 
 static inline int xen_must_unplug_nics(void) {
 #if (defined(CONFIG_XEN_NETDEV_FRONTEND) || \
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-20 08:14 UTC

head link

[Xen-devel] [PATCH 2/3] xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary

It is not immediately clear what this option causes to become
ignored. The actual meaning is that it is not necessary to unplug the
emulated devices to safely use the PV ones, even if the platform does
not support the unplug protocol. (pressumably the user will only add
this option if they have ensured that their domain configuration is
safe).

I think xen_emul_unplug=unnecessary better captures this.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Documentation/kernel-parameters.txt |    5 +++--
 arch/x86/xen/platform-pci-unplug.c  |   13 +++++++------
 drivers/block/xen-blkfront.c        |    2 +-
 include/xen/platform_pci.h          |    2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 8bbe83b..f084af0 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2629,8 +2629,9 @@ and is between 256 and 4096 characters. It is defined in
the file
 			aux-ide-disks -- unplug non-primary-master IDE devices
 			nics -- unplug network devices
 			all -- unplug all emulated devices (NICs and IDE disks)
-			ignore -- continue loading the Xen platform PCI driver even
-				if the version check failed
+			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
 
 	xirc2ps_cs=	[NET,PCMCIA]
diff --git a/arch/x86/xen/platform-pci-unplug.c
b/arch/x86/xen/platform-pci-unplug.c
index 070dfa0..0f45638 100644
--- a/arch/x86/xen/platform-pci-unplug.c
+++ b/arch/x86/xen/platform-pci-unplug.c
@@ -78,10 +78,11 @@ void __init xen_unplug_emulated_devices(void)
 	/* check the version of the xen platform PCI device */
 	r = check_platform_magic();
 	/* If the version matches enable the Xen platform PCI driver.
-	 * Also enable the Xen platform PCI driver if the version is really old
-	 * and the user told us to ignore it. */
+	 * Also enable the Xen platform PCI driver if the host does
+	 * not support the unplug protocol (XEN_PLATFORM_ERR_MAGIC)
+	 * but the user told us that unplugging is unnecessary. */
 	if (r && !(r == XEN_PLATFORM_ERR_MAGIC &&
-			(xen_emul_unplug & XEN_UNPLUG_IGNORE)))
+			(xen_emul_unplug & XEN_UNPLUG_UNNECESSARY)))
 		return;
 	/* Set the default value of xen_emul_unplug depending on whether or
 	 * not the Xen PV frontends and the Xen platform PCI driver have
@@ -102,7 +103,7 @@ void __init xen_unplug_emulated_devices(void)
 		}
 	}
 	/* Now unplug the emulated devices */
-	if (!(xen_emul_unplug & XEN_UNPLUG_IGNORE))
+	if (!(xen_emul_unplug & XEN_UNPLUG_UNNECESSARY))
 		outw(xen_emul_unplug, XEN_IOPORT_UNPLUG);
 	xen_platform_pci_unplug = xen_emul_unplug;
 }
@@ -128,8 +129,8 @@ static int __init parse_xen_emul_unplug(char *arg)
 			xen_emul_unplug |= XEN_UNPLUG_AUX_IDE_DISKS;
 		else if (!strncmp(p, "nics", l))
 			xen_emul_unplug |= XEN_UNPLUG_ALL_NICS;
-		else if (!strncmp(p, "ignore", l))
-			xen_emul_unplug |= XEN_UNPLUG_IGNORE;
+		else if (!strncmp(p, "unnecessary", l))
+			xen_emul_unplug |= XEN_UNPLUG_UNNECESSARY;
 		else if (!strncmp(p, "never", l))
 			xen_emul_unplug |= XEN_UNPLUG_NEVER;
 		else
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index ac1b682..ab735a6 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -834,7 +834,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_IGNORE) {
+		if (xen_platform_pci_unplug & XEN_UNPLUG_UNNECESSARY) {
 			int major;
 
 			if (!VDEV_IS_EXTENDED(vdevice))
diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
index 123b775..590ccfd 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -20,7 +20,7 @@
 #define XEN_UNPLUG_ALL_NICS 2
 #define XEN_UNPLUG_AUX_IDE_DISKS 4
 #define XEN_UNPLUG_ALL 7
-#define XEN_UNPLUG_IGNORE 8
+#define XEN_UNPLUG_UNNECESSARY 8
 #define XEN_UNPLUG_NEVER 16
 
 static inline int xen_must_unplug_nics(void) {
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Aug-20 08:14 UTC

head link

[Xen-devel] [PATCH 3/3] xen: pvhvm: make it clearer that XEN_UNPLUG_* define bits in a bitfield

Make this clear by defining in terms of (1<<N)

XEN_UNPLUG_UNNECESSARY is only used within the kernel and is not
defined as a bit on the unplug IO port. Therefore use a bit which is
outside the potentially valid range of the 16 bit IO port.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 include/xen/platform_pci.h |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
index 590ccfd..a785a3b 100644
--- a/include/xen/platform_pci.h
+++ b/include/xen/platform_pci.h
@@ -16,12 +16,15 @@
 #define XEN_IOPORT_PROTOVER	(XEN_IOPORT_BASE + 2) /* 1 byte access (R) */
 #define XEN_IOPORT_PRODNUM	(XEN_IOPORT_BASE + 2) /* 2 byte access (W) */
 
-#define XEN_UNPLUG_ALL_IDE_DISKS 1
-#define XEN_UNPLUG_ALL_NICS 2
-#define XEN_UNPLUG_AUX_IDE_DISKS 4
-#define XEN_UNPLUG_ALL 7
-#define XEN_UNPLUG_UNNECESSARY 8
-#define XEN_UNPLUG_NEVER 16
+#define XEN_UNPLUG_ALL_IDE_DISKS	(1<<0)
+#define XEN_UNPLUG_ALL_NICS		(1<<1)
+#define XEN_UNPLUG_AUX_IDE_DISKS	(1<<2)
+#define XEN_UNPLUG_ALL			(XEN_UNPLUG_ALL_IDE_DISKS|\
+					 XEN_UNPLUG_ALL_NICS|\
+					 XEN_UNPLUG_AUX_IDE_DISKS)
+
+#define XEN_UNPLUG_UNNECESSARY 		(1<<16)
+#define XEN_UNPLUG_NEVER	 		(1<<17)
 
 static inline int xen_must_unplug_nics(void) {
 #if (defined(CONFIG_XEN_NETDEV_FRONTEND) || \
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2010-Aug-20 11:10 UTC

head link

[Xen-devel] Re: [GIT/PATCH v2 0/3] pvhvm emul unplug fixes

On Fri, 20 Aug 2010, Ian Campbell wrote:> On Thu, 2010-08-19 at 17:52 +0100, Ian Campbell wrote:
> > I''ll add an explicit XEN_UNPLUG_NEVER instead. 
> 
> Here we go.
> 
> Assuming everyone is happy with this round I''ll send the first two
to
> upstream (see the for-upstream/pvhvm branch at the same location). Or
> maybe I should just send all three to Linus? since the third really is
> rather trivial.
> 
> The following changes since commit
da5cabf80e2433131bf0ed8993abc0f7ea618c73:
>   Linus Torvalds (1):
>         Linux 2.6.36-rc1
> 
> are available in the git repository at:
> 
>   git://xenbits.xensource.com/people/ianc/linux-2.6.git for-jeremy/pvhvm
> 
> Ian Campbell (3):
>       xen: pvhvm: allow user to request no emulated device unplug
>       xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary
>       xen: pvhvm: make it clearer that XEN_UNPLUG_* define bits in a
bitfield
> 
>  Documentation/kernel-parameters.txt |    6 ++++--
>  arch/x86/xen/platform-pci-unplug.c  |   18 ++++++++++++------
>  drivers/block/xen-blkfront.c        |    2 +-
>  include/xen/platform_pci.h          |   14 +++++++++-----
>  4 files changed, 26 insertions(+), 14 deletions(-)
> 
> 
they are all fine by me

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jeremy Fitzhardinge

2010-Aug-20 16:41 UTC

head link

[Xen-devel] Re: [GIT/PATCH v2 0/3] pvhvm emul unplug fixes

On 08/20/2010 01:13 AM, Ian Campbell wrote:> On Thu, 2010-08-19 at 17:52 +0100, Ian Campbell wrote:
>> I''ll add an explicit XEN_UNPLUG_NEVER instead. 
> Here we go.
>
> Assuming everyone is happy with this round I''ll send the first two
to
> upstream (see the for-upstream/pvhvm branch at the same location). Or
> maybe I should just send all three to Linus? since the third really is
> rather trivial.
Yep.

All Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

    J
> The following changes since commit
da5cabf80e2433131bf0ed8993abc0f7ea618c73:
>   Linus Torvalds (1):
>         Linux 2.6.36-rc1
>
> are available in the git repository at:
>
>   git://xenbits.xensource.com/people/ianc/linux-2.6.git for-jeremy/pvhvm
>
> Ian Campbell (3):
>       xen: pvhvm: allow user to request no emulated device unplug
>       xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary
>       xen: pvhvm: make it clearer that XEN_UNPLUG_* define bits in a
bitfield
>
>  Documentation/kernel-parameters.txt |    6 ++++--
>  arch/x86/xen/platform-pci-unplug.c  |   18 ++++++++++++------
>  drivers/block/xen-blkfront.c        |    2 +-
>  include/xen/platform_pci.h          |   14 +++++++++-----
>  4 files changed, 26 insertions(+), 14 deletions(-)
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Aug 2010 - [GIT/PATCH 0/3] pvhvm emul unplug fixes

[Xen-devel] [GIT/PATCH 0/3] pvhvm emul unplug fixes

[Xen-devel] [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

[Xen-devel] [PATCH 2/3] xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary

[Xen-devel] [PATCH 3/3] xen: pvhvm: make it clearer that XEN_UNPLUG_* individual bits

[Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

[Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

Re: [Xen-devel] Re: [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

[Xen-devel] [GIT/PATCH v2 0/3] pvhvm emul unplug fixes

[Xen-devel] [PATCH 1/3] xen: pvhvm: allow user to request no emulated device unplug

[Xen-devel] [PATCH 2/3] xen: pvhvm: rename xen_emul_unplug=ignore to =unnnecessary

[Xen-devel] [PATCH 3/3] xen: pvhvm: make it clearer that XEN_UNPLUG_* define bits in a bitfield

[Xen-devel] Re: [GIT/PATCH v2 0/3] pvhvm emul unplug fixes

[Xen-devel] Re: [GIT/PATCH v2 0/3] pvhvm emul unplug fixes