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

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

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

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

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.

>> +  UINT8     Data[0x2000];
> (5) Same here. From peeking at the next patch, we seem to be choosing
> this size arbitrarily.
This is indeed arbitrary...
>
> If it works for you in all relevant boot scenarios, I'm OK with it, but
> we should be clear that this value is arbitrarily chosen. No need for a
> #define, but a comment would be nice.

OK. Will just add a comment such as:

//
// This size is arbitrarily chosen,
// It seems to be sufficient for all I/O requests sent through 
EFI_SCSI_PASS_THRU_PROTOCOL.PassThru().
//

If you wish to have something else, please say so. :)

>
> (6) Should we declare this structure as packed?
There is no such requirement as it's not a wire-format or anything like 
that.
The only rational of defining it as packed is to avoid mapping 
unnecessary pages to device.
But this structure is less than a page-size anyway. So I think it's fine.

If you still think it should be marked as packed, I can change this.

Thanks!
-Liran