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

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

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