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

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

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.

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

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.

>
>>> +  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. :)

This comment looks nice, I'd only append: "for common boot scenarios".

>
>>
>> (6) Should we declare this structure as packed?
> There is no such requirement as it's not a wire-format or anything
> like that.

My thinking was that we shouldn't have any "padding" in a structure
that's exposed to a device.

But, admittedly, due to rounding up the allocation to page size anyway,
we're virtually guaranteed to have unused space at the end. Possibly
having some padding in the middle makes no difference, so you are right:
no need for packing this.

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

I agree with your general point, but I'd like to comment on the size
independently (just for the record): the PVSCSI_DMA_BUFFER structure is
not smaller than a page. It is slightly larger than two pages -- it
contains at least (255 + 2 * 4096) bytes. Again, that doesn't change the
main point.

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

No, I agree it need not be packed.

Thanks!
Laszlo