Re: 214df80a9cb3 - main - nvme: new define for size of host memory buffer sizes

-----Original Message-----
From: <owner-src-committers@freebsd.org> on behalf of Warner Losh <imp@FreeBSD.org>
Date: 2022-04-08, Friday at 22:06
To: <src-committers@FreeBSD.org>, <dev-commits-src-all@FreeBSD.org>, <dev-commits-src-main@FreeBSD.org>
Subject: git: 214df80a9cb3 - main - nvme: new define for size of host memory buffer sizes

    The branch main has been updated by imp:

    URL: https://cgit.FreeBSD.org/src/commit/?id=214df80a9cb3e95a140b13af7d19deec2bbfae76

    commit 214df80a9cb3e95a140b13af7d19deec2bbfae76
    Author:     Warner Losh <imp@FreeBSD.org>
    AuthorDate: 2022-04-09 05:01:06 +0000
    Commit:     Warner Losh <imp@FreeBSD.org>
    CommitDate: 2022-04-09 05:05:25 +0000

        nvme: new define for size of host memory buffer sizes

        The nvme spec defines the various fields that specify sizes for host
        memory buffers in terms of 4096 chunks. So, rather than use a bare 4096
        here, use NVME_HMB_UNITS. This is explicitly not the host page size of
        4096, nor the default memory page size (mps) of the NVMe drive, but its
        own thing and needs its own define.

Hi Warner,

Are you sure about that? 

NVMe-1.4, Figure 297: Host Memory Buffer – Host Memory Buffer Descriptor Entry

| Buffer Size (BSIZE): Indicates the number of contiguous
| memory page size (CC.MPS) units for this descriptor.
| 
| Buffer Address (BADD): Indicates the host memory address for
| this descriptor aligned to the memory page size (CC.MPS). 
| The lower bits (n:0) of this field indicate the offset
| within the memory page is 0h (e.g., if the memory page size
| is 4 KiB, then bits 11:00 shall be 0h; if the memory page
| size is 8 KiB, then bits 12:00 shall be 0h).

They both reference mps, not 4096 bytes.

-Ravi (rpokala@)

        No functional change is intended, only the logical spelling of 4k.

        Sponsored by:           Netflix
    ---
     sys/dev/nvme/nvme.h       | 3 +++
     sys/dev/nvme/nvme_ctrlr.c | 8 ++++----
     2 files changed, 7 insertions(+), 4 deletions(-)

    diff --git a/sys/dev/nvme/nvme.h b/sys/dev/nvme/nvme.h
    index f2ef2467c9b1..55e94c40dd2c 100644
    --- a/sys/dev/nvme/nvme.h
    +++ b/sys/dev/nvme/nvme.h
    @@ -62,6 +62,9 @@
     /* Cap transfers by the maximum addressable by page-sized PRP (4KB -> 2MB). */
     #define NVME_MAX_XFER_SIZE		MIN(maxphys, (PAGE_SIZE/8*PAGE_SIZE))

    +/* Host memory buffer sizes are always in 4096 byte chunks */
    +#define	NVME_HMB_UNITS			4096
    +
     /* Register field definitions */
     #define NVME_CAP_LO_REG_MQES_SHIFT			(0)
     #define NVME_CAP_LO_REG_MQES_MASK			(0xFFFF)
    diff --git a/sys/dev/nvme/nvme_ctrlr.c b/sys/dev/nvme/nvme_ctrlr.c
    index 95a2b5c4285d..6996b3151b0d 100644
    --- a/sys/dev/nvme/nvme_ctrlr.c
    +++ b/sys/dev/nvme/nvme_ctrlr.c
    @@ -936,11 +936,11 @@ nvme_ctrlr_hmb_alloc(struct nvme_controller *ctrlr)
     	max = (uint64_t)physmem * PAGE_SIZE / 20;
     	TUNABLE_UINT64_FETCH("hw.nvme.hmb_max", &max);

    -	min = (long long unsigned)ctrlr->cdata.hmmin * 4096;
    +	min = (long long unsigned)ctrlr->cdata.hmmin * NVME_HMB_UNITS;
     	if (max == 0 || max < min)
     		return;
    -	pref = MIN((long long unsigned)ctrlr->cdata.hmpre * 4096, max);
    -	minc = MAX(ctrlr->cdata.hmminds * 4096, PAGE_SIZE);
    +	pref = MIN((long long unsigned)ctrlr->cdata.hmpre * NVME_HMB_UNITS, max);
    +	minc = MAX(ctrlr->cdata.hmminds * NVME_HMB_UNITS, PAGE_SIZE);
     	if (min > 0 && ctrlr->cdata.hmmaxd > 0)
     		minc = MAX(minc, min / ctrlr->cdata.hmmaxd);
     	ctrlr->hmb_chunk = pref;
    @@ -1023,7 +1023,7 @@ again:
     	for (i = 0; i < ctrlr->hmb_nchunks; i++) {
     		ctrlr->hmb_desc_vaddr[i].addr =
     		    htole64(ctrlr->hmb_chunks[i].hmbc_paddr);
    -		ctrlr->hmb_desc_vaddr[i].size = htole32(ctrlr->hmb_chunk / 4096);
    +		ctrlr->hmb_desc_vaddr[i].size = htole32(ctrlr->hmb_chunk / NVME_HMB_UNITS);
     	}
     	bus_dmamap_sync(ctrlr->hmb_desc_tag, ctrlr->hmb_desc_map,
     	    BUS_DMASYNC_PREWRITE);