From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH] xen/pciif: Clarify what values go in op->err and op->result. Date: Tue, 31 Mar 2015 12:29:07 -0400 Message-ID: <20150331162907.GA15146@l.oracle.com> References: <1427813912-6593-1-git-send-email-konrad.wilk@oracle.com> <1427817923.2115.177.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1427817923.2115.177.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: keir@xen.org, xen-devel@lists.xen.org, ian.jackson@eu.citrix.com, jbeulich@suse.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org 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 > > --- > > 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; > >