From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web12.11591.1585316129748382989 for ; Fri, 27 Mar 2020 06:35:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dYMdLHnH; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585316128; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/3BPo8eG/VBn8xNkmtykj9XVvy7BlvziXp9DVk5mfEI=; b=dYMdLHnH58TdYFKtGZzCNSZ2So7iadlQ2px9mjqZdAyY8DoLbgD49MV8xVh1QlDzmtQLOg jUbr9qF5maxXwWeQOFHt4KN3yRMOHlktR8HQ7Y3CKaknL6d6EhJZrpww5pv6pXLtoHOipF 9kKIQeQQS7jKZtllNjmmaLiZd3UZsGw= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-116-5ALi1n7aOQWwlDxS-Jfrjw-1; Fri, 27 Mar 2020 09:35:27 -0400 X-MC-Unique: 5ALi1n7aOQWwlDxS-Jfrjw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AF22F8010F0; Fri, 27 Mar 2020 13:35:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-36.ams2.redhat.com [10.36.114.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id E44631001B07; Fri, 27 Mar 2020 13:35:23 +0000 (UTC) Subject: Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer To: Liran Alon , devel@edk2.groups.io Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200325161005.16743-1-liran.alon@oracle.com> <20200325161005.16743-15-liran.alon@oracle.com> <45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com> <1d747d1e-064e-f24a-f8ac-ac07b91c3999@oracle.com> From: "Laszlo Ersek" Message-ID: <2220b9a0-e550-6d9c-c102-f3f2f10326d3@redhat.com> Date: Fri, 27 Mar 2020 14:35:22 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1d747d1e-064e-f24a-f8ac-ac07b91c3999@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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