From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <linux-acpi@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-nvdimm@lists.01.org>,
<linux-pci@vger.kernel.org>, Bjorn Helgaas <helgaas@kernel.org>,
"Chris Browy" <cbrowy@avery-design.com>,
Christoph Hellwig <hch@infradead.org>,
"Dan Williams" <dan.j.williams@intel.com>,
David Hildenbrand <david@redhat.com>,
David Rientjes <rientjes@google.com>,
Ira Weiny <ira.weiny@intel.com>,
"Jon Masters" <jcm@jonmasters.org>,
Rafael Wysocki <rafael.j.wysocki@intel.com>,
Randy Dunlap <rdunlap@infradead.org>,
Vishal Verma <vishal.l.verma@intel.com>,
"John Groves (jgroves)" <jgroves@micron.com>,
"Kelley, Sean V" <sean.v.kelley@intel.com>
Subject: Re: [PATCH v2 2/8] cxl/mem: Find device capabilities
Date: Fri, 12 Feb 2021 13:23:28 +0000 [thread overview]
Message-ID: <20210212132328.0000482f@Huawei.com> (raw)
In-Reply-To: <20210211182741.yrojts2cdyoufsfl@intel.com>
On Thu, 11 Feb 2021 10:27:41 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:
> On 21-02-11 09:55:48, Jonathan Cameron wrote:
> > On Wed, 10 Feb 2021 10:16:05 -0800
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > > On 21-02-10 08:55:57, Ben Widawsky wrote:
> > > > On 21-02-10 15:07:59, Jonathan Cameron wrote:
> > > > > On Wed, 10 Feb 2021 13:32:52 +0000
> > > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > > >
> > > > > > On Tue, 9 Feb 2021 16:02:53 -0800
> > > > > > Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > > > >
> > > > > > > Provide enough functionality to utilize the mailbox of a memory device.
> > > > > > > The mailbox is used to interact with the firmware running on the memory
> > > > > > > device. The flow is proven with one implemented command, "identify".
> > > > > > > Because the class code has already told the driver this is a memory
> > > > > > > device and the identify command is mandatory.
> > > > > > >
> > > > > > > CXL devices contain an array of capabilities that describe the
> > > > > > > interactions software can have with the device or firmware running on
> > > > > > > the device. A CXL compliant device must implement the device status and
> > > > > > > the mailbox capability. Additionally, a CXL compliant memory device must
> > > > > > > implement the memory device capability. Each of the capabilities can
> > > > > > > [will] provide an offset within the MMIO region for interacting with the
> > > > > > > CXL device.
> > > > > > >
> > > > > > > The capabilities tell the driver how to find and map the register space
> > > > > > > for CXL Memory Devices. The registers are required to utilize the CXL
> > > > > > > spec defined mailbox interface. The spec outlines two mailboxes, primary
> > > > > > > and secondary. The secondary mailbox is earmarked for system firmware,
> > > > > > > and not handled in this driver.
> > > > > > >
> > > > > > > Primary mailboxes are capable of generating an interrupt when submitting
> > > > > > > a background command. That implementation is saved for a later time.
> > > > > > >
> > > > > > > Link: https://www.computeexpresslink.org/download-the-specification
> > > > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > >
> > > > > > Hi Ben,
> > > > > >
> > > > > >
> > > > > > > +/**
> > > > > > > + * cxl_mem_mbox_send_cmd() - Send a mailbox command to a memory device.
> > > > > > > + * @cxlm: The CXL memory device to communicate with.
> > > > > > > + * @mbox_cmd: Command to send to the memory device.
> > > > > > > + *
> > > > > > > + * Context: Any context. Expects mbox_lock to be held.
> > > > > > > + * Return: -ETIMEDOUT if timeout occurred waiting for completion. 0 on success.
> > > > > > > + * Caller should check the return code in @mbox_cmd to make sure it
> > > > > > > + * succeeded.
> > > > > >
> > > > > > cxl_xfer_log() doesn't check mbox_cmd->return_code and for my test it currently
> > > > > > enters an infinite loop as a result.
> > > >
> > > > I meant to fix that.
> > > >
> > > > > >
> > > > > > I haven't checked other paths, but to my mind it is not a good idea to require
> > > > > > two levels of error checking - the example here proves how easy it is to forget
> > > > > > one.
> > > >
> > > > Demonstrably, you're correct. I think it would be good to have a kernel only
> > > > mbox command that does the error checking though. Let me type something up and
> > > > see how it looks.
> > >
> > > Hi Jonathan. What do you think of this? The bit I'm on the fence about is if I
> > > should validate output size too. I like the simplicity as it is, but it requires
> > > every caller to possibly check output size, which is kind of the same problem
> > > you're originally pointing out.
> >
> > The simplicity is good and this is pretty much what I expected you would end up with
> > (always reassuring)
> >
> > For the output, perhaps just add another parameter to the wrapper for minimum
> > output length expected?
> >
> > Now you mention the length question. It does rather feel like there should also
> > be some protection on memcpy_fromio() copying too much data if the hardware
> > happens to return an unexpectedly long length. Should never happen, but
> > the hardening is worth adding anyway given it's easy to do.
> >
> > Jonathan
> >
>
> I like it.
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 2e199b05f686..58071a203212 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -293,7 +293,7 @@ static void cxl_mem_mbox_put(struct cxl_mem *cxlm)
> * See __cxl_mem_mbox_send_cmd()
> */
> static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode, u8 *in,
> - size_t in_size, u8 *out)
> + size_t in_size, u8 *out, size_t out_min_size)
This is kind of the opposite of what I was expecting. What I'm worried about is
not so much that we receive at least enough data, but rather that we receive too much.
Buggy hardware or potentially a spec change being most likely causes.
So something like
int __cxl_mem_mbox_send_cmd(struct cxl_mem..., struct mbox_cmd, u8 *out, size_t out_sz)
//Or put the max size in the .size_out element of the command and make that inout rather
//than just out direction.
{
...
/* #8 */
if (out_len && mbox_cmd->payload_out) {
if (outlen > out_sz)
//or just copy what we can fit in payload_out and return that size.
return -E2BIG;
memcpy_fromio(mbox_cmd->payload_out, payload, out_len);
}
}
Fine to also check the returned length is at least a minimum size.
> {
> struct mbox_cmd mbox_cmd = {
> .opcode = opcode,
> @@ -303,6 +303,9 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode, u8 *in,
> };
> int rc;
>
> + if (out_min_size > cxlm->payload_size)
> + return -E2BIG;
> +
> rc = cxl_mem_mbox_get(cxlm);
> if (rc)
> return rc;
> @@ -316,6 +319,9 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode, u8 *in,
> if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
> return -ENXIO;
>
> + if (mbox_cmd.size_out < out_min_size)
> + return -ENODATA;
> +
> return mbox_cmd.size_out;
> }
>
> @@ -505,15 +511,10 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
> int rc;
>
> rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_IDENTIFY, NULL, 0,
> - (u8 *)&id);
> + (u8 *)&id, sizeof(id));
> if (rc < 0)
> return rc;
>
> - if (rc < sizeof(id)) {
> - dev_err(&cxlm->pdev->dev, "Short identify data\n");
> - return -ENXIO;
> - }
> -
> /*
> * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
> * For now, only the capacity is exported in sysfs
>
>
> >
> > >
> > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > index 55c5f5a6023f..ad7b2077ab28 100644
> > > --- a/drivers/cxl/mem.c
> > > +++ b/drivers/cxl/mem.c
> > > @@ -284,7 +284,7 @@ static void cxl_mem_mbox_timeout(struct cxl_mem *cxlm,
> > > }
> > >
> > > /**
> > > - * cxl_mem_mbox_send_cmd() - Send a mailbox command to a memory device.
> > > + * __cxl_mem_mbox_send_cmd() - Execute a mailbox command
> > > * @cxlm: The CXL memory device to communicate with.
> > > * @mbox_cmd: Command to send to the memory device.
> > > *
> > > @@ -296,7 +296,8 @@ static void cxl_mem_mbox_timeout(struct cxl_mem *cxlm,
> > > * This is a generic form of the CXL mailbox send command, thus the only I/O
> > > * operations used are cxl_read_mbox_reg(). Memory devices, and perhaps other
> > > * types of CXL devices may have further information available upon error
> > > - * conditions.
> > > + * conditions. Driver facilities wishing to send mailbox commands should use the
> > > + * wrapper command.
> > > *
> > > * The CXL spec allows for up to two mailboxes. The intention is for the primary
> > > * mailbox to be OS controlled and the secondary mailbox to be used by system
> > > @@ -304,8 +305,8 @@ static void cxl_mem_mbox_timeout(struct cxl_mem *cxlm,
> > > * not need to coordinate with each other. The driver only uses the primary
> > > * mailbox.
> > > */
> > > -static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> > > - struct mbox_cmd *mbox_cmd)
> > > +static int __cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm,
> > > + struct mbox_cmd *mbox_cmd)
> > > {
> > > void __iomem *payload = cxlm->mbox_regs + CXLDEV_MBOX_PAYLOAD_OFFSET;
> > > u64 cmd_reg, status_reg;
> > > @@ -469,6 +470,54 @@ static void cxl_mem_mbox_put(struct cxl_mem *cxlm)
> > > mutex_unlock(&cxlm->mbox_mutex);
> > > }
> > >
> > > +/**
> > > + * cxl_mem_mbox_send_cmd() - Send a mailbox command to a memory device.
> > > + * @cxlm: The CXL memory device to communicate with.
> > > + * @opcode: Opcode for the mailbox command.
> > > + * @in: The input payload for the mailbox command.
> > > + * @in_size: The length of the input payload
> > > + * @out: Caller allocated buffer for the output.
> > > + *
> > > + * Context: Any context. Will acquire and release mbox_mutex.
> > > + * Return:
> > > + * * %>=0 - Number of bytes returned in @out.
> > > + * * %-EBUSY - Couldn't acquire exclusive mailbox access.
> > > + * * %-EFAULT - Hardware error occurred.
> > > + * * %-ENXIO - Command completed, but device reported an error.
> > > + *
> > > + * Mailbox commands may execute successfully yet the device itself reported an
> > > + * error. While this distinction can be useful for commands from userspace, the
> > > + * kernel will often only care when both are successful.
> > > + *
> > > + * See __cxl_mem_mbox_send_cmd()
> > > + */
> > > +static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode, u8 *in,
> > > + size_t in_size, u8 *out)
> > > +{
> > > + struct mbox_cmd mbox_cmd = {
> > > + .opcode = opcode,
> > > + .payload_in = in,
> > > + .size_in = in_size,
> > > + .payload_out = out,
> > > + };
> > > + int rc;
> > > +
> > > + rc = cxl_mem_mbox_get(cxlm);
> > > + if (rc)
> > > + return rc;
> > > +
> > > + rc = __cxl_mem_mbox_send_cmd(cxlm, &mbox_cmd);
> > > + cxl_mem_mbox_put(cxlm);
> > > + if (rc)
> > > + return rc;
> > > +
> > > + /* TODO: Map return code to proper kernel style errno */
> > > + if (mbox_cmd.return_code != CXL_MBOX_SUCCESS)
> > > + return -ENXIO;
> > > +
> > > + return mbox_cmd.size_out;
> > > +}
> > > +
> > > /**
> > > * handle_mailbox_cmd_from_user() - Dispatch a mailbox command.
> > > * @cxlmd: The CXL memory device to communicate with.
> > > @@ -1380,33 +1429,18 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
> > > u8 poison_caps;
> > > u8 qos_telemetry_caps;
> > > } __packed id;
> > > - struct mbox_cmd mbox_cmd = {
> > > - .opcode = CXL_MBOX_OP_IDENTIFY,
> > > - .payload_out = &id,
> > > - .size_in = 0,
> > > - };
> > > int rc;
> > >
> > > - /* Retrieve initial device memory map */
> > > - rc = cxl_mem_mbox_get(cxlm);
> > > - if (rc)
> > > - return rc;
> > > -
> > > - rc = cxl_mem_mbox_send_cmd(cxlm, &mbox_cmd);
> > > - cxl_mem_mbox_put(cxlm);
> > > - if (rc)
> > > + rc = cxl_mem_mbox_send_cmd(cxlm, CXL_MBOX_OP_IDENTIFY, NULL, 0,
> > > + (u8 *)&id);
> > > + if (rc < 0)
> > > return rc;
> > >
> > > - /* TODO: Handle retry or reset responses from firmware. */
> > > - if (mbox_cmd.return_code != CXL_MBOX_SUCCESS) {
> > > - dev_err(&cxlm->pdev->dev, "Mailbox command failed (%d)\n",
> > > - mbox_cmd.return_code);
> > > + if (rc < sizeof(id)) {
> > > + dev_err(&cxlm->pdev->dev, "Short identify data\n",
> > > return -ENXIO;
> > > }
> > >
> > > - if (mbox_cmd.size_out != sizeof(id))
> > > - return -ENXIO;
> > > -
> > > /*
> > > * TODO: enumerate DPA map, as 'ram' and 'pmem' do not alias.
> > > * For now, only the capacity is exported in sysfs
> > >
> > >
> > > [snip]
> > >
> >
next prev parent reply other threads:[~2021-02-12 13:25 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-10 0:02 [PATCH v2 0/8] CXL 2.0 Support Ben Widawsky
2021-02-10 0:02 ` [PATCH v2 1/8] cxl/mem: Introduce a driver for CXL-2.0-Type-3 endpoints Ben Widawsky
2021-02-10 16:17 ` Jonathan Cameron
2021-02-10 17:12 ` Ben Widawsky
2021-02-10 17:23 ` Jonathan Cameron
2021-02-10 0:02 ` [PATCH v2 2/8] cxl/mem: Find device capabilities Ben Widawsky
2021-02-10 13:32 ` Jonathan Cameron
2021-02-10 15:07 ` Jonathan Cameron
2021-02-10 16:55 ` Ben Widawsky
2021-02-10 17:30 ` Jonathan Cameron
2021-02-10 18:16 ` Ben Widawsky
2021-02-11 9:55 ` Jonathan Cameron
2021-02-11 15:55 ` Ben Widawsky
2021-02-12 13:27 ` Jonathan Cameron
2021-02-12 15:54 ` Ben Widawsky
2021-02-11 18:27 ` Ben Widawsky
2021-02-12 13:23 ` Jonathan Cameron [this message]
2021-02-10 19:32 ` Ben Widawsky
2021-02-10 17:41 ` Jonathan Cameron
2021-02-10 18:53 ` Ben Widawsky
2021-02-10 19:54 ` Dan Williams
2021-02-11 10:01 ` Jonathan Cameron
2021-02-11 16:04 ` Ben Widawsky
2021-02-10 0:02 ` [PATCH v2 3/8] cxl/mem: Register CXL memX devices Ben Widawsky
2021-02-10 18:17 ` Jonathan Cameron
2021-02-11 10:17 ` Jonathan Cameron
2021-02-11 20:40 ` Dan Williams
2021-02-12 13:33 ` Jonathan Cameron
2021-02-10 0:02 ` [PATCH v2 4/8] cxl/mem: Add basic IOCTL interface Ben Widawsky
2021-02-10 18:45 ` Jonathan Cameron
2021-02-10 20:22 ` Ben Widawsky
2021-02-11 4:40 ` Dan Williams
2021-02-11 10:06 ` Jonathan Cameron
2021-02-11 16:54 ` Ben Widawsky
2021-02-14 16:30 ` Al Viro
2021-02-14 23:14 ` Ben Widawsky
2021-02-14 23:50 ` Al Viro
2021-02-14 23:57 ` Al Viro
2021-02-10 0:02 ` [PATCH v2 5/8] cxl/mem: Add a "RAW" send command Ben Widawsky
2021-02-10 15:26 ` Ariel.Sibley
2021-02-10 16:49 ` Ben Widawsky
2021-02-10 18:03 ` Ariel.Sibley
2021-02-10 18:11 ` Ben Widawsky
2021-02-10 18:46 ` Ariel.Sibley
2021-02-10 19:12 ` Ben Widawsky
2021-02-11 16:43 ` Dan Williams
2021-02-11 11:19 ` Jonathan Cameron
2021-02-11 16:01 ` Ben Widawsky
2021-02-12 13:40 ` Jonathan Cameron
2021-02-10 0:02 ` [PATCH v2 6/8] cxl/mem: Enable commands via CEL Ben Widawsky
2021-02-11 12:02 ` Jonathan Cameron
2021-02-11 17:45 ` Ben Widawsky
2021-02-11 20:34 ` Dan Williams
2021-02-16 13:43 ` Bartosz Golaszewski
2021-02-10 0:02 ` [PATCH v2 7/8] cxl/mem: Add set of informational commands Ben Widawsky
2021-02-11 12:07 ` Jonathan Cameron
2021-02-10 0:02 ` [PATCH v2 8/8] MAINTAINERS: Add maintainers of the CXL driver Ben Widawsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210212132328.0000482f@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=ben.widawsky@intel.com \
--cc=cbrowy@avery-design.com \
--cc=dan.j.williams@intel.com \
--cc=david@redhat.com \
--cc=hch@infradead.org \
--cc=helgaas@kernel.org \
--cc=ira.weiny@intel.com \
--cc=jcm@jonmasters.org \
--cc=jgroves@micron.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvdimm@lists.01.org \
--cc=linux-pci@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
--cc=rdunlap@infradead.org \
--cc=rientjes@google.com \
--cc=sean.v.kelley@intel.com \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).