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
