[Qemu-devel] megasas: Usage of is_write in megasas_map_sgl()

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

Hi Hannes and Co,

I had a quick question wrt to megassas_map_sgl() ->
cpu_physical_memory_map() and usage of the 'is_write' parameter.

In megasas.v3 code on v0.13.0, this appears as:

static int megasas_map_sgl(struct megasas_cmd_t *cmd, int pa_offset)
{
    int i;
    uint16_t flags = le16_to_cpu(cmd->frame->header.flags);
    int is_sgl64 = (flags & MFI_FRAME_SGL64) ? 1 : 0;
    int is_write = (flags & MFI_FRAME_DIR_WRITE) ? 1 : 0;
    int sgl_addr_size = is_sgl64 ? sizeof(uint64_t) : sizeof(uint32_t);
    size_t iov_count = 0;

    cmd->iov = qemu_mallocz(sizeof(struct iovec) * (cmd->frame->header.sge_count + 1));
    for (i = 0; i < cmd->frame->header.sge_count; i++) {
        target_phys_addr_t pa, iov_pa, iov_size;

        pa = cmd->pa + pa_offset;
        if (is_sgl64)
            iov_pa = ldq_phys(pa);
        else
            iov_pa = ldl_phys(pa);
        iov_size = ldl_phys(pa + sgl_addr_size);
        DPRINTF_IO("iov_pa: %lx iov_size: %d, is_write: %d\n", iov_pa, (int)iov_size, is_write);
        cmd->iov[i].iov_base = cpu_physical_memory_map(iov_pa, &iov_size, is_write);
        cmd->iov[i].iov_len = iov_size;
        pa_offset += sgl_addr_size + sizeof(uint32_t);
        iov_count += iov_size;
    }

    <SNIP>

which I believe is following incoming SCSI CDB WRITE/READ
classification, right..?  So for cpu_physical_memory_map() with the
initial 16-byte INQUIRY with the lastest code, this is set to
is_write=0..

Looking at v0.12.5 code, the is_write is inverted in it's equivilent
form inside hw/scsi-generic.c:scsi_generic_map() using the QEMUSGList
that is being setup in the working version megasas_map_sgl():

static void megasas_map_sgl(struct megasas_cmd_t *cmd, int pa_offset)
{
    int i;
    int is_sgl64 = (cmd->frame->header.flags & MFI_FRAME_SGL64) ? 1 : 0;
    int sgl_addr_size = is_sgl64 ? sizeof(uint64_t) : sizeof(uint32_t);

    qemu_sglist_init(&cmd->sg, cmd->frame->header.sge_count);
    for (i = 0; i < cmd->frame->header.sge_count; i++) {
        target_phys_addr_t pa, iov_pa;

        pa = cmd->pa + pa_offset;
        if (is_sgl64)
            iov_pa = ldq_phys(pa);
        else
            iov_pa = ldl_phys(pa);
        qemu_sglist_add(&cmd->sg, iov_pa, ldl_phys(pa + sgl_addr_size));
        pa_offset += sgl_addr_size + sizeof(uint32_t);
    }
}

to scsi_req_sgl() -> scsi-generic.c:scsi_generic_req_sgl() to down
to scsi_generic_map and the inverted is_write:

static int scsi_generic_map(SCSIGenericReq *r, QEMUSGList *sg)
{
    int is_write = !scsi_req_is_write(&r->req);
    target_phys_addr_t cur_addr, cur_len, cur_offset = 0;
    void *mem;
    int i;

    qemu_iovec_reset(&r->iov);
    for (i = 0; i < sg->nsg;) {
        cur_addr = sg->sg[i].base + cur_offset;
        cur_len = sg->sg[i].len - cur_offset;
        DPRINTF("Using cur_addr: 0x%016lx cur_len: 0x%016lx\n", cur_addr, cur_len);

        mem = cpu_physical_memory_map(cur_addr, &cur_len, is_write);
        if (!mem)
            goto err;

        DPRINTF("Adding iovec for mem: %p len: 0x%016lx\n", mem, cur_len);
        qemu_iovec_add(&r->iov, mem, cur_len);
        cur_offset += cur_len;
        if (cur_offset == sg->sg[i].len) {
            cur_offset = 0;
            i++;
        }
    }
    return 0;

    <SNIP>

and here the first 16-byte INQUIRY appears as is_write=1..

The usage of a inverted is_write with cpu_physical_memory_map() also
seems to be the case in dma-helpers.c:dma_brdv_cb():

	mem = cpu_physical_memory_map(cur_addr, &cur_len, !dbs->is_write);

After changing to an inverted is_write in megasas.v3/megasas-upstream-v1
code, this still does *not* seem to make a difference wrt to the 64-bit
Win7 case, and the same BSOD appears..  In any event, we should verify
this with QEMU folks in terms of what the proper usage of is_write for
cpu_physical_memory_map().  Gerd and Kevin comments here..?

I am also wondering if Gerd's original v0.12.5 SCSI rewrite with
the following logic could be having a subtle affect:

*) megasas_map_sgl() being split up into a QEMUSGList at cmd->sg and
   down to cpu_physical_memory_map() -> qemu_iovec_add(&r->iov, ...)

instead of the new:

*) megasas_map_sgl() using a freshly qemu_malloc()'s cmd->iov buffer
   down to cpu_physical_memory_map() for cmd->iov[i].base, and not using
   an QEMUSGList intermediary.

AFAICT this does not seem to be an issue (not tested with a patch yet),
but wanted to double check that the usage of ld*_phys() for iov_pa and
iov_size directly preceeding cpu_physical_memory_map() is OK in
megasas.v3, and that skipping the QEMUSGList part of the mapping from
megasas_cmd->pa to megas_cmd->sg in v0.12.5 code is not creating some
subtle effect to break some guests..?

Any thoughts guys..?

--nab