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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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