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

["Laszlo Ersek" <lersek@redhat.com>]

On 03/27/20 22:31, Liran Alon wrote:
> 
> 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 :)

A new PvScsi.h-internal macro, with replacement text "252", sounds nice.

But in that case, you'll have to check (or trim) the
"EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.SenseDataLength" field value
on input.

I'm fine either way -- my main point is that we shouldn't report more
sense room to the device than what we have available in the DMA buffer.
If the caller passes in a SenseDataLength that's larger than 252 (such
as 254, for example), and our DMA buffer field is 252 bytes large, then
we should give 252 to the device, and report back to the caller
accordingly (= no more than 252 bytes).

Thanks,
Laszlo

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