Hi, Laszlo Shall we cleanup all of APIs in UefiScsiLib? Looks like TEST_UNIT_READY, INQUIRY, MODE_SENSE, REQUEST_SENSE and READ_CAPACITY cmds all have same issue.
Thanks Feng -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, November 25, 2014 18:47 To: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [PATCH v2] MdePkg: UefiScsiLib: do not encode LUN in CDB for READ and WRITE On 11/24/14 20:34, Laszlo Ersek wrote: > The "SCSI Block Commands - 2" (SBC-2) standard defines bits [7:5] of > the CDB byte 1 as Reserved, for the READ and WRITE commands. > > The updated "SCSI Block Commands - 3" (SBC-3) standard defines the > same bitfield as RDPROTECT and WRPROTECT, respectively. > > After reviewing the above standards, and the following commits: > - SVN r8331 (git 676e2a32), > - SVN r8334 (git 6b3ecf5c), > we've determined that UefiScsiLib is incorrect in encoding the LUN in > this bitfield for the READ and WRITE commands. > > Encoding a nonzero LUN there creates unintended RDPROTECT and > WRPROTECT values, which the recipient device is required to reject if > it does not support protection information, with CHECK CONDITION, > ILLEGAL REQUEST, INVALID FIELD IN CDB: > > ScsiDiskRead10: Check Condition happened! > ScsiDisk: Sense Key = 0x5 ASC = 0x24! > ScsiDiskRead10: Check Condition happened! > ScsiDisk: Sense Key = 0x5 ASC = 0x24! > ScsiDiskRead10: Check Condition happened! > ScsiDisk: Sense Key = 0x5 ASC = 0x24! > ScsiDiskRead10: Check Condition happened! > ScsiDisk: Sense Key = 0x5 ASC = 0x24! > FatOpenDevice: read of part_lba failed Device Error > > In practice this flaw breaks UefiScsiLib minimally on SCSI disks with > nonzero LUNs that are emulated by QEMU (after QEMU commit 96bdbbab, > part of v1.2.0). > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1166971 > Reported-by: Cole Robinson <crobi...@redhat.com> > Suggested-by: Paolo Bonzini <pbonz...@redhat.com> > Suggested-by: Feng Tian <feng.t...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > > Notes: > v2: > - permanently remove the LUN encoding [Paolo, Feng] > - > https://github.com/lersek/edk2/commits/bz1166971_rdwrvr_cdb_lun_encode > _v2 > > MdePkg/Library/UefiScsiLib/UefiScsiLib.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c > b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c > index bd838c4..dc1847f 100644 > --- a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c > +++ b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c > @@ -930,7 +930,6 @@ ScsiRead10Command ( > ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); > > Cdb[0] = EFI_SCSI_OP_READ10; > - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & > EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); > WriteUnaligned32 ((UINT32 *)&Cdb[2], SwapBytes32 (StartLba)); > WriteUnaligned16 ((UINT16 *)&Cdb[7], SwapBytes16 ((UINT16) > SectorSize)); > > @@ -1028,7 +1027,6 @@ ScsiWrite10Command ( > ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); > > Cdb[0] = EFI_SCSI_OP_WRITE10; > - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & > EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); > WriteUnaligned32 ((UINT32 *)&Cdb[2], SwapBytes32 (StartLba)); > WriteUnaligned16 ((UINT16 *)&Cdb[7], SwapBytes16 ((UINT16) > SectorSize)); > > @@ -1126,7 +1124,6 @@ ScsiRead16Command ( > ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); > > Cdb[0] = EFI_SCSI_OP_READ16; > - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & > EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); > WriteUnaligned64 ((UINT64 *)&Cdb[2], SwapBytes64 (StartLba)); > WriteUnaligned32 ((UINT32 *)&Cdb[10], SwapBytes32 (SectorSize)); > > @@ -1224,7 +1221,6 @@ ScsiWrite16Command ( > ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); > > Cdb[0] = EFI_SCSI_OP_WRITE16; > - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & > EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); > WriteUnaligned64 ((UINT64 *)&Cdb[2], SwapBytes64 (StartLba)); > WriteUnaligned32 ((UINT32 *)&Cdb[10], SwapBytes32 (SectorSize)); > > Self-NAK, we can now kill a few dead variables and a dead function call too, as Feng indicated earlier; reverting a larger portion of SVN r8334. v3 coming up. Laszlo ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
