Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size

From: Lukas Wunner <lukas@wunner.de>
To: Ira Weiny <ira.weiny@intel.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	linux-pci@vger.kernel.org,
	Gregory Price <gregory.price@memverge.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	"Li, Ming" <ming4.li@intel.com>, Hillf Danton <hdanton@sina.com>,
	Ben Widawsky <bwidawsk@kernel.org>,
	linuxarm@huawei.com, linux-cxl@vger.kernel.org
Subject: Re: [PATCH v2 10/10] PCI/DOE: Relax restrictions on request and response size
Date: Fri, 10 Feb 2023 22:47:27 +0100	[thread overview]
Message-ID: <20230210214727.GB15326@wunner.de> (raw)
In-Reply-To: <63cf37d8f30e3_5cca294fb@iweiny-mobl.notmuch>

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