thr3ads.net - Xen devel - [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values [Aug 2011]

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

Jan Beulich

2011-Aug-17 08:32 UTC

[Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Notifier functions are expected to return NOTIFY_* codes, not -E...
ones. In particular, since the respective hypercalls failing is not
fatal to the operation of the Dom0 kernel, it must be avoided to
return negative values here as those would make it appear as if
NOTIFY_STOP_MASK wa set, suppressing further notification calls to
other interested parties (which is also why we don''t want to use
notifier_from_errno() here).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 drivers/xen/pci.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

--- 3.1-rc2/drivers/xen/pci.c
+++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
@@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
 			    unsigned long action, void *data)
 {
 	struct device *dev = data;
-	int r = 0;
 
 	switch (action) {
 	case BUS_NOTIFY_ADD_DEVICE:
-		r = xen_add_device(dev);
+		xen_add_device(dev);
 		break;
 	case BUS_NOTIFY_DEL_DEVICE:
-		r = xen_remove_device(dev);
+		xen_remove_device(dev);
 		break;
 	default:
-		break;
+		return NOTIFY_DONE;
 	}
 
-	return r;
+	return NOTIFY_OK;
 }
 
-struct notifier_block device_nb = {
+static struct notifier_block device_nb = {
 	.notifier_call = xen_pci_notifier,
 };
 





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

Konrad Rzeszutek Wilk

2011-Aug-17 14:57 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich
wrote:> Notifier functions are expected to return NOTIFY_* codes, not -E...
> ones. In particular, since the respective hypercalls failing is not
> fatal to the operation of the Dom0 kernel, it must be avoided to
So if we fail adding a PCI device, won''t we be unable to actually
setup its MSI?
> return negative values here as those would make it appear as if
> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
> other interested parties (which is also why we don''t want to use
> notifier_from_errno() here).
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> ---
>  drivers/xen/pci.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> --- 3.1-rc2/drivers/xen/pci.c
> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
>  			    unsigned long action, void *data)
>  {
>  	struct device *dev = data;
> -	int r = 0;
>  
>  	switch (action) {
>  	case BUS_NOTIFY_ADD_DEVICE:
> -		r = xen_add_device(dev);
> +		xen_add_device(dev);
>  		break;
>  	case BUS_NOTIFY_DEL_DEVICE:
> -		r = xen_remove_device(dev);
> +		xen_remove_device(dev);
>  		break;
>  	default:
> -		break;
> +		return NOTIFY_DONE;
>  	}
>  
> -	return r;
> +	return NOTIFY_OK;
>  }
>  
> -struct notifier_block device_nb = {
> +static struct notifier_block device_nb = {
>  	.notifier_call = xen_pci_notifier,
>  };
>  
> 
> 
> 
> Notifier functions are expected to return NOTIFY_* codes, not -E...
> ones. In particular, since the respective hypercalls failing is not
> fatal to the operation of the Dom0 kernel, it must be avoided to
> return negative values here as those would make it appear as if
> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
> other interested parties (which is also why we don''t want to use
> notifier_from_errno() here).
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> ---
>  drivers/xen/pci.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> --- 3.1-rc2/drivers/xen/pci.c
> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
>  			    unsigned long action, void *data)
>  {
>  	struct device *dev = data;
> -	int r = 0;
>  
>  	switch (action) {
>  	case BUS_NOTIFY_ADD_DEVICE:
> -		r = xen_add_device(dev);
> +		xen_add_device(dev);
>  		break;
>  	case BUS_NOTIFY_DEL_DEVICE:
> -		r = xen_remove_device(dev);
> +		xen_remove_device(dev);
>  		break;
>  	default:
> -		break;
> +		return NOTIFY_DONE;
>  	}
>  
> -	return r;
> +	return NOTIFY_OK;
>  }
>  
> -struct notifier_block device_nb = {
> +static struct notifier_block device_nb = {
>  	.notifier_call = xen_pci_notifier,
>  };
>  
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

Jan Beulich

2011-Aug-17 15:18 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

>>> On 17.08.11 at 16:57, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich wrote:
>> Notifier functions are expected to return NOTIFY_* codes, not -E...
>> ones. In particular, since the respective hypercalls failing is not
>> fatal to the operation of the Dom0 kernel, it must be avoided to
> 
> So if we fail adding a PCI device, won''t we be unable to actually
> setup its MSI?
Sure (and you also can''t pass through such a device), but
that''s no
reason to fail the notification chain. For one, you don''t know whether
the driver is actually going to use MSI. And even if you knew, it would
be bad behavior imo. Plus even if you want to fail the notifier chain,
just returning a -E... value here is wrong; notifier_from_errno() ought
to be used then.

Jan
>> return negative values here as those would make it appear as if
>> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
>> other interested parties (which is also why we don''t want to
use
>> notifier_from_errno() here).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> 
>> ---
>>  drivers/xen/pci.c |   11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>> 
>> --- 3.1-rc2/drivers/xen/pci.c
>> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
>> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
>>  			    unsigned long action, void *data)
>>  {
>>  	struct device *dev = data;
>> -	int r = 0;
>>  
>>  	switch (action) {
>>  	case BUS_NOTIFY_ADD_DEVICE:
>> -		r = xen_add_device(dev);
>> +		xen_add_device(dev);
>>  		break;
>>  	case BUS_NOTIFY_DEL_DEVICE:
>> -		r = xen_remove_device(dev);
>> +		xen_remove_device(dev);
>>  		break;
>>  	default:
>> -		break;
>> +		return NOTIFY_DONE;
>>  	}
>>  
>> -	return r;
>> +	return NOTIFY_OK;
>>  }
>>  
>> -struct notifier_block device_nb = {
>> +static struct notifier_block device_nb = {
>>  	.notifier_call = xen_pci_notifier,
>>  };
>>  
>> 
>> 
>> 
> 
>> Notifier functions are expected to return NOTIFY_* codes, not -E...
>> ones. In particular, since the respective hypercalls failing is not
>> fatal to the operation of the Dom0 kernel, it must be avoided to
>> return negative values here as those would make it appear as if
>> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
>> other interested parties (which is also why we don''t want to
use
>> notifier_from_errno() here).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> 
>> ---
>>  drivers/xen/pci.c |   11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>> 
>> --- 3.1-rc2/drivers/xen/pci.c
>> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
>> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
>>  			    unsigned long action, void *data)
>>  {
>>  	struct device *dev = data;
>> -	int r = 0;
>>  
>>  	switch (action) {
>>  	case BUS_NOTIFY_ADD_DEVICE:
>> -		r = xen_add_device(dev);
>> +		xen_add_device(dev);
>>  		break;
>>  	case BUS_NOTIFY_DEL_DEVICE:
>> -		r = xen_remove_device(dev);
>> +		xen_remove_device(dev);
>>  		break;
>>  	default:
>> -		break;
>> +		return NOTIFY_DONE;
>>  	}
>>  
>> -	return r;
>> +	return NOTIFY_OK;
>>  }
>>  
>> -struct notifier_block device_nb = {
>> +static struct notifier_block device_nb = {
>>  	.notifier_call = xen_pci_notifier,
>>  };
>>  
> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com 
>> http://lists.xensource.com/xen-devel 



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

Konrad Rzeszutek Wilk

2011-Aug-17 15:31 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

On Wed, Aug 17, 2011 at 04:18:35PM +0100, Jan Beulich
wrote:> >>> On 17.08.11 at 16:57, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich wrote:
> >> Notifier functions are expected to return NOTIFY_* codes, not
-E...
> >> ones. In particular, since the respective hypercalls failing is
not
> >> fatal to the operation of the Dom0 kernel, it must be avoided to
> > 
> > So if we fail adding a PCI device, won''t we be unable to
actually
> > setup its MSI?
> 
> Sure (and you also can''t pass through such a device), but
that''s no
> reason to fail the notification chain. For one, you don''t know
whether
> the driver is actually going to use MSI. And even if you knew, it would
> be bad behavior imo. Plus even if you want to fail the notifier chain,
> just returning a -E... value here is wrong; notifier_from_errno() ought
> to be used then.
Oh, I am not disputing that. I am just wondering whether we should add
some extra printk''s if we fail, and still reutrn either NOTIFY_OK
oir NOTIFY_DONE.

That way at least in the field we will have a good inkling of what
went wrong.> 
> Jan
> 
> >> return negative values here as those would make it appear as if
> >> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
> >> other interested parties (which is also why we don''t want
to use
> >> notifier_from_errno() here).
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> >> 
> >> ---
> >>  drivers/xen/pci.c |   11 +++++------
> >>  1 file changed, 5 insertions(+), 6 deletions(-)
> >> 
> >> --- 3.1-rc2/drivers/xen/pci.c
> >> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
> >> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
> >>  			    unsigned long action, void *data)
> >>  {
> >>  	struct device *dev = data;
> >> -	int r = 0;
> >>  
> >>  	switch (action) {
> >>  	case BUS_NOTIFY_ADD_DEVICE:
> >> -		r = xen_add_device(dev);
> >> +		xen_add_device(dev);
> >>  		break;
> >>  	case BUS_NOTIFY_DEL_DEVICE:
> >> -		r = xen_remove_device(dev);
> >> +		xen_remove_device(dev);
> >>  		break;
> >>  	default:
> >> -		break;
> >> +		return NOTIFY_DONE;
> >>  	}
> >>  
> >> -	return r;
> >> +	return NOTIFY_OK;
> >>  }
> >>  
> >> -struct notifier_block device_nb = {
> >> +static struct notifier_block device_nb = {
> >>  	.notifier_call = xen_pci_notifier,
> >>  };
> >>  
> >> 
> >> 
> >> 
> > 
> >> Notifier functions are expected to return NOTIFY_* codes, not
-E...
> >> ones. In particular, since the respective hypercalls failing is
not
> >> fatal to the operation of the Dom0 kernel, it must be avoided to
> >> return negative values here as those would make it appear as if
> >> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
> >> other interested parties (which is also why we don''t want
to use
> >> notifier_from_errno() here).
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> >> 
> >> ---
> >>  drivers/xen/pci.c |   11 +++++------
> >>  1 file changed, 5 insertions(+), 6 deletions(-)
> >> 
> >> --- 3.1-rc2/drivers/xen/pci.c
> >> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
> >> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
> >>  			    unsigned long action, void *data)
> >>  {
> >>  	struct device *dev = data;
> >> -	int r = 0;
> >>  
> >>  	switch (action) {
> >>  	case BUS_NOTIFY_ADD_DEVICE:
> >> -		r = xen_add_device(dev);
> >> +		xen_add_device(dev);
> >>  		break;
> >>  	case BUS_NOTIFY_DEL_DEVICE:
> >> -		r = xen_remove_device(dev);
> >> +		xen_remove_device(dev);
> >>  		break;
> >>  	default:
> >> -		break;
> >> +		return NOTIFY_DONE;
> >>  	}
> >>  
> >> -	return r;
> >> +	return NOTIFY_OK;
> >>  }
> >>  
> >> -struct notifier_block device_nb = {
> >> +static struct notifier_block device_nb = {
> >>  	.notifier_call = xen_pci_notifier,
> >>  };
> >>  
> > 
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xensource.com 
> >> http://lists.xensource.com/xen-devel 
> 
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Aug-17 15:37 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

>>> On 17.08.11 at 17:31, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Aug 17, 2011 at 04:18:35PM +0100, Jan Beulich wrote:
>> >>> On 17.08.11 at 16:57, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> > On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich wrote:
>> >> Notifier functions are expected to return NOTIFY_* codes, not
-E...
>> >> ones. In particular, since the respective hypercalls failing
is not
>> >> fatal to the operation of the Dom0 kernel, it must be avoided
to
>> > 
>> > So if we fail adding a PCI device, won''t we be unable to
actually
>> > setup its MSI?
>> 
>> Sure (and you also can''t pass through such a device), but
that''s no
>> reason to fail the notification chain. For one, you don''t know
whether
>> the driver is actually going to use MSI. And even if you knew, it would
>> be bad behavior imo. Plus even if you want to fail the notifier chain,
>> just returning a -E... value here is wrong; notifier_from_errno() ought
>> to be used then.
> 
> Oh, I am not disputing that. I am just wondering whether we should add
> some extra printk''s if we fail, and still reutrn either NOTIFY_OK
> oir NOTIFY_DONE.
> 
> That way at least in the field we will have a good inkling of what
> went wrong.
Ah - I just assumed this being silent is intentional, and didn''t want
to
override whoever decided so. We''re in agreement then that this
could/should change.

Jan


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

Konrad Rzeszutek Wilk

2011-Aug-22 16:20 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich
wrote:> Notifier functions are expected to return NOTIFY_* codes, not -E...
> ones. In particular, since the respective hypercalls failing is not
> fatal to the operation of the Dom0 kernel, it must be avoided to
> return negative values here as those would make it appear as if
> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
> other interested parties (which is also why we don''t want to use
> notifier_from_errno() here).
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> ---
>  drivers/xen/pci.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> --- 3.1-rc2/drivers/xen/pci.c
> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
>  			    unsigned long action, void *data)
>  {
>  	struct device *dev = data;
> -	int r = 0;
>  
>  	switch (action) {
>  	case BUS_NOTIFY_ADD_DEVICE:
> -		r = xen_add_device(dev);
> +		xen_add_device(dev);
>  		break;
>  	case BUS_NOTIFY_DEL_DEVICE:
> -		r = xen_remove_device(dev);
> +		xen_remove_device(dev);
>  		break;
>  	default:
> -		break;
> +		return NOTIFY_DONE;
>  	}
>  
> -	return r;
> +	return NOTIFY_OK;
>  }
>  
> -struct notifier_block device_nb = {
> +static struct notifier_block device_nb = {
>  	.notifier_call = xen_pci_notifier,
>  };
>  
> 
Somehow I lost the email thread.. but what if we did this:

diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
index cef4baf..11e231c 100644
--- a/drivers/xen/pci.c
+++ b/drivers/xen/pci.c
@@ -96,13 +96,19 @@ static int xen_pci_notifier(struct notifier_block *nb,
 		r = xen_remove_device(dev);
 		break;
 	default:
-		break;
+		return NOTIFY_DONE;
 	}
-
-	return r;
+	if (r) {
+		dev_err(dev, "Failed to %s - passthrough or MSI/MSI-X might
fail!\n",
+			action == BUS_NOTIFY_ADD_DEVICE ? "add" :
+			(action == BUS_NOTIFY_DEL_DEVICE ? "delete" :
"unknown"));
+		if (action == BUS_NOTIFY_ADD_DEVICE)
+			pci_no_msi();
+	}
+	return NOTIFY_OK;
 }
 
-struct notifier_block device_nb = {
+static struct notifier_block device_nb = {
 	.notifier_call = xen_pci_notifier,
 };
 

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

Jan Beulich

2011-Aug-23 08:19 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

>>> On 22.08.11 at 18:20, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich wrote:
>> Notifier functions are expected to return NOTIFY_* codes, not -E...
>> ones. In particular, since the respective hypercalls failing is not
>> fatal to the operation of the Dom0 kernel, it must be avoided to
>> return negative values here as those would make it appear as if
>> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
>> other interested parties (which is also why we don''t want to
use
>> notifier_from_errno() here).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> 
>> ---
>>  drivers/xen/pci.c |   11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>> 
>> --- 3.1-rc2/drivers/xen/pci.c
>> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
>> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
>>  			    unsigned long action, void *data)
>>  {
>>  	struct device *dev = data;
>> -	int r = 0;
>>  
>>  	switch (action) {
>>  	case BUS_NOTIFY_ADD_DEVICE:
>> -		r = xen_add_device(dev);
>> +		xen_add_device(dev);
>>  		break;
>>  	case BUS_NOTIFY_DEL_DEVICE:
>> -		r = xen_remove_device(dev);
>> +		xen_remove_device(dev);
>>  		break;
>>  	default:
>> -		break;
>> +		return NOTIFY_DONE;
>>  	}
>>  
>> -	return r;
>> +	return NOTIFY_OK;
>>  }
>>  
>> -struct notifier_block device_nb = {
>> +static struct notifier_block device_nb = {
>>  	.notifier_call = xen_pci_notifier,
>>  };
>>  
>> 
> 
> Somehow I lost the email thread.. but what if we did this:
> 
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index cef4baf..11e231c 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -96,13 +96,19 @@ static int xen_pci_notifier(struct notifier_block *nb,
>  		r = xen_remove_device(dev);
>  		break;
>  	default:
> -		break;
> +		return NOTIFY_DONE;
>  	}
> -
> -	return r;
> +	if (r) {
> +		dev_err(dev, "Failed to %s - passthrough or MSI/MSI-X might
fail!\n",
> +			action == BUS_NOTIFY_ADD_DEVICE ? "add" :
> +			(action == BUS_NOTIFY_DEL_DEVICE ? "delete" :
"unknown"));
This part is fine.
> +		if (action == BUS_NOTIFY_ADD_DEVICE)
> +			pci_no_msi();
But this I''m not sure about: First of all, you''re suggesting
to disable MSI
altogether. That''s very unlikely to be necessary.

Requiring the PCI device to be known to Xen to be able to use MSI is
an implementation detail of the hypervisor. And in fact I had a patch
to remove the call to pci_get_pdev() from map_domain_pirq() (since
setup_msi_irq() doesn''t really consume this parameter). This got made
impossible to do (at least for the moment) with George''s
23753:2e0cf9428554 (and later on the device is getting looked up by
__pci_enable_msi{,x}() anyway, but even that is an implementation
choice, not a hard requirement afaict).

I would view it as quite viable an option to have PHYSDEVOP_map_pirq
implicitly create the device in the MAP_PIRQ_TYPE_MSI case. And hence
I wouldn''t want to disable MSI for the device here.

Jan
> +	}
> +	return NOTIFY_OK;
>  }
>  
> -struct notifier_block device_nb = {
> +static struct notifier_block device_nb = {
>  	.notifier_call = xen_pci_notifier,
>  };
>  



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

Konrad Rzeszutek Wilk

2011-Aug-24 20:42 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

On Tue, Aug 23, 2011 at 09:19:04AM +0100, Jan Beulich
wrote:> >>> On 22.08.11 at 18:20, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich wrote:
> >> Notifier functions are expected to return NOTIFY_* codes, not
-E...
> >> ones. In particular, since the respective hypercalls failing is
not
> >> fatal to the operation of the Dom0 kernel, it must be avoided to
> >> return negative values here as those would make it appear as if
> >> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
> >> other interested parties (which is also why we don''t want
to use
> >> notifier_from_errno() here).
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> >> 
> >> ---
> >>  drivers/xen/pci.c |   11 +++++------
> >>  1 file changed, 5 insertions(+), 6 deletions(-)
> >> 
> >> --- 3.1-rc2/drivers/xen/pci.c
> >> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
> >> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
> >>  			    unsigned long action, void *data)
> >>  {
> >>  	struct device *dev = data;
> >> -	int r = 0;
> >>  
> >>  	switch (action) {
> >>  	case BUS_NOTIFY_ADD_DEVICE:
> >> -		r = xen_add_device(dev);
> >> +		xen_add_device(dev);
> >>  		break;
> >>  	case BUS_NOTIFY_DEL_DEVICE:
> >> -		r = xen_remove_device(dev);
> >> +		xen_remove_device(dev);
> >>  		break;
> >>  	default:
> >> -		break;
> >> +		return NOTIFY_DONE;
> >>  	}
> >>  
> >> -	return r;
> >> +	return NOTIFY_OK;
> >>  }
> >>  
> >> -struct notifier_block device_nb = {
> >> +static struct notifier_block device_nb = {
> >>  	.notifier_call = xen_pci_notifier,
> >>  };
> >>  
> >> 
> > 
> > Somehow I lost the email thread.. but what if we did this:
> > 
> > diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> > index cef4baf..11e231c 100644
> > --- a/drivers/xen/pci.c
> > +++ b/drivers/xen/pci.c
> > @@ -96,13 +96,19 @@ static int xen_pci_notifier(struct notifier_block
*nb,
> >  		r = xen_remove_device(dev);
> >  		break;
> >  	default:
> > -		break;
> > +		return NOTIFY_DONE;
> >  	}
> > -
> > -	return r;
> > +	if (r) {
> > +		dev_err(dev, "Failed to %s - passthrough or MSI/MSI-X might
fail!\n",
> > +			action == BUS_NOTIFY_ADD_DEVICE ? "add" :
> > +			(action == BUS_NOTIFY_DEL_DEVICE ? "delete" :
"unknown"));
> 
> This part is fine.
> 
> > +		if (action == BUS_NOTIFY_ADD_DEVICE)
> > +			pci_no_msi();
> 
> But this I''m not sure about: First of all, you''re
suggesting to disable MSI
> altogether. That''s very unlikely to be necessary.
> 
> Requiring the PCI device to be known to Xen to be able to use MSI is
> an implementation detail of the hypervisor. And in fact I had a patch
> to remove the call to pci_get_pdev() from map_domain_pirq() (since
> setup_msi_irq() doesn''t really consume this parameter). This got
made
> impossible to do (at least for the moment) with George''s
> 23753:2e0cf9428554 (and later on the device is getting looked up by
> __pci_enable_msi{,x}() anyway, but even that is an implementation
> choice, not a hard requirement afaict).
> 
> I would view it as quite viable an option to have PHYSDEVOP_map_pirq
> implicitly create the device in the MAP_PIRQ_TYPE_MSI case. And hence
> I wouldn''t want to disable MSI for the device here.
Ok. let me drop that section and just go with the warning.

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

George Dunlap

2011-Aug-25 09:14 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

On Wed, 2011-08-24 at 21:42 +0100, Konrad Rzeszutek Wilk
wrote:> > Requiring the PCI device to be known to Xen to be able to use MSI is
> > an implementation detail of the hypervisor. And in fact I had a patch
> > to remove the call to pci_get_pdev() from map_domain_pirq() (since
> > setup_msi_irq() doesn''t really consume this parameter). This
got made
> > impossible to do (at least for the moment) with George''s
> > 23753:2e0cf9428554 (and later on the device is getting looked up by
> > __pci_enable_msi{,x}() anyway, but even that is an implementation
> > choice, not a hard requirement afaict).
The device has to be known to Xen in order to do proper interrupt
remapping, does it not?  At least if the AMD per-device interrupt
remapping tables.

I suppose if you really wanted to, we could refactor the code so that
the time Xen needed to know devices for IRQs coincided exactly with when
Xen needs to know devices for IOMMUs.  I don''t understand the
motivation
for such a requirement yet.

 -George


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

Jan Beulich

2011-Aug-25 10:07 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

>>> On 25.08.11 at 11:14, George Dunlap
<george.dunlap@citrix.com> wrote:
> On Wed, 2011-08-24 at 21:42 +0100, Konrad Rzeszutek Wilk wrote:
>> > Requiring the PCI device to be known to Xen to be able to use MSI
is
>> > an implementation detail of the hypervisor. And in fact I had a
patch
>> > to remove the call to pci_get_pdev() from map_domain_pirq() (since
>> > setup_msi_irq() doesn''t really consume this parameter).
This got made
>> > impossible to do (at least for the moment) with George''s
>> > 23753:2e0cf9428554 (and later on the device is getting looked up
by
>> > __pci_enable_msi{,x}() anyway, but even that is an implementation
>> > choice, not a hard requirement afaict).
> 
> The device has to be known to Xen in order to do proper interrupt
> remapping, does it not?  At least if the AMD per-device interrupt
> remapping tables.
Yes, but doesn''t this only affect device passthrough? Further, Xen
does its own (poor man''s) PCI bus scan, so generally it knows of all
devices (at least on segment 0).

Additionally the Dom0 kernel doesn''t even know whether IOMMUs are
actually in use by Xen.

Bottom line - imo still no reason to disable MSI (neither globally nor for
the device).

Jan
> I suppose if you really wanted to, we could refactor the code so that
> the time Xen needed to know devices for IRQs coincided exactly with when
> Xen needs to know devices for IOMMUs.  I don''t understand the
motivation
> for such a requirement yet.
> 
>  -George



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

George Dunlap

2011-Aug-25 10:15 UTC

head link

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

On Thu, 2011-08-25 at 11:07 +0100, Jan Beulich wrote:> >>> On 25.08.11 at 11:14, George Dunlap
<george.dunlap@citrix.com> wrote:
> > On Wed, 2011-08-24 at 21:42 +0100, Konrad Rzeszutek Wilk wrote:
> >> > Requiring the PCI device to be known to Xen to be able to use
MSI is
> >> > an implementation detail of the hypervisor. And in fact I had
a patch
> >> > to remove the call to pci_get_pdev() from map_domain_pirq()
(since
> >> > setup_msi_irq() doesn''t really consume this
parameter). This got made
> >> > impossible to do (at least for the moment) with
George''s
> >> > 23753:2e0cf9428554 (and later on the device is getting looked
up by
> >> > __pci_enable_msi{,x}() anyway, but even that is an
implementation
> >> > choice, not a hard requirement afaict).
> > 
> > The device has to be known to Xen in order to do proper interrupt
> > remapping, does it not?  At least if the AMD per-device interrupt
> > remapping tables.
> 
> Yes, but doesn''t this only affect device passthrough? Further, Xen
> does its own (poor man''s) PCI bus scan, so generally it knows of
all
> devices (at least on segment 0).
>From what I understand, the IOMMU doesn''t know which devices are
beingpassed through; if it''s on, all devices have to be given permissions
for
whatever they need to do.  You can boot with iommu=off, but then you
can''t safely pass any devices through to other VMs, including driver
domains, and you can''t pass devices through to HVM domains at all (no
address translation for DMA).
> Additionally the Dom0 kernel doesn''t even know whether IOMMUs are
> actually in use by Xen.
> 
> Bottom line - imo still no reason to disable MSI (neither globally nor for
> the device).
I have no idea about this.  I was just commenting about my c/s requiring
Xen to know about PCI devices (which is I assume why you cc''d me). :-)

 -George


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

Xen devel - Aug 2011 - [PATCH] xen/pci: make bus notifier handler return sane values

[Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values