[GIT/PATCH 0/3] pvhvm emul unplug fixes

[GIT/PATCH 0/3] pvhvm emul unplug fixes

[Ian Campbell]

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(-)

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

[Ian Campbell]

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

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

[Stefano Stabellini]

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.

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

[Ian Campbell]

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.

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

[Ian Campbell]

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.

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

[Stefano Stabellini]

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 :)

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

[Jeremy Fitzhardinge]

 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

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

[Gianni Tedesco]

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

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

[Jeremy Fitzhardinge]

 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

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

[Ian Campbell]

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.

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

[Ian Campbell]

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(-)

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

[Ian Campbell]

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

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

[Ian Campbell]

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

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

[Ian Campbell]

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

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

[Stefano Stabellini]

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

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

[Jeremy Fitzhardinge]

 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(-)
>
>

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

[Ian Campbell]

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

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

[Ian Campbell]

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