Re: [PATCH 03/14] cxl/mem: Find device capabilities

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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>,
	Ira Weiny <ira.weiny@intel.com>, Jon Masters <jcm@jonmasters.org>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Rafael Wysocki <rafael.j.wysocki@intel.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Vishal Verma <vishal.l.verma@intel.com>,
	daniel.lll@alibaba-inc.com,
	"John Groves (jgroves)" <jgroves@micron.com>,
	"Kelley, Sean V" <sean.v.kelley@intel.com>
Subject: Re: [PATCH 03/14] cxl/mem: Find device capabilities
Date: Mon, 1 Feb 2021 13:08:49 -0500	[thread overview]
Message-ID: <20210201180849.GH197521@fedora> (raw)
In-Reply-To: <20210201175041.qs56jk5tdbgn2zia@intel.com>

On Mon, Feb 01, 2021 at 09:50:41AM -0800, Ben Widawsky wrote:
> On 21-02-01 12:41:36, Konrad Rzeszutek Wilk wrote:
> > > +static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> > > +{
> > > +	struct device *dev = &cxlm->pdev->dev;
> > > +	int cap, cap_count;
> > > +	u64 cap_array;
> > > +
> > > +	cap_array = readq(cxlm->regs + CXLDEV_CAP_ARRAY_OFFSET);
> > > +	if (CXL_GET_FIELD(cap_array, CXLDEV_CAP_ARRAY_ID) != CXLDEV_CAP_ARRAY_CAP_ID)
> > > +		return -ENODEV;
> > > +
> > > +	cap_count = CXL_GET_FIELD(cap_array, CXLDEV_CAP_ARRAY_COUNT);
> > > +
> > > +	for (cap = 1; cap <= cap_count; cap++) {
> > > +		void __iomem *register_block;
> > > +		u32 offset;
> > > +		u16 cap_id;
> > > +
> > > +		cap_id = readl(cxlm->regs + cap * 0x10) & 0xffff;
> > > +		offset = readl(cxlm->regs + cap * 0x10 + 0x4);
> > > +		register_block = cxlm->regs + offset;
> > > +
> > > +		switch (cap_id) {
> > > +		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
> > > +			dev_dbg(dev, "found Status capability (0x%x)\n",
> > > +				offset);
> > 
> > That 80 character limit is no longer a requirement. Can you just make
> > this one line? And perhaps change 'found' to 'Found' ?
> > 
> 
> Funny that.
> https://lore.kernel.org/linux-cxl/20201111073449.GA16235@infradead.org/

 "If there is a good reason to go against the
style (a line which becomes far less readable if split to fit within the
80-column limit, for example), just do it.
"

I would say that having an offset on its own line is kind of silly.

> 
> > > +			cxlm->status.regs = register_block;
> > > +			break;
> > > +		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
> > > +			dev_dbg(dev, "found Mailbox capability (0x%x)\n",
> > > +				offset);
> > > +			cxlm->mbox.regs = register_block;
> > > +			break;
> > > +		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
> > > +			dev_dbg(dev,
> > > +				"found Secondary Mailbox capability (0x%x)\n",
> > > +				offset);
> > > +			break;
> > > +		case CXLDEV_CAP_CAP_ID_MEMDEV:
> > > +			dev_dbg(dev, "found Memory Device capability (0x%x)\n",
> > > +				offset);
> > > +			cxlm->mem.regs = register_block;
> > > +			break;
> > > +		default:
> > > +			dev_warn(dev, "Unknown cap ID: %d (0x%x)\n", cap_id,
> > > +				 offset);
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (!cxlm->status.regs || !cxlm->mbox.regs || !cxlm->mem.regs) {
> > > +		dev_err(dev, "registers not found: %s%s%s\n",
> > > +			!cxlm->status.regs ? "status " : "",
> > > +			!cxlm->mbox.regs ? "mbox " : "",
> > > +			!cxlm->mem.regs ? "mem" : "");
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
> > > +{
> > > +	const int cap = cxl_read_mbox_reg32(cxlm, CXLDEV_MB_CAPS_OFFSET);
> > > +
> > > +	cxlm->mbox.payload_size =
> > > +		1 << CXL_GET_FIELD(cap, CXLDEV_MB_CAP_PAYLOAD_SIZE);
> > > +
> > 
> > I think the static analyzers are not going to be happy that you are not
> > checking the value of `cap` before using it.
> > 
> > Perhaps you should check that first before doing the manipulations?
> > 
> 
> I'm not following the request. CXL_GET_FIELD is just doing the shift and mask on
> cap.
> 
> Can you explain what you're hoping to see?

My thoughts were that if cxl_read_mbox_reg32 gave you -1 (would be wacky
but we live in the world of having a healthy vision of devices not
always giving right values).

Then your payload_size bit shifting will get bent out of shape as you
are effectively casting cap to unsigned, which means it will end up
being 0xfffffffffffffffff.. and your bit shifting end up with a bunch
of zeros at the end.
> 
> > > +	/* 8.2.8.4.3 */
> > > +	if (cxlm->mbox.payload_size < 256) {

If this ends up being casted back to signed, this conditional will
catch it (-4096 < 256, true).

But if it does not (so 0xffffffff<256, false), then this wacky
number will pass this check and you may reference a payload_size that is
larger than the reality and copy the wrong set of values (buffer
overflow).

So what I was thinking is that you want to check `cap` to make sure it
is not negative nor to large?

> > 
> > #define for 256?