| Message ID | 4dba01ff87d630abdd5a09d52e954d3c212d2018.1674468099.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Collection of DOE material | expand |
Hi Lukas, On Mon, Jan 23, 2023 at 11:20:00AM +0100, Lukas Wunner wrote: > An upcoming user of DOE is CMA (Component Measurement and Authentication, > PCIe r6.0 sec 6.31). > > It builds on SPDM (Security Protocol and Data Model): > https://www.dmtf.org/dsp/DSP0274 > > SPDM message sizes are not always a multiple of dwords. To transport > them over DOE without using bounce buffers, allow sending requests and > receiving responses whose final dword is only partially populated. Can you add a note about this non-dwordness? The sec 6.30.1 Data Object Header 2 "Length" field is in DW and the code in pci_doe_send_req() and pci_doe_recv_resp() still reads/writes dwords. I don't see the 6.31 text that requires non-dword sizes. From a spec point of view, AFAICS, DOE is still specified in dwords, but I guess you're leaving the actual PCI config-level DOE interface in dwords and just making it more convenient for callers by having pci_doe_*() hide the details of any partial dwords at the end by transferring the entire dword over PCI but only copying the requested bytes to/from the caller's buffer? > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > --- > drivers/pci/doe.c | 66 ++++++++++++++++++++++++++++------------------- > 1 file changed, 40 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index 0263bcfdddd8..2113ec95379f 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -76,13 +76,6 @@ struct pci_doe_protocol { > * @private: Private data for the consumer > * @work: Used internally by the mailbox > * @doe_mb: Used internally by the mailbox > - * > - * The payload sizes and rv are specified in bytes with the following > - * restrictions concerning the protocol. > - * > - * 1) The request_pl_sz must be a multiple of double words (4 bytes) > - * 2) The response_pl_sz must be >= a single double word (4 bytes) > - * 3) rv is returned as bytes but it will be a multiple of double words > */ > struct pci_doe_task { > struct pci_doe_protocol prot; > @@ -153,7 +146,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > { > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > - size_t length; > + size_t length, remainder; > u32 val; > int i; > > @@ -171,7 +164,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > return -EIO; > > /* Length is 2 DW of header + length of payload in DW */ > - length = 2 + task->request_pl_sz / sizeof(u32); > + length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32)); > if (length > PCI_DOE_MAX_LENGTH) > return -EIO; > if (length == PCI_DOE_MAX_LENGTH) > @@ -184,10 +177,20 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, > length)); > + > + /* Write payload */ > for (i = 0; i < task->request_pl_sz / sizeof(u32); i++) > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > task->request_pl[i]); > > + /* Write last payload dword */ > + remainder = task->request_pl_sz % sizeof(u32); > + if (remainder) { > + val = 0; > + memcpy(&val, &task->request_pl[i], remainder); > + pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val); > + } > + > pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO); > > return 0; > @@ -207,11 +210,11 @@ static bool pci_doe_data_obj_ready(struct pci_doe_mb *doe_mb) > > static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *task) > { > + size_t length, payload_length, remainder, received; > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > - size_t length, payload_length; > + int i = 0; > u32 val; > - int i; > > /* Read the first dword to get the protocol */ > pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); > @@ -238,15 +241,34 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > > /* First 2 dwords have already been read */ > length -= 2; > - payload_length = min(length, task->response_pl_sz / sizeof(u32)); > - /* Read the rest of the response payload */ > - for (i = 0; i < payload_length; i++) { > - pci_read_config_dword(pdev, offset + PCI_DOE_READ, > - &task->response_pl[i]); > + received = task->response_pl_sz; > + payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32)); > + remainder = task->response_pl_sz % sizeof(u32); > + if (!remainder) > + remainder = sizeof(u32); > + > + if (length < payload_length) { > + received = length * sizeof(u32); > + payload_length = length; > + remainder = sizeof(u32); > + } > + > + if (payload_length) { > + /* Read all payload dwords except the last */ > + for (; i < payload_length - 1; i++) { > + pci_read_config_dword(pdev, offset + PCI_DOE_READ, > + &task->response_pl[i]); > + pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > + } > + > + /* Read last payload dword */ > + pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); > + memcpy(&task->response_pl[i], &val, remainder); > /* Prior to the last ack, ensure Data Object Ready */ > - if (i == (payload_length - 1) && !pci_doe_data_obj_ready(doe_mb)) > + if (!pci_doe_data_obj_ready(doe_mb)) > return -EIO; > pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > + i++; > } > > /* Flush excess length */ > @@ -260,7 +282,7 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) > return -EIO; > > - return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32); > + return received; > } > > static void signal_task_complete(struct pci_doe_task *task, int rv) > @@ -560,14 +582,6 @@ static int pci_doe_submit_task(struct pci_doe_mb *doe_mb, > if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type)) > return -EINVAL; > > - /* > - * DOE requests must be a whole number of DW and the response needs to > - * be big enough for at least 1 DW > - */ > - if (task->request_pl_sz % sizeof(u32) || > - task->response_pl_sz < sizeof(u32)) > - return -EINVAL; > - > if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) > return -EIO; > > -- > 2.39.1 >
Lukas Wunner wrote: > An upcoming user of DOE is CMA (Component Measurement and Authentication, > PCIe r6.0 sec 6.31). > > It builds on SPDM (Security Protocol and Data Model): > https://www.dmtf.org/dsp/DSP0274 > > SPDM message sizes are not always a multiple of dwords. To transport > them over DOE without using bounce buffers, allow sending requests and > receiving responses whose final dword is only partially populated. > > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > --- > drivers/pci/doe.c | 66 ++++++++++++++++++++++++++++------------------- > 1 file changed, 40 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index 0263bcfdddd8..2113ec95379f 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -76,13 +76,6 @@ struct pci_doe_protocol { > * @private: Private data for the consumer > * @work: Used internally by the mailbox > * @doe_mb: Used internally by the mailbox > - * > - * The payload sizes and rv are specified in bytes with the following > - * restrictions concerning the protocol. > - * > - * 1) The request_pl_sz must be a multiple of double words (4 bytes) > - * 2) The response_pl_sz must be >= a single double word (4 bytes) > - * 3) rv is returned as bytes but it will be a multiple of double words > */ > struct pci_doe_task { > struct pci_doe_protocol prot; > @@ -153,7 +146,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > { > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > - size_t length; > + size_t length, remainder; > u32 val; > int i; > > @@ -171,7 +164,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > return -EIO; > > /* Length is 2 DW of header + length of payload in DW */ > - length = 2 + task->request_pl_sz / sizeof(u32); > + length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32)); > if (length > PCI_DOE_MAX_LENGTH) > return -EIO; > if (length == PCI_DOE_MAX_LENGTH) > @@ -184,10 +177,20 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, > length)); > + > + /* Write payload */ > for (i = 0; i < task->request_pl_sz / sizeof(u32); i++) > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > task->request_pl[i]); > > + /* Write last payload dword */ > + remainder = task->request_pl_sz % sizeof(u32); > + if (remainder) { > + val = 0; > + memcpy(&val, &task->request_pl[i], remainder); Are there any issues with endianess here? > + pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val); > + } > + > pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO); > > return 0; > @@ -207,11 +210,11 @@ static bool pci_doe_data_obj_ready(struct pci_doe_mb *doe_mb) > > static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *task) > { > + size_t length, payload_length, remainder, received; > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > - size_t length, payload_length; > + int i = 0; > u32 val; > - int i; > > /* Read the first dword to get the protocol */ > pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); > @@ -238,15 +241,34 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > > /* First 2 dwords have already been read */ > length -= 2; > - payload_length = min(length, task->response_pl_sz / sizeof(u32)); > - /* Read the rest of the response payload */ > - for (i = 0; i < payload_length; i++) { > - pci_read_config_dword(pdev, offset + PCI_DOE_READ, > - &task->response_pl[i]); > + received = task->response_pl_sz; > + payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32)); > + remainder = task->response_pl_sz % sizeof(u32); > + if (!remainder) > + remainder = sizeof(u32); > + > + if (length < payload_length) { > + received = length * sizeof(u32); > + payload_length = length; > + remainder = sizeof(u32); It was a bit confusing why remainder was set to a dword here. But I got that it is because length and payload_length are both in dwords. > + } > + > + if (payload_length) { > + /* Read all payload dwords except the last */ > + for (; i < payload_length - 1; i++) { > + pci_read_config_dword(pdev, offset + PCI_DOE_READ, > + &task->response_pl[i]); > + pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > + } > + > + /* Read last payload dword */ > + pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); > + memcpy(&task->response_pl[i], &val, remainder); Same question on endianess. Ira > /* Prior to the last ack, ensure Data Object Ready */ > - if (i == (payload_length - 1) && !pci_doe_data_obj_ready(doe_mb)) > + if (!pci_doe_data_obj_ready(doe_mb)) > return -EIO; > pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > + i++; > } > > /* Flush excess length */ > @@ -260,7 +282,7 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) > return -EIO; > > - return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32); > + return received; > } > > static void signal_task_complete(struct pci_doe_task *task, int rv) > @@ -560,14 +582,6 @@ static int pci_doe_submit_task(struct pci_doe_mb *doe_mb, > if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type)) > return -EINVAL; > > - /* > - * DOE requests must be a whole number of DW and the response needs to > - * be big enough for at least 1 DW > - */ > - if (task->request_pl_sz % sizeof(u32) || > - task->response_pl_sz < sizeof(u32)) > - return -EINVAL; > - > if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) > return -EIO; > > -- > 2.39.1 >
On Mon, 23 Jan 2023 11:20:00 +0100 Lukas Wunner <lukas@wunner.de> wrote: > An upcoming user of DOE is CMA (Component Measurement and Authentication, > PCIe r6.0 sec 6.31). > > It builds on SPDM (Security Protocol and Data Model): > https://www.dmtf.org/dsp/DSP0274 > > SPDM message sizes are not always a multiple of dwords. To transport > them over DOE without using bounce buffers, allow sending requests and > receiving responses whose final dword is only partially populated. > > Tested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Ah. This... I can't immediately find the original discussion thread, but I'm fairly sure we had a version of the DOE code that did this (maybe we just discussed doing it and never had code...) IIRC, at the time feedback was strongly in favour of making the handling of non dword payloads a problem for the caller (e.g. PCI/CMA) not the DOE core, mainly so that we could keep the layering simple. DOE part of PCI spec says DWORD multiples only, CMA has non dword entries. Personally I'm fully in favour of making our lives easier and handling this at the DOE layer! The CMA padding code is nasty as we have to deal with caching just the right bits of the payload for the running hashes. With it at this layer I'd imagine that code gets much simpler Assuming resolution to Ira's question on endianness is resolved. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/pci/doe.c | 66 ++++++++++++++++++++++++++++------------------- > 1 file changed, 40 insertions(+), 26 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index 0263bcfdddd8..2113ec95379f 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -76,13 +76,6 @@ struct pci_doe_protocol { > * @private: Private data for the consumer > * @work: Used internally by the mailbox > * @doe_mb: Used internally by the mailbox > - * > - * The payload sizes and rv are specified in bytes with the following > - * restrictions concerning the protocol. > - * > - * 1) The request_pl_sz must be a multiple of double words (4 bytes) > - * 2) The response_pl_sz must be >= a single double word (4 bytes) > - * 3) rv is returned as bytes but it will be a multiple of double words > */ > struct pci_doe_task { > struct pci_doe_protocol prot; > @@ -153,7 +146,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > { > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > - size_t length; > + size_t length, remainder; > u32 val; > int i; > > @@ -171,7 +164,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > return -EIO; > > /* Length is 2 DW of header + length of payload in DW */ > - length = 2 + task->request_pl_sz / sizeof(u32); > + length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32)); > if (length > PCI_DOE_MAX_LENGTH) > return -EIO; > if (length == PCI_DOE_MAX_LENGTH) > @@ -184,10 +177,20 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, > length)); > + > + /* Write payload */ > for (i = 0; i < task->request_pl_sz / sizeof(u32); i++) > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > task->request_pl[i]); > > + /* Write last payload dword */ > + remainder = task->request_pl_sz % sizeof(u32); > + if (remainder) { > + val = 0; > + memcpy(&val, &task->request_pl[i], remainder); > + pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val); > + } > + > pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO); > > return 0; > @@ -207,11 +210,11 @@ static bool pci_doe_data_obj_ready(struct pci_doe_mb *doe_mb) > > static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *task) > { > + size_t length, payload_length, remainder, received; > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > - size_t length, payload_length; > + int i = 0; > u32 val; > - int i; > > /* Read the first dword to get the protocol */ > pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); > @@ -238,15 +241,34 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > > /* First 2 dwords have already been read */ > length -= 2; > - payload_length = min(length, task->response_pl_sz / sizeof(u32)); > - /* Read the rest of the response payload */ > - for (i = 0; i < payload_length; i++) { > - pci_read_config_dword(pdev, offset + PCI_DOE_READ, > - &task->response_pl[i]); > + received = task->response_pl_sz; > + payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32)); > + remainder = task->response_pl_sz % sizeof(u32); > + if (!remainder) > + remainder = sizeof(u32); > + > + if (length < payload_length) { > + received = length * sizeof(u32); > + payload_length = length; > + remainder = sizeof(u32); > + } > + > + if (payload_length) { > + /* Read all payload dwords except the last */ > + for (; i < payload_length - 1; i++) { > + pci_read_config_dword(pdev, offset + PCI_DOE_READ, > + &task->response_pl[i]); > + pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > + } > + > + /* Read last payload dword */ > + pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); > + memcpy(&task->response_pl[i], &val, remainder); > /* Prior to the last ack, ensure Data Object Ready */ > - if (i == (payload_length - 1) && !pci_doe_data_obj_ready(doe_mb)) > + if (!pci_doe_data_obj_ready(doe_mb)) > return -EIO; > pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > + i++; > } > > /* Flush excess length */ > @@ -260,7 +282,7 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) > return -EIO; > > - return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32); > + return received; > } > > static void signal_task_complete(struct pci_doe_task *task, int rv) > @@ -560,14 +582,6 @@ static int pci_doe_submit_task(struct pci_doe_mb *doe_mb, > if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type)) > return -EINVAL; > > - /* > - * DOE requests must be a whole number of DW and the response needs to > - * be big enough for at least 1 DW > - */ > - if (task->request_pl_sz % sizeof(u32) || > - task->response_pl_sz < sizeof(u32)) > - return -EINVAL; > - > if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) > return -EIO; >
On Tue, Jan 24, 2023 at 12:43:15PM +0000, Jonathan Cameron wrote: > On Mon, 23 Jan 2023 11:20:00 +0100 > Lukas Wunner <lukas@wunner.de> wrote: > > > An upcoming user of DOE is CMA (Component Measurement and Authentication, > > PCIe r6.0 sec 6.31). > > > > It builds on SPDM (Security Protocol and Data Model): > > https://www.dmtf.org/dsp/DSP0274 > > > > SPDM message sizes are not always a multiple of dwords. To transport > > them over DOE without using bounce buffers, allow sending requests and > > receiving responses whose final dword is only partially populated. > > > > Tested-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Ah. This... > > I can't immediately find the original discussion thread, but I'm fairly > sure we had a version of the DOE code that did this (maybe we just > discussed doing it and never had code...) > > IIRC, at the time feedback was strongly in favour of making > the handling of non dword payloads a problem for the caller (e.g. PCI/CMA) > not the DOE core, mainly so that we could keep the layering simple. > DOE part of PCI spec says DWORD multiples only, CMA has non dword > entries. I can't remember, but I might have been the voice in favor of making it the caller's problem. Your argument about dealing with it here makes a lot of sense, and I'm OK with it, but I *would* like to add some text to the commit log and comments in the code about what is happening here. Otherwise there's an unexplained disconnect between the DWORD spec language and the byte-oriented code. > Personally I'm fully in favour of making our lives easier and handling > this at the DOE layer! The CMA padding code is nasty as we have to deal > with caching just the right bits of the payload for the running hashes. > With it at this layer I'd imagine that code gets much simpler > > Assuming resolution to Ira's question on endianness is resolved. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Tue, 24 Jan 2023 17:51:55 -0600 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Jan 24, 2023 at 12:43:15PM +0000, Jonathan Cameron wrote: > > On Mon, 23 Jan 2023 11:20:00 +0100 > > Lukas Wunner <lukas@wunner.de> wrote: > > > > > An upcoming user of DOE is CMA (Component Measurement and Authentication, > > > PCIe r6.0 sec 6.31). > > > > > > It builds on SPDM (Security Protocol and Data Model): > > > https://www.dmtf.org/dsp/DSP0274 > > > > > > SPDM message sizes are not always a multiple of dwords. To transport > > > them over DOE without using bounce buffers, allow sending requests and > > > receiving responses whose final dword is only partially populated. > > > > > > Tested-by: Ira Weiny <ira.weiny@intel.com> > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > > Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > > Ah. This... > > > > I can't immediately find the original discussion thread, but I'm fairly > > sure we had a version of the DOE code that did this (maybe we just > > discussed doing it and never had code...) > > > > IIRC, at the time feedback was strongly in favour of making > > the handling of non dword payloads a problem for the caller (e.g. PCI/CMA) > > not the DOE core, mainly so that we could keep the layering simple. > > DOE part of PCI spec says DWORD multiples only, CMA has non dword > > entries. > > I can't remember, but I might have been the voice in favor of making > it the caller's problem. Your argument about dealing with it here > makes a lot of sense, and I'm OK with it, but I *would* like to add > some text to the commit log and comments in the code about what is > happening here. Otherwise there's an unexplained disconnect between > the DWORD spec language and the byte-oriented code. Absolutely agree. Calling out why we have a mismatch with the specification will avoid a bunch of head scratching in the future! > > > Personally I'm fully in favour of making our lives easier and handling > > this at the DOE layer! The CMA padding code is nasty as we have to deal > > with caching just the right bits of the payload for the running hashes. > > With it at this layer I'd imagine that code gets much simpler > > > > Assuming resolution to Ira's question on endianness is resolved. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Mon, Jan 23, 2023 at 05:43:53PM -0800, Ira Weiny wrote: > Lukas Wunner wrote: > > SPDM message sizes are not always a multiple of dwords. To transport > > them over DOE without using bounce buffers, allow sending requests and > > receiving responses whose final dword is only partially populated. [...] > > + /* Write last payload dword */ > > + remainder = task->request_pl_sz % sizeof(u32); > > + if (remainder) { > > + val = 0; > > + memcpy(&val, &task->request_pl[i], remainder); > > Are there any issues with endianess here? Indeed there were. I've fixed that in v3. > > /* First 2 dwords have already been read */ > > length -= 2; > > - payload_length = min(length, task->response_pl_sz / sizeof(u32)); > > - /* Read the rest of the response payload */ > > - for (i = 0; i < payload_length; i++) { > > - pci_read_config_dword(pdev, offset + PCI_DOE_READ, > > - &task->response_pl[i]); > > + received = task->response_pl_sz; > > + payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32)); > > + remainder = task->response_pl_sz % sizeof(u32); > > + if (!remainder) > > + remainder = sizeof(u32); > > + > > + if (length < payload_length) { > > + received = length * sizeof(u32); > > + payload_length = length; > > + remainder = sizeof(u32); > > It was a bit confusing why remainder was set to a dword here. But I got > that it is because length and payload_length are both in dwords. Here in pci_doe_recv_resp(), "remainder" signifies the number of data bytes in the last payload dword. If the response received via DOE is shorter than the buffer it is written into, then the last payload dword contains 4 data bytes. (DOE transmits full dwords.) I've added a code comment in v3 to hopefully avoid any confusion: /* remainder signifies number of data bytes in last payload dword */ Thanks, Lukas
On Tue, Jan 24, 2023 at 05:51:55PM -0600, Bjorn Helgaas wrote: > On Tue, Jan 24, 2023 at 12:43:15PM +0000, Jonathan Cameron wrote: > > On Mon, 23 Jan 2023 11:20:00 +0100 Lukas Wunner <lukas@wunner.de> wrote: > > > An upcoming user of DOE is CMA (Component Measurement and Authentication, > > > PCIe r6.0 sec 6.31). > > > > > > It builds on SPDM (Security Protocol and Data Model): > > > https://www.dmtf.org/dsp/DSP0274 > > > > > > SPDM message sizes are not always a multiple of dwords. To transport > > > them over DOE without using bounce buffers, allow sending requests and > > > receiving responses whose final dword is only partially populated. [...] > > IIRC, at the time feedback was strongly in favour of making > > the handling of non dword payloads a problem for the caller (e.g. PCI/CMA) > > not the DOE core, mainly so that we could keep the layering simple. > > DOE part of PCI spec says DWORD multiples only, CMA has non dword > > entries. > > I can't remember, but I might have been the voice in favor of making > it the caller's problem. Your argument about dealing with it here > makes a lot of sense, and I'm OK with it, but I *would* like to add > some text to the commit log and comments in the code about what is > happening here. Otherwise there's an unexplained disconnect between > the DWORD spec language and the byte-oriented code. In v3 I amended both the commit message and the kerneldoc for pci_doe() to make it clear that support for arbitrary-sized request and response buffers is not stipulated by the spec, but merely for the convenience of the caller. Thanks, Lukas
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index 0263bcfdddd8..2113ec95379f 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -76,13 +76,6 @@ struct pci_doe_protocol { * @private: Private data for the consumer * @work: Used internally by the mailbox * @doe_mb: Used internally by the mailbox - * - * The payload sizes and rv are specified in bytes with the following - * restrictions concerning the protocol. - * - * 1) The request_pl_sz must be a multiple of double words (4 bytes) - * 2) The response_pl_sz must be >= a single double word (4 bytes) - * 3) rv is returned as bytes but it will be a multiple of double words */ struct pci_doe_task { struct pci_doe_protocol prot; @@ -153,7 +146,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, { struct pci_dev *pdev = doe_mb->pdev; int offset = doe_mb->cap_offset; - size_t length; + size_t length, remainder; u32 val; int i; @@ -171,7 +164,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, return -EIO; /* Length is 2 DW of header + length of payload in DW */ - length = 2 + task->request_pl_sz / sizeof(u32); + length = 2 + DIV_ROUND_UP(task->request_pl_sz, sizeof(u32)); if (length > PCI_DOE_MAX_LENGTH) return -EIO; if (length == PCI_DOE_MAX_LENGTH) @@ -184,10 +177,20 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, length)); + + /* Write payload */ for (i = 0; i < task->request_pl_sz / sizeof(u32); i++) pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, task->request_pl[i]); + /* Write last payload dword */ + remainder = task->request_pl_sz % sizeof(u32); + if (remainder) { + val = 0; + memcpy(&val, &task->request_pl[i], remainder); + pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val); + } + pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_GO); return 0; @@ -207,11 +210,11 @@ static bool pci_doe_data_obj_ready(struct pci_doe_mb *doe_mb) static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *task) { + size_t length, payload_length, remainder, received; struct pci_dev *pdev = doe_mb->pdev; int offset = doe_mb->cap_offset; - size_t length, payload_length; + int i = 0; u32 val; - int i; /* Read the first dword to get the protocol */ pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); @@ -238,15 +241,34 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas /* First 2 dwords have already been read */ length -= 2; - payload_length = min(length, task->response_pl_sz / sizeof(u32)); - /* Read the rest of the response payload */ - for (i = 0; i < payload_length; i++) { - pci_read_config_dword(pdev, offset + PCI_DOE_READ, - &task->response_pl[i]); + received = task->response_pl_sz; + payload_length = DIV_ROUND_UP(task->response_pl_sz, sizeof(u32)); + remainder = task->response_pl_sz % sizeof(u32); + if (!remainder) + remainder = sizeof(u32); + + if (length < payload_length) { + received = length * sizeof(u32); + payload_length = length; + remainder = sizeof(u32); + } + + if (payload_length) { + /* Read all payload dwords except the last */ + for (; i < payload_length - 1; i++) { + pci_read_config_dword(pdev, offset + PCI_DOE_READ, + &task->response_pl[i]); + pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); + } + + /* Read last payload dword */ + pci_read_config_dword(pdev, offset + PCI_DOE_READ, &val); + memcpy(&task->response_pl[i], &val, remainder); /* Prior to the last ack, ensure Data Object Ready */ - if (i == (payload_length - 1) && !pci_doe_data_obj_ready(doe_mb)) + if (!pci_doe_data_obj_ready(doe_mb)) return -EIO; pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); + i++; } /* Flush excess length */ @@ -260,7 +282,7 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) return -EIO; - return min(length, task->response_pl_sz / sizeof(u32)) * sizeof(u32); + return received; } static void signal_task_complete(struct pci_doe_task *task, int rv) @@ -560,14 +582,6 @@ static int pci_doe_submit_task(struct pci_doe_mb *doe_mb, if (!pci_doe_supports_prot(doe_mb, task->prot.vid, task->prot.type)) return -EINVAL; - /* - * DOE requests must be a whole number of DW and the response needs to - * be big enough for at least 1 DW - */ - if (task->request_pl_sz % sizeof(u32) || - task->response_pl_sz < sizeof(u32)) - return -EINVAL; - if (test_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags)) return -EIO;