* [PATCH] xen/pciif: Clarify what values go in op->err and op->result.
@ 2015-03-31 14:58 Konrad Rzeszutek Wilk
2015-03-31 15:27 ` Konrad Rzeszutek Wilk
2015-03-31 16:05 ` Ian Campbell
0 siblings, 2 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-03-31 14:58 UTC (permalink / raw)
To: ian.campbell, ian.jackson, jbeulich, keir, tim, xen-devel
Cc: Konrad Rzeszutek Wilk
The earlier comment says that errno values go in op->err.
However all implementations (NetBSD, Linux) of the most
common operations use XEN_PCI_ERR_* instead of -EXX values.
The exception is the xen-pciback in Linux code when doing
XEN_PCI_OP_enable_msix can stash the -EXX in op->result
and in op->err.
As such lets clarify what '->err' and '->result' are
suppose to contain.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/include/public/io/pciif.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/xen/include/public/io/pciif.h b/xen/include/public/io/pciif.h
index a4ba13c..535963a 100644
--- a/xen/include/public/io/pciif.h
+++ b/xen/include/public/io/pciif.h
@@ -71,7 +71,7 @@ struct xen_pci_op {
/* IN: what action to perform: XEN_PCI_OP_* */
uint32_t cmd;
- /* OUT: will contain an error number (if any) from errno.h */
+ /* OUT: will contain an XEN_PCI_ERR_* value. */
int32_t err;
/* IN: which device to touch */
@@ -83,7 +83,9 @@ struct xen_pci_op {
int32_t offset;
int32_t size;
- /* IN/OUT: Contains the result after a READ or the value to WRITE */
+ /* IN/OUT: Contains the result after a READ or the value to WRITE.
+ * If the err does not have XEN_PCI_ERR_success, depending on
+ * XEN_PCI_OP_* might have the errno value. */
uint32_t value;
/* IN: Contains extra infor for this operation */
uint32_t info;
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.
2015-03-31 14:58 [PATCH] xen/pciif: Clarify what values go in op->err and op->result Konrad Rzeszutek Wilk
@ 2015-03-31 15:27 ` Konrad Rzeszutek Wilk
2015-03-31 16:05 ` Ian Campbell
1 sibling, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-03-31 15:27 UTC (permalink / raw)
To: ian.campbell, ian.jackson, jbeulich, keir, tim, xen-devel
On Tue, Mar 31, 2015 at 10:58:32AM -0400, Konrad Rzeszutek Wilk wrote:
> The earlier comment says that errno values go in op->err.
> However all implementations (NetBSD, Linux) of the most
> common operations use XEN_PCI_ERR_* instead of -EXX values.
>
> The exception is the xen-pciback in Linux code when doing
And also linux-2.6.18.hg.
> XEN_PCI_OP_enable_msix can stash the -EXX in op->result
> and in op->err.
>
> As such lets clarify what '->err' and '->result' are
> suppose to contain.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> xen/include/public/io/pciif.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/xen/include/public/io/pciif.h b/xen/include/public/io/pciif.h
> index a4ba13c..535963a 100644
> --- a/xen/include/public/io/pciif.h
> +++ b/xen/include/public/io/pciif.h
> @@ -71,7 +71,7 @@ struct xen_pci_op {
> /* IN: what action to perform: XEN_PCI_OP_* */
> uint32_t cmd;
>
> - /* OUT: will contain an error number (if any) from errno.h */
> + /* OUT: will contain an XEN_PCI_ERR_* value. */
> int32_t err;
>
> /* IN: which device to touch */
> @@ -83,7 +83,9 @@ struct xen_pci_op {
> int32_t offset;
> int32_t size;
>
> - /* IN/OUT: Contains the result after a READ or the value to WRITE */
> + /* IN/OUT: Contains the result after a READ or the value to WRITE.
> + * If the err does not have XEN_PCI_ERR_success, depending on
> + * XEN_PCI_OP_* might have the errno value. */
> uint32_t value;
> /* IN: Contains extra infor for this operation */
> uint32_t info;
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.
2015-03-31 14:58 [PATCH] xen/pciif: Clarify what values go in op->err and op->result Konrad Rzeszutek Wilk
2015-03-31 15:27 ` Konrad Rzeszutek Wilk
@ 2015-03-31 16:05 ` Ian Campbell
2015-03-31 16:29 ` Konrad Rzeszutek Wilk
1 sibling, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2015-03-31 16:05 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: keir, xen-devel, ian.jackson, jbeulich, tim
On Tue, 2015-03-31 at 10:58 -0400, Konrad Rzeszutek Wilk wrote:
> The earlier comment says that errno values go in op->err.
> However all implementations (NetBSD, Linux) of the most
> common operations use XEN_PCI_ERR_* instead of -EXX values.
>
> The exception is the xen-pciback in Linux code when doing
> XEN_PCI_OP_enable_msix can stash the -EXX in op->result
> and in op->err.
i.e. both of them contain the same thing? How unhelpful!
What would be the impact of "correcting" ->result to do the right thing?
(as documented below after this patch).
>
> As such lets clarify what '->err' and '->result' are
> suppose to contain.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> xen/include/public/io/pciif.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/xen/include/public/io/pciif.h b/xen/include/public/io/pciif.h
> index a4ba13c..535963a 100644
> --- a/xen/include/public/io/pciif.h
> +++ b/xen/include/public/io/pciif.h
> @@ -71,7 +71,7 @@ struct xen_pci_op {
> /* IN: what action to perform: XEN_PCI_OP_* */
> uint32_t cmd;
>
> - /* OUT: will contain an error number (if any) from errno.h */
> + /* OUT: will contain an XEN_PCI_ERR_* value. */
> int32_t err;
>
> /* IN: which device to touch */
> @@ -83,7 +83,9 @@ struct xen_pci_op {
> int32_t offset;
> int32_t size;
>
> - /* IN/OUT: Contains the result after a READ or the value to WRITE */
> + /* IN/OUT: Contains the result after a READ or the value to WRITE.
> + * If the err does not have XEN_PCI_ERR_success, depending on
s/the err does not have/err is not/
> + * XEN_PCI_OP_* might have the errno value. */
might under what circumstances? Can that be documented (perhaps as a
default here and a small number of exceptions?)
> uint32_t value;
> /* IN: Contains extra infor for this operation */
> uint32_t info;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.
2015-03-31 16:05 ` Ian Campbell
@ 2015-03-31 16:29 ` Konrad Rzeszutek Wilk
2015-04-15 16:05 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-03-31 16:29 UTC (permalink / raw)
To: Ian Campbell; +Cc: keir, xen-devel, ian.jackson, jbeulich, tim
On Tue, Mar 31, 2015 at 05:05:23PM +0100, Ian Campbell wrote:
> On Tue, 2015-03-31 at 10:58 -0400, Konrad Rzeszutek Wilk wrote:
> > The earlier comment says that errno values go in op->err.
> > However all implementations (NetBSD, Linux) of the most
> > common operations use XEN_PCI_ERR_* instead of -EXX values.
> >
> > The exception is the xen-pciback in Linux code when doing
> > XEN_PCI_OP_enable_msix can stash the -EXX in op->result
> > and in op->err.
>
> i.e. both of them contain the same thing? How unhelpful!
>
> What would be the impact of "correcting" ->result to do the right thing?
> (as documented below after this patch).
Ugh. The frontend (Linux) first checks op->err. If it is non-zero
then it returns op->err back up. If op->err is zero
but op->result is non-zero, then it returns op->result up the
stack.
The 'stack' differs depending on what XEN_PCI_OP it is.
For XEN_PCI_OP_conf_read and XEN_PCI_OP_conf_write
it expects 'err' to contain XEN_PCI_ERR* values. And it converts them.
In upstream Linux:
The XEN_PCI_OP_enable_msix it expects 'err' to contain
-EXX values. Which means that whoever called 'pci_enable_msi_range' will
get the 'err' value.
In Linux 2.6.18, if 'err' has any value it will convert all of them
to '-EINVAL'.
For XEN_PCI_OP_enable_msi if 'err' has any value it will convert
all of them to -EINVAL.
For XEN_PCI_OP_disable_msix and XEN_PCI_OP_disable_msi it just
reports the value.
NetBSD only implements XEN_PCI_OP_conf_write and XEN_PCI_OP_conf_read.
It looks to me that the upstream Linux kernel frontend driver needs
to do what the linux-2.6.18 does (return -EINVAL if there are any errors).
>
> >
> > As such lets clarify what '->err' and '->result' are
> > suppose to contain.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> > xen/include/public/io/pciif.h | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/include/public/io/pciif.h b/xen/include/public/io/pciif.h
> > index a4ba13c..535963a 100644
> > --- a/xen/include/public/io/pciif.h
> > +++ b/xen/include/public/io/pciif.h
> > @@ -71,7 +71,7 @@ struct xen_pci_op {
> > /* IN: what action to perform: XEN_PCI_OP_* */
> > uint32_t cmd;
> >
> > - /* OUT: will contain an error number (if any) from errno.h */
> > + /* OUT: will contain an XEN_PCI_ERR_* value. */
> > int32_t err;
> >
> > /* IN: which device to touch */
> > @@ -83,7 +83,9 @@ struct xen_pci_op {
> > int32_t offset;
> > int32_t size;
> >
> > - /* IN/OUT: Contains the result after a READ or the value to WRITE */
> > + /* IN/OUT: Contains the result after a READ or the value to WRITE.
> > + * If the err does not have XEN_PCI_ERR_success, depending on
>
> s/the err does not have/err is not/
>
> > + * XEN_PCI_OP_* might have the errno value. */
>
> might under what circumstances? Can that be documented (perhaps as a
> default here and a small number of exceptions?)
>
> > uint32_t value;
> > /* IN: Contains extra infor for this operation */
> > uint32_t info;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.
2015-03-31 16:29 ` Konrad Rzeszutek Wilk
@ 2015-04-15 16:05 ` Ian Campbell
2015-04-15 18:01 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2015-04-15 16:05 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: ian.jackson, tim, keir, jbeulich, xen-devel
On Tue, 2015-03-31 at 12:29 -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 31, 2015 at 05:05:23PM +0100, Ian Campbell wrote:
> > On Tue, 2015-03-31 at 10:58 -0400, Konrad Rzeszutek Wilk wrote:
> > > The earlier comment says that errno values go in op->err.
> > > However all implementations (NetBSD, Linux) of the most
> > > common operations use XEN_PCI_ERR_* instead of -EXX values.
> > >
> > > The exception is the xen-pciback in Linux code when doing
> > > XEN_PCI_OP_enable_msix can stash the -EXX in op->result
> > > and in op->err.
> >
> > i.e. both of them contain the same thing? How unhelpful!
> >
> > What would be the impact of "correcting" ->result to do the right thing?
> > (as documented below after this patch).
>
> Ugh. The frontend (Linux) first checks op->err. If it is non-zero
> then it returns op->err back up. If op->err is zero
> but op->result is non-zero, then it returns op->result up the
> stack.
>
> The 'stack' differs depending on what XEN_PCI_OP it is.
>
> For XEN_PCI_OP_conf_read and XEN_PCI_OP_conf_write
> it expects 'err' to contain XEN_PCI_ERR* values. And it converts them.
>
> In upstream Linux:
> The XEN_PCI_OP_enable_msix it expects 'err' to contain
> -EXX values. Which means that whoever called 'pci_enable_msi_range' will
> get the 'err' value.
>
> In Linux 2.6.18, if 'err' has any value it will convert all of them
> to '-EINVAL'.
>
> For XEN_PCI_OP_enable_msi if 'err' has any value it will convert
> all of them to -EINVAL.
>
> For XEN_PCI_OP_disable_msix and XEN_PCI_OP_disable_msi it just
> reports the value.
>
> NetBSD only implements XEN_PCI_OP_conf_write and XEN_PCI_OP_conf_read.
>
> It looks to me that the upstream Linux kernel frontend driver needs
> to do what the linux-2.6.18 does (return -EINVAL if there are any errors).
So what are the next steps? Patches to other things? What about this
one, should I expect a new version, drop it or apply it?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.
2015-04-15 16:05 ` Ian Campbell
@ 2015-04-15 18:01 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-04-15 18:01 UTC (permalink / raw)
To: Ian Campbell; +Cc: ian.jackson, tim, keir, jbeulich, xen-devel
On Wed, Apr 15, 2015 at 05:05:16PM +0100, Ian Campbell wrote:
> On Tue, 2015-03-31 at 12:29 -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Mar 31, 2015 at 05:05:23PM +0100, Ian Campbell wrote:
> > > On Tue, 2015-03-31 at 10:58 -0400, Konrad Rzeszutek Wilk wrote:
> > > > The earlier comment says that errno values go in op->err.
> > > > However all implementations (NetBSD, Linux) of the most
> > > > common operations use XEN_PCI_ERR_* instead of -EXX values.
> > > >
> > > > The exception is the xen-pciback in Linux code when doing
> > > > XEN_PCI_OP_enable_msix can stash the -EXX in op->result
> > > > and in op->err.
> > >
> > > i.e. both of them contain the same thing? How unhelpful!
> > >
> > > What would be the impact of "correcting" ->result to do the right thing?
> > > (as documented below after this patch).
> >
> > Ugh. The frontend (Linux) first checks op->err. If it is non-zero
> > then it returns op->err back up. If op->err is zero
> > but op->result is non-zero, then it returns op->result up the
> > stack.
> >
> > The 'stack' differs depending on what XEN_PCI_OP it is.
> >
> > For XEN_PCI_OP_conf_read and XEN_PCI_OP_conf_write
> > it expects 'err' to contain XEN_PCI_ERR* values. And it converts them.
> >
> > In upstream Linux:
> > The XEN_PCI_OP_enable_msix it expects 'err' to contain
> > -EXX values. Which means that whoever called 'pci_enable_msi_range' will
> > get the 'err' value.
> >
> > In Linux 2.6.18, if 'err' has any value it will convert all of them
> > to '-EINVAL'.
> >
> > For XEN_PCI_OP_enable_msi if 'err' has any value it will convert
> > all of them to -EINVAL.
> >
> > For XEN_PCI_OP_disable_msix and XEN_PCI_OP_disable_msi it just
> > reports the value.
> >
> > NetBSD only implements XEN_PCI_OP_conf_write and XEN_PCI_OP_conf_read.
> >
> > It looks to me that the upstream Linux kernel frontend driver needs
> > to do what the linux-2.6.18 does (return -EINVAL if there are any errors).
>
> So what are the next steps? Patches to other things? What about this
> one, should I expect a new version, drop it or apply it?
I neglected to mention that the upstream frontend driver will end
up converting an uint32_t to int, with the end result that the
error is actually 0xffffffffa (or such).
Which means that it really does not matter what (-EXX or XEN_PCI_ERR_*)
or where (op->err or op->result) the backend stashes it as the frontend
screws it up.
Which makes me comfortable in proposing this patch that mandates
op->err to use XEN_PCI_ERR_* and stick in op->result -EXX if the
opcode wants it.
It won't affect how the frontend deals with it as with even that
change it will still return 0xfffff.. on failures.
In short, I will repost this patch and include this long rant in it.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-04-15 18:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-31 14:58 [PATCH] xen/pciif: Clarify what values go in op->err and op->result Konrad Rzeszutek Wilk
2015-03-31 15:27 ` Konrad Rzeszutek Wilk
2015-03-31 16:05 ` Ian Campbell
2015-03-31 16:29 ` Konrad Rzeszutek Wilk
2015-04-15 16:05 ` Ian Campbell
2015-04-15 18:01 ` Konrad Rzeszutek Wilk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.