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

[PATCH v2] 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 (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:

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

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Update the commit with the discovery.
---
 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 v2] xen/pciif: Clarify what values go in op->err and op->result.

[Jan Beulich]

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

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

[Konrad Rzeszutek Wilk]

On June 15, 2015 5:45:09 AM EDT, Jan Beulich <JBeulich@suse.com> wrote:
>>>> 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.

Yes, thank you.
>
>> - 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.

Ah, hadn't realized we have made an public -XEN_Exx defines, will use that.

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

OK, will be more explicit.
>
>Jan