[PATCH] bug-fixes to hvc-xen driver in v3.4 (and earlier).

[PATCH] bug-fixes to hvc-xen driver in v3.4 (and earlier).

[Konrad Rzeszutek Wilk]

Three of the patches could be squashed in one, but it makes more sense
to review them as three. These patches fix the case of an PVHVM
guest not being able to resume propely b/c of hitting:

 142         BUG_ON(info->type != IRQT_UNBOUND && info->type != type);

(in events.c) and also adds a WARN to catch situations like these.

The reason for this is that the Xen python toolstack does not
setup HVM_PARAM_CONSOLE_EVTCHN parameter, so we would use the
default value (0).. and ended up using the same IRQ line as the
xen-platform-pci driver. Huh? Note: the 'xl' toolstack _does_
set it.

The reason is that the when the xen-platform-pci starts, it requests
an PIRQ, and the info structure in events.c is filled as such:

 info->type = IRQT_PIRQ
 info->events = 0
 info->irq = 28
 evtchn_to_irq[0] = 28

And when xen_hvc_init is called during bootup, it figures out the
event from HVM_PARAM_CONSOLE_EVTCHN as zero, and plugs them in:

 532                 info->irq = bind_evtchn_to_irq(info->evtchn);

getting IRQ 28 as bind_evtchn_to_irq does a quick lookup:

 818         irq = evtchn_to_irq[evtchn];

and gives an already used IRQ line.

When we suspend the guest (xm save), then try to resume, we call
xen_console_resume, which does:

301         if (info != NULL && info->irq)
302                 rebind_evtchn_irq(info->evtchn, info->irq);

and that triggers the BUG_ON mentioned above (b/c the type
we want is IRQT_EVTCHN and the type info->type is IRQT_PIRQ).

 drivers/tty/hvc/hvc_xen.c |   24 +++++++++++-------------
 drivers/xen/events.c      |   11 +++++++++--
 2 files changed, 20 insertions(+), 15 deletions(-)

[PATCH 1/4] xen/hvc: Collapse error logic.

[Konrad Rzeszutek Wilk]

All of the error paths are doing the same logic. In which
case we might as well collapse them in one path.

CC: stable@kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index d3d91da..afc7fc2 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -216,22 +216,16 @@ static int xen_hvm_console_init(void)
 		return 0;
 
 	r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
-	if (r < 0) {
-		kfree(info);
-		return -ENODEV;
-	}
+	if (r < 0)
+		goto err;
 	info->evtchn = v;
 	hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
-	if (r < 0) {
-		kfree(info);
-		return -ENODEV;
-	}
+	if (r < 0)
+		goto err;
 	mfn = v;
 	info->intf = ioremap(mfn << PAGE_SHIFT, PAGE_SIZE);
-	if (info->intf == NULL) {
-		kfree(info);
-		return -ENODEV;
-	}
+	if (info->intf == NULL)
+		goto err;
 	info->vtermno = HVC_COOKIE;
 
 	spin_lock(&xencons_lock);
@@ -239,6 +233,9 @@ static int xen_hvm_console_init(void)
 	spin_unlock(&xencons_lock);
 
 	return 0;
+err:
+	kfree(info);
+	return -ENODEV;
 }
 
 static int xen_pv_console_init(void)
-- 
1.7.7.5

[PATCH 2/4] xen/hvc: Fix error cases around HVM_PARAM_CONSOLE_PFN

[Konrad Rzeszutek Wilk]

We weren't resetting the parameter to be passed in to a
known default. Nor were we checking the return value of
hvm_get_parameter.

CC: stable@kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index afc7fc2..3277f0e 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -219,7 +219,8 @@ static int xen_hvm_console_init(void)
 	if (r < 0)
 		goto err;
 	info->evtchn = v;
-	hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
+	v = 0;
+	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
 	if (r < 0)
 		goto err;
 	mfn = v;
-- 
1.7.7.5

[PATCH 3/4] xen/hvc: Check HVM_PARAM_CONSOLE_[EVTCHN|PFN] for correctness.

[Konrad Rzeszutek Wilk]

We need to make sure that those parameters are setup to be correct.
As such the value of 0 is deemed invalid and we find that we
bail out. The hypervisor sets by default all of them to be zero
and when the hypercall is done does a simple:

 a.value = d->arch.hvm_domain.params[a.index];

Which means that if the Xen toolstack forgot to setup the proper
HVM_PARAM_CONSOLE_EVTCHN, we would get the default value of 0
and use that.

CC: stable@kernel.org
Fixes-Oracle-Bug: 14091238
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 3277f0e..5fb1cb9 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -216,12 +216,12 @@ static int xen_hvm_console_init(void)
 		return 0;
 
 	r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
-	if (r < 0)
+	if (r < 0 || v == 0)
 		goto err;
 	info->evtchn = v;
 	v = 0;
 	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
-	if (r < 0)
+	if (r < 0 || v == 0)
 		goto err;
 	mfn = v;
 	info->intf = ioremap(mfn << PAGE_SHIFT, PAGE_SIZE);
-- 
1.7.7.5

[PATCH 4/4] xen/events: Add WARN_ON when quick lookup found invalid type.

[Konrad Rzeszutek Wilk]

All of the bind_XYZ_to_irq do a quick lookup to see if the
event exists. And if they that value is returned instead of
initialized. This patch adds an extra logic to check that the
type returned is the proper one and we can use it to find
drivers that are doing something naught.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/events.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index faae2f9..093851b 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -827,6 +827,9 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 					      handle_edge_irq, "event");
 
 		xen_irq_info_evtchn_init(irq, evtchn);
+	} else {
+		struct irq_info *info = info_for_irq(irq);
+		WARN_ON(info && info->type != IRQT_EVTCHN);
 	}
 
 out:
@@ -862,8 +865,10 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
 		xen_irq_info_ipi_init(cpu, irq, evtchn, ipi);
 
 		bind_evtchn_to_cpu(evtchn, cpu);
+	} else {
+		struct irq_info *info = info_for_irq(irq);
+		WARN_ON(info && info->type != IRQT_IPI);
 	}
-
  out:
 	mutex_unlock(&irq_mapping_update_lock);
 	return irq;
@@ -939,8 +944,10 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
 		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
 
 		bind_evtchn_to_cpu(evtchn, cpu);
+	} else {
+		struct irq_info *info = info_for_irq(irq);
+		WARN_ON(info && info->type != IRQT_VIRQ);
 	}
-
 out:
 	mutex_unlock(&irq_mapping_update_lock);
 
-- 
1.7.7.5

Re: [PATCH 1/4] xen/hvc: Collapse error logic.

[Stefano Stabellini]

On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> All of the error paths are doing the same logic. In which
> case we might as well collapse them in one path.
> 
> CC: stable@kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Re: [Xen-devel] [PATCH 2/4] xen/hvc: Fix error cases around HVM_PARAM_CONSOLE_PFN

[Stefano Stabellini]

On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> We weren't resetting the parameter to be passed in to a
> known default. Nor were we checking the return value of
> hvm_get_parameter.
> 
> CC: stable@kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
>  drivers/tty/hvc/hvc_xen.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index afc7fc2..3277f0e 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -219,7 +219,8 @@ static int xen_hvm_console_init(void)
>  	if (r < 0)
>  		goto err;
>  	info->evtchn = v;
> -	hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> +	v = 0;
> +	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
>  	if (r < 0)
>  		goto err;
>  	mfn = v;

Is 0 the right default here?
Maybe something invalid like (~0UL) would be better?

Re: [PATCH 3/4] xen/hvc: Check HVM_PARAM_CONSOLE_[EVTCHN|PFN] for correctness.

[Stefano Stabellini]

On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> We need to make sure that those parameters are setup to be correct.
> As such the value of 0 is deemed invalid and we find that we
> bail out. The hypervisor sets by default all of them to be zero
> and when the hypercall is done does a simple:
> 
>  a.value = d->arch.hvm_domain.params[a.index];
> 
> Which means that if the Xen toolstack forgot to setup the proper
> HVM_PARAM_CONSOLE_EVTCHN, we would get the default value of 0
> and use that.

I see, so the default value for v needs to be 0, because it's what the
hypervisor would return anyway if the parameter hasn't been set.


> CC: stable@kernel.org
> Fixes-Oracle-Bug: 14091238
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/tty/hvc/hvc_xen.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 3277f0e..5fb1cb9 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -216,12 +216,12 @@ static int xen_hvm_console_init(void)
>  		return 0;
>  
>  	r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
> -	if (r < 0)
> +	if (r < 0 || v == 0)
>  		goto err;
>  	info->evtchn = v;
>  	v = 0;
>  	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> -	if (r < 0)
> +	if (r < 0 || v == 0)
>  		goto err;
>  	mfn = v;
>  	info->intf = ioremap(mfn << PAGE_SHIFT, PAGE_SIZE);

I think that we should add a comment stating that even though mfn = 0
and evtchn = 0 are theoretically correct values, in practice they never
are and they mean that a legacy toolstack hasn't initialized the pv
console correctly.

Re: [Xen-devel] [PATCH 4/4] xen/events: Add WARN_ON when quick lookup found invalid type.

[Stefano Stabellini]

On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> All of the bind_XYZ_to_irq do a quick lookup to see if the
> event exists. And if they that value is returned instead of

                           ^

> initialized. This patch adds an extra logic to check that the
> type returned is the proper one and we can use it to find
> drivers that are doing something naught.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/xen/events.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index faae2f9..093851b 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -827,6 +827,9 @@ int bind_evtchn_to_irq(unsigned int evtchn)
>  					      handle_edge_irq, "event");
>  
>  		xen_irq_info_evtchn_init(irq, evtchn);
> +	} else {
> +		struct irq_info *info = info_for_irq(irq);
> +		WARN_ON(info && info->type != IRQT_EVTCHN);
>  	}

considering that when evtchn_to_irq[evtchn] is != -1, a corresponding
struct irq_info is always supposed to exists, shouldn't this be:

WARN_ON(info == NULL || info->type != IRQT_EVTCHN);

?


>  out:
> @@ -862,8 +865,10 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
>  		xen_irq_info_ipi_init(cpu, irq, evtchn, ipi);
>  
>  		bind_evtchn_to_cpu(evtchn, cpu);
> +	} else {
> +		struct irq_info *info = info_for_irq(irq);
> +		WARN_ON(info && info->type != IRQT_IPI);
>  	}
> -
>   out:
>  	mutex_unlock(&irq_mapping_update_lock);
>  	return irq;
> @@ -939,8 +944,10 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
>  		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
>  
>  		bind_evtchn_to_cpu(evtchn, cpu);
> +	} else {
> +		struct irq_info *info = info_for_irq(irq);
> +		WARN_ON(info && info->type != IRQT_VIRQ);
>  	}
> -
>  out:
>  	mutex_unlock(&irq_mapping_update_lock);

same here

Re: [Xen-devel] [PATCH 4/4] xen/events: Add WARN_ON when quick lookup found invalid type.

[Konrad Rzeszutek Wilk]

On Thu, May 24, 2012 at 11:58:11AM +0100, Stefano Stabellini wrote:
> On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> > All of the bind_XYZ_to_irq do a quick lookup to see if the
> > event exists. And if they that value is returned instead of
> 
>                            ^

Lack of filters for coffee caused that :-)

> 
> > initialized. This patch adds an extra logic to check that the
> > type returned is the proper one and we can use it to find
> > drivers that are doing something naught.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/xen/events.c |   11 +++++++++--
> >  1 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index faae2f9..093851b 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -827,6 +827,9 @@ int bind_evtchn_to_irq(unsigned int evtchn)
> >  					      handle_edge_irq, "event");
> >  
> >  		xen_irq_info_evtchn_init(irq, evtchn);
> > +	} else {
> > +		struct irq_info *info = info_for_irq(irq);
> > +		WARN_ON(info && info->type != IRQT_EVTCHN);
> >  	}
> 
> considering that when evtchn_to_irq[evtchn] is != -1, a corresponding
> struct irq_info is always supposed to exists, shouldn't this be:
> 
> WARN_ON(info == NULL || info->type != IRQT_EVTCHN);

<nods> Much better. Will do that.

> 
> ?
> 
> 
> >  out:
> > @@ -862,8 +865,10 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
> >  		xen_irq_info_ipi_init(cpu, irq, evtchn, ipi);
> >  
> >  		bind_evtchn_to_cpu(evtchn, cpu);
> > +	} else {
> > +		struct irq_info *info = info_for_irq(irq);
> > +		WARN_ON(info && info->type != IRQT_IPI);
> >  	}
> > -
> >   out:
> >  	mutex_unlock(&irq_mapping_update_lock);
> >  	return irq;
> > @@ -939,8 +944,10 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
> >  		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
> >  
> >  		bind_evtchn_to_cpu(evtchn, cpu);
> > +	} else {
> > +		struct irq_info *info = info_for_irq(irq);
> > +		WARN_ON(info && info->type != IRQT_VIRQ);
> >  	}
> > -
> >  out:
> >  	mutex_unlock(&irq_mapping_update_lock);
> 
> same here
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [Xen-devel] [PATCH 2/4] xen/hvc: Fix error cases around HVM_PARAM_CONSOLE_PFN

[Konrad Rzeszutek Wilk]

On Thu, May 24, 2012 at 11:47:12AM +0100, Stefano Stabellini wrote:
> On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> > We weren't resetting the parameter to be passed in to a
> > known default. Nor were we checking the return value of
> > hvm_get_parameter.
> > 
> > CC: stable@kernel.org
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> >  drivers/tty/hvc/hvc_xen.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > index afc7fc2..3277f0e 100644
> > --- a/drivers/tty/hvc/hvc_xen.c
> > +++ b/drivers/tty/hvc/hvc_xen.c
> > @@ -219,7 +219,8 @@ static int xen_hvm_console_init(void)
> >  	if (r < 0)
> >  		goto err;
> >  	info->evtchn = v;
> > -	hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> > +	v = 0;
> > +	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> >  	if (r < 0)
> >  		goto err;
> >  	mfn = v;
> 
> Is 0 the right default here?
> Maybe something invalid like (~0UL) would be better?

Perhaps both? The zero is the default non-initialized value. But
-0UL is also a good check value.

Re: [Xen-devel] [PATCH 2/4] xen/hvc: Fix error cases around HVM_PARAM_CONSOLE_PFN

[Konrad Rzeszutek Wilk]

On Thu, May 24, 2012 at 01:31:10PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, May 24, 2012 at 11:47:12AM +0100, Stefano Stabellini wrote:
> > On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> > > We weren't resetting the parameter to be passed in to a
> > > known default. Nor were we checking the return value of
> > > hvm_get_parameter.
> > > 
> > > CC: stable@kernel.org
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > >
> > >  drivers/tty/hvc/hvc_xen.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > > index afc7fc2..3277f0e 100644
> > > --- a/drivers/tty/hvc/hvc_xen.c
> > > +++ b/drivers/tty/hvc/hvc_xen.c
> > > @@ -219,7 +219,8 @@ static int xen_hvm_console_init(void)
> > >  	if (r < 0)
> > >  		goto err;
> > >  	info->evtchn = v;
> > > -	hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> > > +	v = 0;
> > > +	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> > >  	if (r < 0)
> > >  		goto err;
> > >  	mfn = v;
> > 
> > Is 0 the right default here?
> > Maybe something invalid like (~0UL) would be better?
> 
> Perhaps both? The zero is the default non-initialized value. But
> -0UL is also a good check value.

Somehow I misread your comment as checking the return value, not the
default value.

I think zero is the right choice as that is the default non-initialized
value.

Re: [Xen-devel] [PATCH 3/4] xen/hvc: Check HVM_PARAM_CONSOLE_[EVTCHN|PFN] for correctness.

[Konrad Rzeszutek Wilk]

> I think that we should add a comment stating that even though mfn = 0
> and evtchn = 0 are theoretically correct values, in practice they never
> are and they mean that a legacy toolstack hasn't initialized the pv
> console correctly.

Like this?

>From 5842f5768599094758931b74190cdf93641a8e35 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 23 May 2012 12:56:59 -0400
Subject: [PATCH] xen/hvc: Check HVM_PARAM_CONSOLE_[EVTCHN|PFN] for
 correctness.

We need to make sure that those parameters are setup to be correct.
As such the value of 0 is deemed invalid and we find that we
bail out. The hypervisor sets by default all of them to be zero
and when the hypercall is done does a simple:

 a.value = d->arch.hvm_domain.params[a.index];

Which means that if the Xen toolstack forgot to setup the proper
HVM_PARAM_CONSOLE_EVTCHN (or the PFN one), we would get the
default value of 0 and use that.

CC: stable@kernel.org
Fixes-Oracle-Bug: 14091238
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/tty/hvc/hvc_xen.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 3277f0e..944eaeb 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -214,14 +214,19 @@ static int xen_hvm_console_init(void)
 	/* already configured */
 	if (info->intf != NULL)
 		return 0;
-
+	/*
+	 * If the toolstack (or the hypervisor) hasn't set these values, the
+	 * default value is 0. Even though mfn = 0 and evtchn = 0 are
+	 * theoretically correct values, in practice they never are and they
+	 * mean that a legacy toolstack hasn't initialized the pv console correctly.
+	 */
 	r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
-	if (r < 0)
+	if (r < 0 || v == 0)
 		goto err;
 	info->evtchn = v;
 	v = 0;
 	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
-	if (r < 0)
+	if (r < 0 || v == 0)
 		goto err;
 	mfn = v;
 	info->intf = ioremap(mfn << PAGE_SHIFT, PAGE_SIZE);
-- 
1.7.7.5

Re: [Xen-devel] [PATCH 4/4] xen/events: Add WARN_ON when quick lookup found invalid type.

[Konrad Rzeszutek Wilk]

On Thu, May 24, 2012 at 11:58:11AM +0100, Stefano Stabellini wrote:
> On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> > All of the bind_XYZ_to_irq do a quick lookup to see if the
> > event exists. And if they that value is returned instead of
> 
>                            ^

... snip. How about this:

>From 5963cc2699db7b2893e7925bd2e68ada9d97e8ef Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 23 May 2012 13:28:44 -0400
Subject: [PATCH] xen/events: Add WARN_ON when quick lookup found invalid
 type.

All of the bind_XYZ_to_irq do a quick lookup to see if the
event exists. And if it does, then the initialized IRQ number
is returned instead of initializing a new IRQ number.

This patch adds an extra logic to check that the type returned
is proper one and that there is an IRQ handler setup for it.

This patch has the benefit of being able to find drivers that
are doing something naught.

[v1: Enhanced based on Stefano's review]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/events.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index faae2f9..81f338a 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -827,6 +827,9 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 					      handle_edge_irq, "event");
 
 		xen_irq_info_evtchn_init(irq, evtchn);
+	} else {
+		struct irq_info *info = info_for_irq(irq);
+		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
 	}
 
 out:
@@ -862,8 +865,10 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
 		xen_irq_info_ipi_init(cpu, irq, evtchn, ipi);
 
 		bind_evtchn_to_cpu(evtchn, cpu);
+	} else {
+		struct irq_info *info = info_for_irq(irq);
+		WARN_ON(info == NULL || info->type != IRQT_IPI);
 	}
-
  out:
 	mutex_unlock(&irq_mapping_update_lock);
 	return irq;
@@ -939,8 +944,10 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
 		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
 
 		bind_evtchn_to_cpu(evtchn, cpu);
+	} else {
+		struct irq_info *info = info_for_irq(irq);
+		WARN_ON(info == NULL || info->type != IRQT_VIRQ);
 	}
-
 out:
 	mutex_unlock(&irq_mapping_update_lock);
 
-- 
1.7.7.5

Re: [Xen-devel] [PATCH 3/4] xen/hvc: Check HVM_PARAM_CONSOLE_[EVTCHN|PFN] for correctness.

[Stefano Stabellini]

On Thu, 24 May 2012, Konrad Rzeszutek Wilk wrote:
> > I think that we should add a comment stating that even though mfn = 0
> > and evtchn = 0 are theoretically correct values, in practice they never
> > are and they mean that a legacy toolstack hasn't initialized the pv
> > console correctly.
> 
> Like this?

Yep

> >From 5842f5768599094758931b74190cdf93641a8e35 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 23 May 2012 12:56:59 -0400
> Subject: [PATCH] xen/hvc: Check HVM_PARAM_CONSOLE_[EVTCHN|PFN] for
>  correctness.
> 
> We need to make sure that those parameters are setup to be correct.
> As such the value of 0 is deemed invalid and we find that we
> bail out. The hypervisor sets by default all of them to be zero
> and when the hypercall is done does a simple:
> 
>  a.value = d->arch.hvm_domain.params[a.index];
> 
> Which means that if the Xen toolstack forgot to setup the proper
> HVM_PARAM_CONSOLE_EVTCHN (or the PFN one), we would get the
> default value of 0 and use that.
> 
> CC: stable@kernel.org
> Fixes-Oracle-Bug: 14091238
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

> ---
>  drivers/tty/hvc/hvc_xen.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 3277f0e..944eaeb 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -214,14 +214,19 @@ static int xen_hvm_console_init(void)
>  	/* already configured */
>  	if (info->intf != NULL)
>  		return 0;
> -
> +	/*
> +	 * If the toolstack (or the hypervisor) hasn't set these values, the
> +	 * default value is 0. Even though mfn = 0 and evtchn = 0 are
> +	 * theoretically correct values, in practice they never are and they
> +	 * mean that a legacy toolstack hasn't initialized the pv console correctly.
> +	 */
>  	r = hvm_get_parameter(HVM_PARAM_CONSOLE_EVTCHN, &v);
> -	if (r < 0)
> +	if (r < 0 || v == 0)
>  		goto err;
>  	info->evtchn = v;
>  	v = 0;
>  	r = hvm_get_parameter(HVM_PARAM_CONSOLE_PFN, &v);
> -	if (r < 0)
> +	if (r < 0 || v == 0)
>  		goto err;
>  	mfn = v;
>  	info->intf = ioremap(mfn << PAGE_SHIFT, PAGE_SIZE);
> -- 
> 1.7.7.5
> 

Re: [Xen-devel] [PATCH 4/4] xen/events: Add WARN_ON when quick lookup found invalid type.

[Stefano Stabellini]

On Thu, 24 May 2012, Konrad Rzeszutek Wilk wrote:
> On Thu, May 24, 2012 at 11:58:11AM +0100, Stefano Stabellini wrote:
> > On Wed, 23 May 2012, Konrad Rzeszutek Wilk wrote:
> > > All of the bind_XYZ_to_irq do a quick lookup to see if the
> > > event exists. And if they that value is returned instead of
> > 
> >                            ^
> 
> ... snip. How about this:
> 
> >From 5963cc2699db7b2893e7925bd2e68ada9d97e8ef Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 23 May 2012 13:28:44 -0400
> Subject: [PATCH] xen/events: Add WARN_ON when quick lookup found invalid
>  type.
> 
> All of the bind_XYZ_to_irq do a quick lookup to see if the
> event exists. And if it does, then the initialized IRQ number
> is returned instead of initializing a new IRQ number.
> 
> This patch adds an extra logic to check that the type returned
> is proper one and that there is an IRQ handler setup for it.
> 
> This patch has the benefit of being able to find drivers that
> are doing something naught.
> 
> [v1: Enhanced based on Stefano's review]
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>  drivers/xen/events.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index faae2f9..81f338a 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -827,6 +827,9 @@ int bind_evtchn_to_irq(unsigned int evtchn)
>  					      handle_edge_irq, "event");
>  
>  		xen_irq_info_evtchn_init(irq, evtchn);
> +	} else {
> +		struct irq_info *info = info_for_irq(irq);
> +		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
>  	}
>  
>  out:
> @@ -862,8 +865,10 @@ static int bind_ipi_to_irq(unsigned int ipi, unsigned int cpu)
>  		xen_irq_info_ipi_init(cpu, irq, evtchn, ipi);
>  
>  		bind_evtchn_to_cpu(evtchn, cpu);
> +	} else {
> +		struct irq_info *info = info_for_irq(irq);
> +		WARN_ON(info == NULL || info->type != IRQT_IPI);
>  	}
> -
>   out:
>  	mutex_unlock(&irq_mapping_update_lock);
>  	return irq;
> @@ -939,8 +944,10 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
>  		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
>  
>  		bind_evtchn_to_cpu(evtchn, cpu);
> +	} else {
> +		struct irq_info *info = info_for_irq(irq);
> +		WARN_ON(info == NULL || info->type != IRQT_VIRQ);
>  	}
> -
>  out:
>  	mutex_unlock(&irq_mapping_update_lock);
>  

I don't want to nitpick but you removed 2 out of 3 spaced before the out
label.
In any case:

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Re: [Xen-devel] [PATCH 4/4] xen/events: Add WARN_ON when quick lookup found invalid type.

[Stefano Stabellini]

On Fri, 25 May 2012, Stefano Stabellini wrote:
> > @@ -939,8 +944,10 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
> >  		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
> >  
> >  		bind_evtchn_to_cpu(evtchn, cpu);
> > +	} else {
> > +		struct irq_info *info = info_for_irq(irq);
> > +		WARN_ON(info == NULL || info->type != IRQT_VIRQ);
> >  	}
> > -
> >  out:
> >  	mutex_unlock(&irq_mapping_update_lock);
> >  
> 
> I don't want to nitpick but you removed 2 out of 3 spaced before the out
> label.

I meant newlines.

> In any case:
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 

Re: [Xen-devel] [PATCH 4/4] xen/events: Add WARN_ON when quick lookup found invalid type.

[Konrad Rzeszutek Wilk]

On Fri, May 25, 2012 at 10:19:10AM +0100, Stefano Stabellini wrote:
> On Fri, 25 May 2012, Stefano Stabellini wrote:
> > > @@ -939,8 +944,10 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
> > >  		xen_irq_info_virq_init(cpu, irq, evtchn, virq);
> > >  
> > >  		bind_evtchn_to_cpu(evtchn, cpu);
> > > +	} else {
> > > +		struct irq_info *info = info_for_irq(irq);
> > > +		WARN_ON(info == NULL || info->type != IRQT_VIRQ);
> > >  	}
> > > -
> > >  out:
> > >  	mutex_unlock(&irq_mapping_update_lock);
> > >  
> > 
> > I don't want to nitpick but you removed 2 out of 3 spaced before the out
> > label.
> 
> I meant newlines.

Good eyes! The final version will have those unmolested.

> 
> > In any case:
> > 
> > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >