| Message ID | 1468352205-9137-3-git-send-email-aditr@vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > This patch enables posting Verb requests and receiving responses to/from > the backend PVRDMA emulation layer. > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > Reviewed-by: George Zhang <georgezhang@vmware.com> > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > Reviewed-by: Bryan Tan <bryantan@vmware.com> > Signed-off-by: Adit Ranadive <aditr@vmware.com> > --- > drivers/infiniband/hw/pvrdma/pvrdma_cmd.c | 104 ++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_cmd.c > > diff --git a/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c > new file mode 100644 > index 0000000..c22debf > --- /dev/null > +++ b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c > @@ -0,0 +1,104 @@ > +/* > + * Copyright (c) 2012-2016 VMware, Inc. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of EITHER the GNU General Public License > + * version 2 as published by the Free Software Foundation or the BSD > + * 2-Clause License. This program is distributed in the hope that it > + * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED > + * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. > + * See the GNU General Public License version 2 for more details at > + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program available in the file COPYING in the main > + * directory of this source tree. > + * > + * The BSD 2-Clause License > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * - Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the following > + * disclaimer. > + * > + * - Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > + * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR > + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, > + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED > + * OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include <linux/list.h> > + > +#include "pvrdma.h" > + > +#define PVRDMA_CMD_TIMEOUT 10000 /* ms */ > + > +static int pvrdma_cmd_recv(struct pvrdma_dev *dev, union pvrdma_cmd_resp *resp) > +{ > + dev_dbg(&dev->pdev->dev, "receive response from device\n"); > + > + spin_lock(&dev->cmd_lock); > + memcpy(resp, dev->resp_slot, sizeof(*resp)); > + spin_unlock(&dev->cmd_lock); > + > + return 0; > +} > + > +int > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > + bool expect_resp, union pvrdma_cmd_resp *resp) > +{ > + int err; > + > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > + > + /* Serializiation */ > + down(&dev->cmd_sema); > + > + spin_lock(&dev->cmd_lock); > + memcpy(dev->cmd_slot, req, sizeof(*req)); Just to protect against memory corruption suggesting to do: memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > + spin_unlock(&dev->cmd_lock); > + > + init_completion(&dev->cmd_done); > + pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0); > + > + /* Make sure the request is written before reading status. */ > + mb(); > + err = pvrdma_read_reg(dev, PVRDMA_REG_ERR); > + if (err == 0) { > + if (expect_resp) { How about utilizing resp instead of extra argument. This will save you for example from the local variable rsp in function pvrdma_destroy_qp (patch #10) > + err = wait_for_completion_interruptible_timeout( > + &dev->cmd_done, > + msecs_to_jiffies(PVRDMA_CMD_TIMEOUT)); > + if (err == 0) { > + dev_warn(&dev->pdev->dev, > + "completion timeout\n"); Is this really a warning only? i.e. i assume some high level flow is break now. > + err = -ETIMEDOUT; > + } else { > + err = pvrdma_cmd_recv(dev, resp); > + } > + } > + } else { > + dev_warn(&dev->pdev->dev, "failed to write request %d\n", err); > + } > + > + up(&dev->cmd_sema); > + > + return err; > +} > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > This patch enables posting Verb requests and receiving responses to/from > the backend PVRDMA emulation layer. > > Reviewed-by: Jorgen Hansen <jhansen@vmware.com> > Reviewed-by: George Zhang <georgezhang@vmware.com> > Reviewed-by: Aditya Sarwade <asarwade@vmware.com> > Reviewed-by: Bryan Tan <bryantan@vmware.com> > Signed-off-by: Adit Ranadive <aditr@vmware.com> > --- > drivers/infiniband/hw/pvrdma/pvrdma_cmd.c | 104 ++++++++++++++++++++++++++++++ > 1 file changed, 104 insertions(+) > create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_cmd.c > > diff --git a/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c > new file mode 100644 > index 0000000..c22debf > --- /dev/null > +++ b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c > @@ -0,0 +1,104 @@ > +/* > + * Copyright (c) 2012-2016 VMware, Inc. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of EITHER the GNU General Public License > + * version 2 as published by the Free Software Foundation or the BSD > + * 2-Clause License. This program is distributed in the hope that it > + * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED > + * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. > + * See the GNU General Public License version 2 for more details at > + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program available in the file COPYING in the main > + * directory of this source tree. > + * > + * The BSD 2-Clause License > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * - Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the following > + * disclaimer. > + * > + * - Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS > + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE > + * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR > + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, > + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED > + * OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include <linux/list.h> > + > +#include "pvrdma.h" > + > +#define PVRDMA_CMD_TIMEOUT 10000 /* ms */ > + > +static int pvrdma_cmd_recv(struct pvrdma_dev *dev, union pvrdma_cmd_resp *resp) > +{ > + dev_dbg(&dev->pdev->dev, "receive response from device\n"); > + > + spin_lock(&dev->cmd_lock); > + memcpy(resp, dev->resp_slot, sizeof(*resp)); > + spin_unlock(&dev->cmd_lock); In case your virtual HW supports more than one concurrent commands, if cmd_lock protects cmd_slot i would expect to have extra resp_lock to protect resp_slot. btw, otherwise why not just lock the entire flow: memcpy(dev->cmd_slot, req, sizeof(*req)); pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0); pvrdma_read_reg(dev, PVRDMA_REG_ERR); memcpy(resp, dev->resp_slot, sizeof(*resp)); This might improve performances. > + > + return 0; > +} > + > +int > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > + bool expect_resp, union pvrdma_cmd_resp *resp) > +{ > + int err; > + > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > + > + /* Serializiation */ > + down(&dev->cmd_sema); > + > + spin_lock(&dev->cmd_lock); > + memcpy(dev->cmd_slot, req, sizeof(*req)); > + spin_unlock(&dev->cmd_lock); > + > + init_completion(&dev->cmd_done); > + pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0); > + > + /* Make sure the request is written before reading status. */ > + mb(); > + err = pvrdma_read_reg(dev, PVRDMA_REG_ERR); > + if (err == 0) { > + if (expect_resp) { > + err = wait_for_completion_interruptible_timeout( > + &dev->cmd_done, > + msecs_to_jiffies(PVRDMA_CMD_TIMEOUT)); > + if (err == 0) { > + dev_warn(&dev->pdev->dev, > + "completion timeout\n"); > + err = -ETIMEDOUT; > + } else { > + err = pvrdma_cmd_recv(dev, resp); Maybe it is a silly question but in case of two commands that issued almost simultaneously, it possible that fast execution of second command will reach this point first and the reasponse here is of the first one? > + } > + } > + } else { > + dev_warn(&dev->pdev->dev, "failed to write request %d\n", err); > + } > + > + up(&dev->cmd_sema); > + > + return err; > +} > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/18/16 6:13 AM, Yuval Shaia wrote: >> + >> +static int pvrdma_cmd_recv(struct pvrdma_dev *dev, union pvrdma_cmd_resp *resp) >> +{ >> + dev_dbg(&dev->pdev->dev, "receive response from device\n"); >> + >> + spin_lock(&dev->cmd_lock); >> + memcpy(resp, dev->resp_slot, sizeof(*resp)); >> + spin_unlock(&dev->cmd_lock); > > In case your virtual HW supports more than one concurrent commands, if > cmd_lock protects cmd_slot i would expect to have extra resp_lock to > protect resp_slot. Thanks for the suggestion! Currently, our virtual hardware only supports processing one command at a time. I'll skip adding the resp->lock since it may not add much value here. > btw, otherwise why not just lock the entire flow: > memcpy(dev->cmd_slot, req, sizeof(*req)); > pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0); > pvrdma_read_reg(dev, PVRDMA_REG_ERR); > memcpy(resp, dev->resp_slot, sizeof(*resp)); > > This might improve performances. > Since this is in the slow control path and we execute 1 command at a time, not sure if that would be beneficial. >> + >> + return 0; >> +} >> + >> +int >> +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, >> + bool expect_resp, union pvrdma_cmd_resp *resp) >> +{ >> + int err; >> + >> + dev_dbg(&dev->pdev->dev, "post request to device\n"); >> + >> + /* Serializiation */ >> + down(&dev->cmd_sema); >> + >> + spin_lock(&dev->cmd_lock); >> + memcpy(dev->cmd_slot, req, sizeof(*req)); >> + spin_unlock(&dev->cmd_lock); >> + >> + init_completion(&dev->cmd_done); >> + pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0); >> + >> + /* Make sure the request is written before reading status. */ >> + mb(); >> + err = pvrdma_read_reg(dev, PVRDMA_REG_ERR); >> + if (err == 0) { >> + if (expect_resp) { >> + err = wait_for_completion_interruptible_timeout( >> + &dev->cmd_done, >> + msecs_to_jiffies(PVRDMA_CMD_TIMEOUT)); >> + if (err == 0) { >> + dev_warn(&dev->pdev->dev, >> + "completion timeout\n"); >> + err = -ETIMEDOUT; >> + } else { >> + err = pvrdma_cmd_recv(dev, resp); > > Maybe it is a silly question but in case of two commands that issued almost > simultaneously, it possible that fast execution of second command will > reach this point first and the reasponse here is of the first one? > I don't think that may happen since we do have a semaphore here to protect that. Thanks, Adit -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia@oracle.com> wrote: > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > > This patch enables posting Verb requests and receiving responses to/from > > the backend PVRDMA emulation layer. > > ... > > +int > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > > + bool expect_resp, union pvrdma_cmd_resp *resp) > > +{ > > + int err; > > + > > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > > + > > + /* Serializiation */ > > + down(&dev->cmd_sema); > > + > > + spin_lock(&dev->cmd_lock); > > + memcpy(dev->cmd_slot, req, sizeof(*req)); > > Just to protect against memory corruption suggesting to do: > memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > Done. > > + spin_unlock(&dev->cmd_lock); > > + > > + init_completion(&dev->cmd_done); > > + pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0); > > + > > + /* Make sure the request is written before reading status. */ > > + mb(); > > + err = pvrdma_read_reg(dev, PVRDMA_REG_ERR); > > + if (err == 0) { > > + if (expect_resp) { > > How about utilizing resp instead of extra argument. > This will save you for example from the local variable rsp in function > pvrdma_destroy_qp (patch #10) > Ok. Will remove the extra argument. > > + err = wait_for_completion_interruptible_timeout( > > + &dev->cmd_done, > > + msecs_to_jiffies(PVRDMA_CMD_TIMEOUT)); > > + if (err == 0) { > > + dev_warn(&dev->pdev->dev, > > + "completion timeout\n"); > > Is this really a warning only? i.e. i assume some high level flow is > break now. > I have changed this to a dev_err. Thanks, Adit -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote: > On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia@oracle.com> wrote: > > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > > > This patch enables posting Verb requests and receiving responses to/from > > > the backend PVRDMA emulation layer. > > > > > ... > > > > +int > > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > > > + bool expect_resp, union pvrdma_cmd_resp *resp) > > > +{ > > > + int err; > > > + > > > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > > > + > > > + /* Serializiation */ > > > + down(&dev->cmd_sema); > > > + > > > + spin_lock(&dev->cmd_lock); > > > + memcpy(dev->cmd_slot, req, sizeof(*req)); > > > > Just to protect against memory corruption suggesting to do: > > memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > > > > Done. Hi Adit, I didn't check how "dev->cmd_slot" was declared and if it is equal to req in size. Additionally I didn't check if memcpy has any size limitations, but wanted to warn you that implementation of this suggestion as is will leave your system in much worse situation of partially copied data. if you really want to check sizes, please do it with BUILD_ON compilation macro. I have serious doubts if you need it. Thanks
On Fri, 29 Jul 2016 12:37:29 +0000, Leon Romanovsky <leon@kernel.org> wrote: > On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote: > > On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia@oracle.com> wrote: > > > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > > > > This patch enables posting Verb requests and receiving responses to/from > > > > the backend PVRDMA emulation layer. > > > > > > > > ... > > > > > > +int > > > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > > > > + bool expect_resp, union pvrdma_cmd_resp *resp) > > > > +{ > > > > + int err; > > > > + > > > > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > > > > + > > > > + /* Serializiation */ > > > > + down(&dev->cmd_sema); > > > > + > > > > + spin_lock(&dev->cmd_lock); > > > > + memcpy(dev->cmd_slot, req, sizeof(*req)); > > > > > > Just to protect against memory corruption suggesting to do: > > > memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > > > > > > > Done. > > Hi Adit, > I didn't check how "dev->cmd_slot" was declared and if it is equal to > req in size. Additionally I didn't check if memcpy has any size > limitations, but wanted to warn you that implementation of this > suggestion as is will leave your system in much worse situation of > partially copied data. > > if you really want to check sizes, please do it with BUILD_ON compilation > macro. I have serious doubts if you need it. > > Thanks Hi Leon, Thanks for taking a look! I think this is okay to do was that we allocate a page for cmd_slot anyways and the size of req will always be smaller than PAGE_SIZE. If the request is malformed, our virtual hardware should throw an error for it. I dont think we would run into partially copied data. Having said that, I dont see a necessary advantage of using min here so am willing to drop it if you feel strongly about it. Can you clarify what you mean by memcpy size limitations? Thanks, Adit -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 29, 2016 at 09:31:16PM +0000, Adit Ranadive wrote: > On Fri, 29 Jul 2016 12:37:29 +0000, Leon Romanovsky <leon@kernel.org> wrote: > > On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote: > > > On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia@oracle.com> wrote: > > > > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > > > > > This patch enables posting Verb requests and receiving responses to/from > > > > > the backend PVRDMA emulation layer. > > > > > > > > > > > ... > > > > > > > > +int > > > > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > > > > > + bool expect_resp, union pvrdma_cmd_resp *resp) > > > > > +{ > > > > > + int err; > > > > > + > > > > > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > > > > > + > > > > > + /* Serializiation */ > > > > > + down(&dev->cmd_sema); > > > > > + > > > > > + spin_lock(&dev->cmd_lock); > > > > > + memcpy(dev->cmd_slot, req, sizeof(*req)); > > > > > > > > Just to protect against memory corruption suggesting to do: > > > > memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > > > > > > > > > > Done. > > > > Hi Adit, > > I didn't check how "dev->cmd_slot" was declared and if it is equal to > > req in size. Additionally I didn't check if memcpy has any size > > limitations, but wanted to warn you that implementation of this > > suggestion as is will leave your system in much worse situation of > > partially copied data. > > > > if you really want to check sizes, please do it with BUILD_ON compilation > > macro. I have serious doubts if you need it. > > > > Thanks > > Hi Leon, > > Thanks for taking a look! > I think this is okay to do was that we allocate a page for cmd_slot anyways > and the size of req will always be smaller than PAGE_SIZE. If the request is > malformed, our virtual hardware should throw an error for it. > I dont think we would run into partially copied data. > > Having said that, I dont see a necessary advantage of using min here so am > willing to drop it if you feel strongly about it. > > Can you clarify what you mean by memcpy size limitations? I didn't have time to look after reasons for Yuval's comment about min(..), so I assumed that one of the possibilities will be limitation of memcpy which I'm not aware. > > Thanks, > Adit
On Sun, Jul 31, 2016 at 09:15:47AM +0300, Leon Romanovsky wrote: > On Fri, Jul 29, 2016 at 09:31:16PM +0000, Adit Ranadive wrote: > > On Fri, 29 Jul 2016 12:37:29 +0000, Leon Romanovsky <leon@kernel.org> wrote: > > > On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote: > > > > On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia@oracle.com> wrote: > > > > > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > > > > > > This patch enables posting Verb requests and receiving responses to/from > > > > > > the backend PVRDMA emulation layer. > > > > > > > > > > > > > > ... > > > > > > > > > > +int > > > > > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > > > > > > + bool expect_resp, union pvrdma_cmd_resp *resp) > > > > > > +{ > > > > > > + int err; > > > > > > + > > > > > > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > > > > > > + > > > > > > + /* Serializiation */ > > > > > > + down(&dev->cmd_sema); > > > > > > + > > > > > > + spin_lock(&dev->cmd_lock); > > > > > > + memcpy(dev->cmd_slot, req, sizeof(*req)); > > > > > > > > > > Just to protect against memory corruption suggesting to do: > > > > > memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > > > > > > > > > > > > > Done. > > > > > > Hi Adit, > > > I didn't check how "dev->cmd_slot" was declared and if it is equal to > > > req in size. Additionally I didn't check if memcpy has any size > > > limitations, but wanted to warn you that implementation of this > > > suggestion as is will leave your system in much worse situation of > > > partially copied data. > > > > > > if you really want to check sizes, please do it with BUILD_ON compilation > > > macro. I have serious doubts if you need it. > > > > > > Thanks > > > > Hi Leon, > > > > Thanks for taking a look! > > I think this is okay to do was that we allocate a page for cmd_slot anyways > > and the size of req will always be smaller than PAGE_SIZE. If the request is > > malformed, our virtual hardware should throw an error for it. > > I dont think we would run into partially copied data. > > > > Having said that, I dont see a necessary advantage of using min here so am > > willing to drop it if you feel strongly about it. > > > > Can you clarify what you mean by memcpy size limitations? > > I didn't have time to look after reasons for Yuval's comment about > min(..), so I assumed that one of the possibilities will be limitation > of memcpy which I'm not aware. I understand that it is not such realistic that one of the structures in union pvrdma_cmd_req will be more then PAGE_SIZE but any other alternative to protect from such case? > > > > > Thanks, > > Adit -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 14, 2016 at 02:25:36PM +0300, Yuval Shaia wrote: > On Sun, Jul 31, 2016 at 09:15:47AM +0300, Leon Romanovsky wrote: > > On Fri, Jul 29, 2016 at 09:31:16PM +0000, Adit Ranadive wrote: > > > On Fri, 29 Jul 2016 12:37:29 +0000, Leon Romanovsky <leon@kernel.org> wrote: > > > > On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote: > > > > > On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia@oracle.com> wrote: > > > > > > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > > > > > > > This patch enables posting Verb requests and receiving responses to/from > > > > > > > the backend PVRDMA emulation layer. > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > +int > > > > > > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > > > > > > > + bool expect_resp, union pvrdma_cmd_resp *resp) > > > > > > > +{ > > > > > > > + int err; > > > > > > > + > > > > > > > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > > > > > > > + > > > > > > > + /* Serializiation */ > > > > > > > + down(&dev->cmd_sema); > > > > > > > + > > > > > > > + spin_lock(&dev->cmd_lock); > > > > > > > + memcpy(dev->cmd_slot, req, sizeof(*req)); > > > > > > > > > > > > Just to protect against memory corruption suggesting to do: > > > > > > memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > > > > > > > > > > > > > > > > Done. > > > > > > > > Hi Adit, > > > > I didn't check how "dev->cmd_slot" was declared and if it is equal to > > > > req in size. Additionally I didn't check if memcpy has any size > > > > limitations, but wanted to warn you that implementation of this > > > > suggestion as is will leave your system in much worse situation of > > > > partially copied data. > > > > > > > > if you really want to check sizes, please do it with BUILD_ON compilation > > > > macro. I have serious doubts if you need it. > > > > > > > > Thanks > > > > > > Hi Leon, > > > > > > Thanks for taking a look! > > > I think this is okay to do was that we allocate a page for cmd_slot anyways > > > and the size of req will always be smaller than PAGE_SIZE. If the request is > > > malformed, our virtual hardware should throw an error for it. > > > I dont think we would run into partially copied data. > > > > > > Having said that, I dont see a necessary advantage of using min here so am > > > willing to drop it if you feel strongly about it. > > > > > > Can you clarify what you mean by memcpy size limitations? > > > > I didn't have time to look after reasons for Yuval's comment about > > min(..), so I assumed that one of the possibilities will be limitation > > of memcpy which I'm not aware. > > I understand that it is not such realistic that one of the structures in > union pvrdma_cmd_req will be more then PAGE_SIZE but any other alternative > to protect from such case? BUILD_BUG_ON(...) is a perfect way to achieve it. > > > > > > > > > Thanks, > > > Adit > >
On Sun, Aug 14, 2016 at 17:24:46 +0000, Leon Romanovsky <leon@kernel.org> wrote: > On Sun, Aug 14, 2016 at 02:25:36PM +0300, Yuval Shaia wrote: > > On Sun, Jul 31, 2016 at 09:15:47AM +0300, Leon Romanovsky wrote: > > > On Fri, Jul 29, 2016 at 09:31:16PM +0000, Adit Ranadive wrote: > > > > On Fri, 29 Jul 2016 12:37:29 +0000, Leon Romanovsky <leon@kernel.org> wrote: > > > > > On Thu, Jul 28, 2016 at 10:51:39PM +0000, Adit Ranadive wrote: > > > > > > On Mon, 18 Jul 2016 15:46:53 +0300 Yuval Shaia <yuval.shaia@oracle.com> wrote: > > > > > > > On Tue, Jul 12, 2016 at 12:36:32PM -0700, Adit Ranadive wrote: > > > > > > > > This patch enables posting Verb requests and receiving responses to/from > > > > > > > > the backend PVRDMA emulation layer. > > > > > > > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > +int > > > > > > > > +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, > > > > > > > > + bool expect_resp, union pvrdma_cmd_resp *resp) > > > > > > > > +{ > > > > > > > > + int err; > > > > > > > > + > > > > > > > > + dev_dbg(&dev->pdev->dev, "post request to device\n"); > > > > > > > > + > > > > > > > > + /* Serializiation */ > > > > > > > > + down(&dev->cmd_sema); > > > > > > > > + > > > > > > > > + spin_lock(&dev->cmd_lock); > > > > > > > > + memcpy(dev->cmd_slot, req, sizeof(*req)); > > > > > > > > > > > > > > Just to protect against memory corruption suggesting to do: > > > > > > > memcpy(dev->cmd_slot, req, min(PAGE_SIZE, sizeof(*req))); > > > > > > > > > > > > > > > > > > > Done. > > > > > > > > > > Hi Adit, > > > > > I didn't check how "dev->cmd_slot" was declared and if it is equal to > > > > > req in size. Additionally I didn't check if memcpy has any size > > > > > limitations, but wanted to warn you that implementation of this > > > > > suggestion as is will leave your system in much worse situation of > > > > > partially copied data. > > > > > > > > > > if you really want to check sizes, please do it with BUILD_ON compilation > > > > > macro. I have serious doubts if you need it. > > > > > > > > > > Thanks > > > > > > > > Hi Leon, > > > > > > > > Thanks for taking a look! > > > > I think this is okay to do was that we allocate a page for cmd_slot anyways > > > > and the size of req will always be smaller than PAGE_SIZE. If the request is > > > > malformed, our virtual hardware should throw an error for it. > > > > I dont think we would run into partially copied data. > > > > > > > > Having said that, I dont see a necessary advantage of using min here so am > > > > willing to drop it if you feel strongly about it. > > > > > > > > Can you clarify what you mean by memcpy size limitations? > > > > > > I didn't have time to look after reasons for Yuval's comment about > > > min(..), so I assumed that one of the possibilities will be limitation > > > of memcpy which I'm not aware. > > > > I understand that it is not such realistic that one of the structures in > > union pvrdma_cmd_req will be more then PAGE_SIZE but any other alternative > > to protect from such case? > > BUILD_BUG_ON(...) is a perfect way to achieve it. Thanks for your comments, Leon and Yuval! I will add that in v4 of the patch series. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c new file mode 100644 index 0000000..c22debf --- /dev/null +++ b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c @@ -0,0 +1,104 @@ +/* + * Copyright (c) 2012-2016 VMware, Inc. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of EITHER the GNU General Public License + * version 2 as published by the Free Software Foundation or the BSD + * 2-Clause License. This program is distributed in the hope that it + * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED + * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. + * See the GNU General Public License version 2 for more details at + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html. + * + * You should have received a copy of the GNU General Public License + * along with this program available in the file COPYING in the main + * directory of this source tree. + * + * The BSD 2-Clause License + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above + * copyright notice, this list of conditions and the following + * disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials + * provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED + * OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include <linux/list.h> + +#include "pvrdma.h" + +#define PVRDMA_CMD_TIMEOUT 10000 /* ms */ + +static int pvrdma_cmd_recv(struct pvrdma_dev *dev, union pvrdma_cmd_resp *resp) +{ + dev_dbg(&dev->pdev->dev, "receive response from device\n"); + + spin_lock(&dev->cmd_lock); + memcpy(resp, dev->resp_slot, sizeof(*resp)); + spin_unlock(&dev->cmd_lock); + + return 0; +} + +int +pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req, + bool expect_resp, union pvrdma_cmd_resp *resp) +{ + int err; + + dev_dbg(&dev->pdev->dev, "post request to device\n"); + + /* Serializiation */ + down(&dev->cmd_sema); + + spin_lock(&dev->cmd_lock); + memcpy(dev->cmd_slot, req, sizeof(*req)); + spin_unlock(&dev->cmd_lock); + + init_completion(&dev->cmd_done); + pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0); + + /* Make sure the request is written before reading status. */ + mb(); + err = pvrdma_read_reg(dev, PVRDMA_REG_ERR); + if (err == 0) { + if (expect_resp) { + err = wait_for_completion_interruptible_timeout( + &dev->cmd_done, + msecs_to_jiffies(PVRDMA_CMD_TIMEOUT)); + if (err == 0) { + dev_warn(&dev->pdev->dev, + "completion timeout\n"); + err = -ETIMEDOUT; + } else { + err = pvrdma_cmd_recv(dev, resp); + } + } + } else { + dev_warn(&dev->pdev->dev, "failed to write request %d\n", err); + } + + up(&dev->cmd_sema); + + return err; +}