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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 12.06.15 at 22:57, <konrad.wilk@oracle.com> 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 (upstream & XenClassic)
> code when doing XEN_PCI_OP_enable_msix can stash the -EXX in op->result
> and in op->err, but they are also the only ones implementing this
> operation.
> 
> Here is how it works right now with the XEN_PCI_OP:

>From here on, other than said above, you appear to talk about
frontend behavior. This should be made explicit.

> - XEN_PCI_OP_conf_read and XEN_PCI_OP_conf_write
>   it expects 'err' to contain XEN_PCI_ERR* values. And it converts them
>   as it sees fit to -Exx.
>   Note that NetBSD only implements XEN_PCI_OP_conf_write and
>   XEN_PCI_OP_conf_read.
> 
> - For XEN_PCI_OP_enable_msi if 'err' has any value it will convert
>   all of them to -EINVAL (Linux).
> 
> - For XEN_PCI_OP_disable_msix and XEN_PCI_OP_disable_msi it just
>   reports the value (printk) and discards the 'err'.
> 
> - The XEN_PCI_OP_enable_msix differs on the frontend (classic Linux
>   vs upstream).
>   In Linux classic, if 'err' has any value it will convert all of them
>   to '-EINVAL'.
>   In Linux upstream it will convert the 'err' to uint32_t and pass it
>   back up (to 'pci_enable_msi_range'). However due to the casting
>   errors it ends up being 0xffffffffa (or such) and is useless.
> 
>   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 or ignores it.
> 
> Which means this patch will not break existing implementations and mandating
> op->err to use XEN_PCI_ERR_* and stick in op->result -EXX if the
> opcode wants it is the step in the right direction.

Albeit you realize that passing -E... values here is bogus anyway.
If anything, this should be -XEN_E..., so that their values don't
vary by OS.

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

The comment (apart from being badly formatted) is still too vague
to be of any use to the reader. Plus I think references to other
fields in the structure should either quote the field name or add
"field" after the name.

Jan