Re: [Qemu-devel] [PATCHv2 3/4] scsi: clarify sense codes for LUN0 emulation

[Laszlo Ersek <lersek@redhat.com>]

On 08/17/17 22:57, Laszlo Ersek wrote:
> On 08/04/17 12:49, Paolo Bonzini wrote:
>> On 04/08/2017 10:36, Hannes Reinecke wrote:
>>> The LUN0 emulation is just that, an emulation for a non-existing
>>> LUN0. So we should be returning LUN_NOT_SUPPORTED for any request
>>> coming from any other LUN.
>>> And we should be aborting unhandled commands with INVALID OPCODE,
>>> not LUN NOT SUPPORTED.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>>> ---
>>>  hw/scsi/scsi-bus.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>>> index 8419c75..79a222f 100644
>>> --- a/hw/scsi/scsi-bus.c
>>> +++ b/hw/scsi/scsi-bus.c
>>> @@ -583,6 +583,11 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>  {
>>>      SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
>>>  
>>> +    if (req->lun != 0) {
>>> +        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>> +        scsi_req_complete(req, CHECK_CONDITION);
>>> +        return 0;
>>> +    }
>>>      switch (buf[0]) {
>>>      case REPORT_LUNS:
>>>          if (!scsi_target_emulate_report_luns(r)) {
>>> @@ -613,7 +618,7 @@ static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
>>>      case TEST_UNIT_READY:
>>>          break;
>>>      default:
>>> -        scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
>>> +        scsi_req_build_sense(req, SENSE_CODE(INVALID_OPCODE));
>>>          scsi_req_complete(req, CHECK_CONDITION);
>>>          return 0;
>>>      illegal_request:
>>>
>>
>> I am queuing this one since it's an independent bugfix.
> 
> This patch (ded6ddc5a7b9, "scsi: clarify sense codes for LUN0
> emulation", 2017-08-04) seems to confuse the media detection in edk2's
> "MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c".
> 
> Namely, when it enumerates the {targets}x{LUNs} matrix on the
> virtio-scsi HBA, it now reports the following message, for each
> (target,LUN) pair to which no actual SCSI device (like disk or CD-ROM)
> is assigned on the command line:
> 
> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
> 
> Unfortunately, this is not all that happens -- the ScsiDiskDxe driver
> even installs a BlockIo protocol instance on the handle (again, there is
> no media, and no actual SCSI device), on which further protocols are
> stacked, such as BlockIo2:
> 
> ScsiDisk: Sense Key = 0x5 ASC = 0x25!
> InstallProtocolInterface: [EfiBlockIoProtocol] 13A59A3A8
> InstallProtocolInterface: [EfiBlockIo2Protocol] 13A59A3D8
> InstallProtocolInterface: [EfiDiskInfoProtocol] 13A59A4D0
> 
> In turn, in BDS, UEFI boot options are auto-generated for these devices,
> which is not nice, given that this procedure in BDS is very
> pflash-intensive, and pflash access is remarkably slow on aarch64 KVM.
> 
> For example, if I use one virtio-scsi HBA, and put a CD-ROM on target 0,
> LUN 0, and a disk on target 1, LUN 0, then edk2 will create protocol
> interfaces, and matching boot options, for
> 
>   2 targets * 7 LUNs/target = 14 LUNs
> 
> of which only 2 make sense.
> 
> 
> If I revert the patch (on top of v2.10.0-rc3), then everything works as
> before -- BlockIo protocol instances are produced only for actual
> devices (with media).
> 
> I guess the path forward is to fix the ScsiDiskDxe driver in edk2; the
> new ASC should be recognized.
> 
> My question is, how *exactly* did this patch change the reported sense
> key and ASC? That is, what did they use to be *before*? INVALID_OPCODE?

I found the bug in edk2. It's a missing error check. I'll send a patch
and CC you guys.

Thanks
Laszlo