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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel