[PATCH] xen/pciif: Clarify what values go in op->err and op->result.

[PATCH] xen/pciif: Clarify what values go in op->err and op->result.

[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

Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.

[Konrad Rzeszutek Wilk]

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
> 

Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.

[Ian Campbell]

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;

Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.

[Konrad Rzeszutek Wilk]

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

Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.

[Ian Campbell]

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?

Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result.

[Konrad Rzeszutek Wilk]

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