Re: [PATCH v2 5/5] hw/nvme: flexible data placement emulation

[Keith Busch]

On Fri, Feb 17, 2023 at 01:07:43PM +0100, Jesper Devantier wrote:
> +static void nvme_do_write_fdp(NvmeCtrl *n, NvmeRequest *req, uint64_t slba,
> +                              uint32_t nlb)
> +{
> +    NvmeNamespace *ns = req->ns;
> +    NvmeRwCmd *rw = (NvmeRwCmd *)&req->cmd;
> +    uint64_t data_size = nvme_l2b(ns, nlb);
> +    uint32_t dw12 = le32_to_cpu(req->cmd.cdw12);
> +    uint8_t dtype = (dw12 >> 20) & 0xf;
> +    uint16_t pid = le16_to_cpu(rw->dspec);
> +    uint16_t ph, rg, ruhid;
> +    NvmeReclaimUnit *ru;
> +
> +    if (dtype != NVME_DIRECTIVE_DATA_PLACEMENT
> +        || !nvme_parse_pid(ns, pid, &ph, &rg)) {

Style nit, the "||" ought to go in the previous line.

> +        ph = 0;
> +        rg = 0;
> +    }
> +
> +    ruhid = ns->fdp.phs[ph];
> +    ru = &ns->endgrp->fdp.ruhs[ruhid].rus[rg];
> +
> +    nvme_fdp_stat_inc(&ns->endgrp->fdp.hbmw, data_size);
> +    nvme_fdp_stat_inc(&ns->endgrp->fdp.mbmw, data_size);
> +
> +    //trace_pci_nvme_fdp_ruh_write(ruh->rgid, ruh->ruhid, ruh->nlb_ruamw, 
> nlb);
> +
> +    while (nlb) {
> +        if (nlb < ru->ruamw) {
> +            ru->ruamw -= nlb;
> +            break;
> +        }
> +
> +        nlb -= ru->ruamw;
> +        //trace_pci_nvme_fdp_ruh_change(ruh->rgid, ruh->ruhid);

Please use the trace points if you find them useful, otherwise just delete
them instead of committing commented out code.

Beyond that, looks good! For the series:

Reviewed-by: Keith Busch <kbusch@kernel.org>