Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

["Liran Alon" <liran.alon@oracle.com>]

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.
>