On 21-02-12 13:27:06, Jonathan Cameron wrote:
> On Thu, 11 Feb 2021 07:55:29 -0800
> Ben Widawsky <ben.widaw...@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.widaw...@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.came...@huawei.com> wrote:
> > > > > >
> > > > > > > On Tue, 9 Feb 2021 16:02:53 -0800
> > > > > > > Ben Widawsky <ben.widaw...@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.widaw...@intel.com>
> > > > > > > > Reviewed-by: Dan Williams <dan.j.willi...@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
> >
> > Some background because I forget what I've said previously... It's
> > unfortunate
> > that the spec maxes at 1M mailbox size but has enough bits in the length
> > field
> > to support 2M-1. I've made some requests to have this fixed, so maybe 3.0
> > won't
> > be awkward like this.
>
> Agreed spec should be tighter here, but I'd argue over 1M indicates buggy
> hardware.
>
> >
> > I think it makes sense to do as you suggested. One question though, do you
> > have
> > an opinion on we return to the caller as the output payload size, do we cap
> > it
> > at 1M also, or are we honest?
> >
> > - if (out_len && mbox_cmd->payload_out)
> > - memcpy_fromio(mbox_cmd->payload_out, payload, out_len);
> > + if (out_len && mbox_cmd->payload_out) {
> > + size_t n = min_t(size_t, cxlm->payload_size, out_len);
> > + memcpy_fromio(mbox_cmd->payload_out, payload, n);
> > + }
>
> Ah, I read emails in wrong order. What you have is what I expected and got
> confused about in your other email.
>
> >
> > So...
> > mbox_cmd->size_out = out_len;
> > mbox_cmd->size_out = n;
>
> Good question. My gut says the second one.
> Maybe it's worth a warning print to let us know something
> unexpected happened.
>
I also prefer 'n', It's unfortunate though if userspace hits this condition, it
would have to scrape kernel logs to find out. Perhaps though userspace wouldn't
ever really care.
> >
> >
> > >
> > >
> > > >
> > > > 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]
> > > >
> > >
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
