On Thu, Jun 25, 2020 at 01:31:56PM +0000, Lin Ma wrote: > On 2020-06-25 21:25, Lin Ma wrote: > >> + /* > >> + * 8 + 16 is the length in bytes of response header and > >> + * one LBA status descriptor > >> + */ > >> + memset(outbuf, 0, 8 + 16); > >> + outbuf[3] = 20; > >> + outbuf[8] = (req->cmd.lba >> 56) & 0xff; > >> + outbuf[9] = (req->cmd.lba >> 48) & 0xff; > >> + outbuf[10] = (req->cmd.lba >> 40) & 0xff; > >> + outbuf[11] = (req->cmd.lba >> 32) & 0xff; > >> + outbuf[12] = (req->cmd.lba >> 24) & 0xff; > >> + outbuf[13] = (req->cmd.lba >> 16) & 0xff; > >> + outbuf[14] = (req->cmd.lba >> 8) & 0xff; > >> + outbuf[15] = req->cmd.lba & 0xff; > >> + outbuf[16] = (*num_blocks >> 24) & 0xff; > >> + outbuf[17] = (*num_blocks >> 16) & 0xff; > >> + outbuf[18] = (*num_blocks >> 8) & 0xff; > >> + outbuf[19] = *num_blocks & 0xff; > >> + outbuf[20] = *is_deallocated ? 1 : 0; > > > > SCSI defines 3 values and QEMU can represent all of them: > > > > 0 - mapped or unknown > > 1 - deallocated > > 2 - anchored > > > > See the BDRV_BLOCK_* constants in include/block/block.h. The > > is_deallocated boolean is not enough to represent this state, but the > > bdrv_block_status() return value can be used instead. > > I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'. > It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored', > > I'd like to use these combinations to analyze the bdrv_block_status() return > value: > 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | > BDRV_BLOCK_ZERO > 'anchored': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! > BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA > Am I right?
My understanding is that the SCSI status are mapped to QEMU block status as follows: allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID deallocated: !BDRV_BLOCK_DATA I have CCed Eric Blake, who is familiar with block status. > >> +} > >> + > >> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf) > >> { > >> SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req); > >> @@ -2076,6 +2159,13 @@ static int32_t > >> scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf) > >> > >> /* Protection, exponent and lowest lba field left blank. */ > >> break; > >> + } else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) { > >> + if (req->cmd.lba > s->qdev.max_lba) { > >> + goto illegal_lba; > >> + } > >> + scsi_disk_emulate_get_lba_status(req, outbuf); > >> + r->iov.iov_len = req->cmd.xfer; > >> + return r->iov.iov_len; > > > > Is there something tricky going on here with iov_len that prevents us > > from using break here and sharing the functions normal return code path? > > Using 'break' here and following the normal return code path cause the > assertion > 'assert(!r->req.aiocb)'. > > In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in > function > scsi_disk_emulate_command, It causes the same assertion if no this 'return' > statement. > I borrowed this idea to avoid the assertion. The assertion shows that this patch isn't asynchronous. I think the assertion will pass once you convert GET_LBA_STATUS to async and then a break statement will work.
signature.asc
Description: PGP signature