On 27/03/2020 16:35, Laszlo Ersek wrote:
On 03/27/20 01:05, Liran Alon wrote:
On 27/03/2020 0:17, Laszlo Ersek wrote:
On 03/25/20 17:10, Liran Alon wrote:
PvScsiRestorePciAttributes (Dev);
diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
index 6d23b6e1eccf..7f91d70fec79 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.h
+++ b/OvmfPkg/PvScsiDxe/PvScsi.h
@@ -31,6 +31,11 @@ typedef struct {
PVSCSI_DMA_DESC RingCmpsDmaDesc;
} PVSCSI_RING_DESC;
+typedef struct {
+ UINT8 SenseData[MAX_UINT8];
(4) Is the maximum possible size of the sense data specified
somewhere? If so, it would be nice to document it with a comment at
least.
This is a good point. Thanks for pointing it out.
Turns out "SCSI Commands Reference Manual" section 2.4.1.1.1
Descriptor format sense data overview specifies:
"The additional sense length shall be less than or equal to 244 (i.e.,
limiting the total length of the sense data to 252 bytes)"
i.e. The max possible size of sense data is 252.
As another evidence, I saw QEMU's include/hw/scsi/scsi.h indeed
defining:
#define SCSI_SENSE_BUF_SIZE 252
This change was done by QEMU commit c5f52875b980 ("scsi: Change scsi
sense buf size to 252") which says in it's commit message:
"According to SPC-4, section 4.5.2.1, 252 is the limit of sense data."
Interestingly, this QEMU commit changed the value of
SCSI_SENSE_BUF_SIZE from 96 to 252.
But then I found this in EDK2
OvmfPkg/Include/IndustryStandard/VirtioScsi.h:
//
// We expect these maximum sizes from the host. Also we force the
CdbLength and
// SenseDataLength parameters of
EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() not
// to exceed these limits. See UEFI 2.3.1 errata C 14.7.
//
#define VIRTIO_SCSI_CDB_SIZE 32
#define VIRTIO_SCSI_SENSE_SIZE 96
Which seems to be wrong...
No, these macros / limits are valid. They match the virtio specification
(both 0.9.5 and 1.0) -- they match the "cdb_size" and "sense_size"
configuration values that the virtio-scsi device is required to expose
by default.
While the driver could theoretically ask for larger sizes, these values
have never caused a problem in practice. I don't see a reason to change
the constants (let alone to complicate the code).
It seems most appropriate to add a SCSI_MAX_SENSE_SIZE constant to
EDK2 MdePkg/Include/IndustryStandard/Scsi.h.
And then use that from my PvScsi driver.
I can also submit a separate patch (Not part of this series) to remove
this VIRTIO_SCSI_SENSE_SIZE constant.
Please don't; there's no reason to change the VirtioScsiDxe driver.
Note that VirtioScsi doesn't have a technical issue here because it
provides this max sense length to the device in VirtioScsiInit():
Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
Right, and even those config writes are mostly documentation / "just to
be sure" oriented. Because, there's also a comment in those parts:
//
// We expect these maximum sizes from the host. Since they are
// guest-negotiable, ask for them rather than just checking them.
//
So if it's OK by you, I think I will just add a patch to this series
defining SCSI_MAX_SENSE_SIZE in
MdePkg/Include/IndustryStandard/Scsi.h, and then rely on that when
defining SenseData in PVSCSI_DMA_BUFFER.
My experience tells me that making any OvmfPkg change dependent on new
patches for the core modules (MdePkg, MdeModulePkg, UefiCpuPkg, ...)
slows down the upstreaming of the OvmfPkg change significantly.
Therefore, I would strongly advise against this ordering of changes.
Today, upon reviewing patch#15, I commented again on the "SenseData"
array size (see under point (2) in my patch#15 feedback). Accordingly,
for now, please stick with the existent "SenseData" array declaration,
just add a comment. I expect / hope to merge your v3 posting.
As an alternative, I can also define SenseData to be of size 252 (which
I would #define as a constant in PvScsi.h),
and explain that it is the limit of total length of SenseData according
to SCSI specification.
And indeed leave for a separate patch-series to move this PvScsi.h
private constant and put it in the generic
IndustryStandard/Scsi.h header file.
I think it's nicer than just defining SenseData to be MAX_UINT8 because
of the field size in PassThru packet.
But I will of course do as you prefer. Just let me know your choice :)
(Side comment: I would also like to ask Nikita to R-b the full final
patch set on the list, once I'm ready to merge the series, because I'd
like to testify to Nikita's review effort in the upstream git history.)
Ok. I will let him know to review my v3 submission upstream.
-Liran
Then, with the series merged, you can propose a separate patch series (2
patches), later -- introducing the new constant in MdePkg, plus putting
the new constant to use in the (then-upstream) PvScsi feature. No matter
how long that is delayed, the main feature will have been merged.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56511): https://edk2.groups.io/g/devel/message/56511
Mute This Topic: https://groups.io/mt/72544125/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
