Re: [PATCH] nvme: allow use of cmb on v1.4 controllers

On Jan 14 18:33, Christoph Hellwig wrote:

On Thu, Jan 14, 2021 at 02:24:41PM +0100, Klaus Jensen wrote:

quoted

From: Klaus Jensen <redacted>

Since NVMe v1.4 the Controller Memory Buffer must be explicitly enabled
by the host.

Thanks, this has been on my TODO list for a while.

quoted

+	vs = readl(dev->bar + NVME_REG_VS);
+
+	if (vs >= NVME_VS(1, 4, 0)) {
+		if (!NVME_CAP_CMBS(dev->ctrl.cap))
+			return;

We should not check the version here, but the presence of the new
register so that controllers claiming an older version complicance
that implement the new scheme are supported as well.

Understood!

Given that the old scheme is highly dangerous in virtualized enviroments
we should also warn about it and eventually required an explicit opt-in
to use it.

Not completely sure what warning you'd want, so I'll leave it to you to
add an 'else dev_warn(...)'. :)

quoted

+	if (vs >= NVME_VS(1, 4, 0)) {
+		cmbmsc = pci_resource_start(pdev, bar);
+		cmbmsc |= (NVME_CMBMSC_CRE | NVME_CMBMSC_CMSE);
+
+		/*
+		 * Writing the low bits may cause the controller to check the
+		 * validity of the 52 bit controller base address in the CMBMSC
+		 * register, so write the high bits first.
+		 */
+		writel(cmbmsc >> 32, dev->bar + NVME_REG_CMBMSC + 4);
+		writel(cmbmsc, dev->bar + NVME_REG_CMBMSC);

I think we should just use writeq here, with the hilo wrapper for 32-bit
platforms here.  The reason why NVMe has so avoided the 64-bit access
is for buggy Apple controllers, but they are unlikely to implement CMB.

Right, I tried writeq, but on QEMU I observed two 4 byte writes in wrong
order anyway.

hi_lo_writeq looks good.