FEMU icon indicating copy to clipboard operation
FEMU copied to clipboard

FEMU ocssd code issue

Open anjinsung opened this issue 3 years ago • 1 comments

  1. pr_lba function in femu/hw/ocssd/oc20.c,
#ifdef DEUBUG_OC20
static void pr_lba(Oc20Namespace *lns, uint64_t lba)
{
    Oc20AddrF *addrf = &lns->lbaf;

    uint64_t grp = OC20_LBA_GET_SECTR(addrf, lba);
    uint64_t lun = OC20_LBA_GET_CHUNK(addrf, lba);
    uint64_t chk = OC20_LBA_GET_PUNIT(addrf, lba);
    uint64_t sec = OC20_LBA_GET_GROUP(addrf, lba);

    femu_log("LBA(0x%lx): ch(%lu), lun(%lu), blk(%lu), sec(%lu)\n", lba, grp,
             lun, chk, sec);
}
#endif

OC20_LBA_GET_SECTR mismatched grp, other variables mismatched as well.

  1. oc20_rw in femu/hw/ocssd/oc20.c,
static uint16_t oc20_rw(FemuCtrl *n, NvmeCmd *cmd, NvmeRequest *req, bool vector)
{
    Oc20RwCmd *lrw = (Oc20RwCmd *)cmd;
    NvmeNamespace *ns = cmd_ns(n, cmd);
    uint64_t prp1 = le64_to_cpu(lrw->dptr.prp1);
    uint64_t prp2 = le64_to_cpu(lrw->dptr.prp2);
    uint32_t nlb  = le16_to_cpu(lrw->nlb) + 1;
    uint64_t lbal = le64_to_cpu(lrw->lbal);
    int lbads = NVME_ID_NS_LBADS(ns);
    uint16_t err;

    if (nlb > OC20_CMD_MAX_LBAS) {
        nvme_set_error_page(n, req->sq->sqid, req->cqe.cid, NVME_INVALID_FIELD,
                            offsetof(Oc20RwCmd, lbal), 0, req->ns->id);
        return NVME_INVALID_FIELD | NVME_DNR;
    }

    req->predef = 0;
    req->nlb = nlb;
    req->slba = (uint64_t)g_malloc0(sizeof(uint64_t) * nlb);
    req->is_write = oc20_rw_is_write(req) ? true : false;

    if (vector) {
        if (nlb > 1) {
            uint32_t len = nlb * sizeof(uint64_t);
            nvme_addr_read(n, lbal, (void *)req->slba, len);
        } else {
            ((uint64_t *)req->slba)[0] = lbal;
        }
    } else { /* For SPDK quirks */
        for (int i = 0; i < nlb; i++) {
            ((uint64_t *)req->slba)[i] = lbal + i;
        }
    }

    err = oc20_rw_check_vector_req(n, cmd, req);
    if (err) {
        goto fail_free;
    }

    if (nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, nlb << lbads, n)) {
        femu_err("%s,malformed prp\n", __func__);
        err = NVME_INVALID_FIELD | NVME_DNR;
        goto fail_free;
    }

    uint64_t aio_sector_list[OC20_CMD_MAX_LBAS];
    for (int i = 0; i < nlb; i++) {
#ifdef DEBUG_OC20
        pr_lba(lns, ((uint64_t *)req->slba)[i]);
#endif
        aio_sector_list[i] = (((uint64_t *)req->slba)[i] << lbads);
    }
    backend_rw(n->mbe, &req->qsg, aio_sector_list, req->is_write);

    oc20_advance_status(n, ns, cmd, req);

    if (req->is_write) {
        oc20_advance_wp(n, ns, ((uint64_t *)req->slba)[0], nlb, req);
    }

    g_free((void *)req->slba);

    return NVME_SUCCESS;

fail_free:
    g_free((void *)req->slba);
    return err;
}

line : pr_lba(lns, ((uint64_t *)req->slba)[i]); lns is not defined for that function. It must be declared at the top or changed to ns->state for it to work.

Also, pr_lba is DEUBUG_OC20, in the middle of oc20_rw is DEBUG_OC20, they have different #ifdef. I wonder if it's just a typo or an intentional distinction.

anjinsung avatar Jun 30 '22 13:06 anjinsung

Good catch. Thanks for reporting this! Interested in submitting a patch to fix this?

DEUBUG_OC20 is a typo :)

huaicheng avatar Aug 31 '22 21:08 huaicheng