Hi, Laszlo After checked history, I don't know why these old logic were introduced.
R8330 is: ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); Cdb[1] = (UINT8) (Lun & EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); R8331 removed these two lines. But R8334 added it back with modified semantics: ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); Cdb[1] = (UINT8) ((Lun << 5) & EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); <- LUN is shifted towards left... don't know why... I think R8334 and even R8330 are totally wrong. I didn't see any spec which explained the usage of LUN should like this to fill CDB1 (if I missed something, please let me know). My personal suggestion would be removing them and roll back to R8331. UefiScsiLib doesn't support read/write those protected info. Thanks Feng -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Saturday, November 22, 2014 14:35 To: edk2-devel list; Olivier Martin; Kinney, Michael D; Justen, Jordan L; Cole Robinson Subject: [edk2] [PATCH 1/3] MdePkg: UefiScsiLib: turn 'LUN encoding in CDB' into an explicit feature The "SCSI Block Commands - 2" (SBC-2) standard defines bits [7:5] of the CDB byte 1 as Reserved, for the READ, WRITE, and VERIFY commands. The updated "SCSI Block Commands - 3" (SBC-3) standard defined the same bit-fields as RDPROTECT, WRPROTECT, and VRPROTECT, respectively. Historically hardware has existed that was based on SBC-2, but required driver software to encode the Logical Unit Number (LUN) in the above -- at that time Reserved -- bit-fields. (Thereby forcing the driver to violate even SBC-2). Such a LUN encoding creates nonsensical RDPROTECT, WRPROTECT, and VRPROTECT values for devices that conform to the SBC-3. In particular, if the encoded value is nonzero (corresponding to a nonzero LUN), and the recipient device does not support protection information, then it is *required* to reject the command 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 UefiScsiLib has always encoded the LUN in the CDB for these commands. SVN r8331 (git commit 676e2a32) disabled it briefly, but SVN r8334 (git commit 6b3ecf5c) reenabled it just a few hours later. Unfortunately, the current code breaks on devices that follow the above SBC-3 requirement; for example on SCSI disks with nonzero LUNs emulated by QEMU (after QEMU commit 96bdbbab, part of v1.2.0). Turn the 'LUN encoding in CDB' feature into a Feature PCD, so that edk2 platforms can control it explicitly. The default value preserves the present behavior. RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1166971 Reported-by: Cole Robinson <crobi...@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <ler...@redhat.com> --- MdePkg/Library/UefiScsiLib/UefiScsiLib.inf | 2 ++ MdePkg/Library/UefiScsiLib/UefiScsiLib.c | 16 ++++++++++++---- MdePkg/MdePkg.dec | 9 +++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/MdePkg/Library/UefiScsiLib/UefiScsiLib.inf b/MdePkg/Library/UefiScsiLib/UefiScsiLib.inf index 1eb9076..a2e8618 100644 --- a/MdePkg/Library/UefiScsiLib/UefiScsiLib.inf +++ b/MdePkg/Library/UefiScsiLib/UefiScsiLib.inf @@ -43,3 +43,5 @@ DebugLib BaseLib +[FeaturePcd] + gEfiMdePkgTokenSpaceGuid.PcdScsiReadWriteVerifyCdbLunEncoding ## +CONSUMES diff --git a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c index bd838c4..f6a1146 100644 --- a/MdePkg/Library/UefiScsiLib/UefiScsiLib.c +++ b/MdePkg/Library/UefiScsiLib/UefiScsiLib.c @@ -930,7 +930,9 @@ ScsiRead10Command ( ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); Cdb[0] = EFI_SCSI_OP_READ10; - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); + if (FeaturePcdGet (PcdScsiReadWriteVerifyCdbLunEncoding)) { + 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 +1030,9 @@ ScsiWrite10Command ( ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); Cdb[0] = EFI_SCSI_OP_WRITE10; - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); + if (FeaturePcdGet (PcdScsiReadWriteVerifyCdbLunEncoding)) { + 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 +1130,9 @@ ScsiRead16Command ( ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); Cdb[0] = EFI_SCSI_OP_READ16; - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); + if (FeaturePcdGet (PcdScsiReadWriteVerifyCdbLunEncoding)) { + 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 +1230,9 @@ ScsiWrite16Command ( ScsiIo->GetDeviceLocation (ScsiIo, &Target, &Lun); Cdb[0] = EFI_SCSI_OP_WRITE16; - Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); + if (FeaturePcdGet (PcdScsiReadWriteVerifyCdbLunEncoding)) { + Cdb[1] = (UINT8) (LShiftU64 (Lun, 5) & EFI_SCSI_LOGICAL_UNIT_NUMBER_MASK); + } WriteUnaligned64 ((UINT64 *)&Cdb[2], SwapBytes64 (StartLba)); WriteUnaligned32 ((UINT32 *)&Cdb[10], SwapBytes32 (SectorSize)); diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index 6dc696e..5359b59 100644 --- a/MdePkg/MdePkg.dec +++ b/MdePkg/MdePkg.dec @@ -1428,6 +1428,15 @@ # @Prompt Validate ORDERED_COLLECTION structure gEfiMdePkgTokenSpaceGuid.PcdValidateOrderedCollection|FALSE|BOOLEAN|0x0000002a + ## Determines if UefiScsiLib encodes the Logical Unit Number (LUN) in + bits # [7:5] in byte 1 of the Command Descriptor Block (CDB) for the + the READ, # WRITE, VERIFY commands.<BR><BR> + # TRUE - Encode the LUN in bits [7:5] of Cdb[1] for READ, WRITE, VERIFY.<BR> + # FALSE - Do not encode the LUN in bits [7:5] of Cdb[1] for READ, WRITE, + # VERIFY.<BR> + # @Prompt Encode LUN in CDB for READ, WRITE, VERIFY. + + gEfiMdePkgTokenSpaceGuid.PcdScsiReadWriteVerifyCdbLunEncoding|TRUE|BOO + LEAN|0x0000002b + [PcdsFixedAtBuild] ## Status code value for indicating a watchdog timer has expired. # EFI_COMPUTING_UNIT_HOST_PROCESSOR | EFI_CU_HP_EC_TIMER_EXPIRED -- 1.8.3.1 ------------------------------------------------------------------------------ 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
