From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.15579.1592220453111429448 for ; Mon, 15 Jun 2020 04:27:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=gJ+322d3; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592220452; 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=mQFshx9KFKoBB5Kn9cNtunXwmdl6BI2wSoYcGWHaR2s=; b=gJ+322d38tlM107fTj2i7ugD55l+6eJyoa0iDnEvUYfnyqhkkM6uY7oZcEC3njhSZBfYL6 FWffFx8oQI8fBKlqO0T8uUW52QP4kfEeqWgBe8IlYtIf/0zDpwMiz8aZM/fDgDHtMbgFEE 3l4TtClwQs54HwfldVUrgu3Yp00Pts0= 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-169-64D57H29Mju2XHILpS7mXg-1; Mon, 15 Jun 2020 07:27:26 -0400 X-MC-Unique: 64D57H29Mju2XHILpS7mXg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E6CC2BFC6; Mon, 15 Jun 2020 11:27:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-132.ams2.redhat.com [10.36.112.132]) by smtp.corp.redhat.com (Postfix) with ESMTP id CE1007F4F9; Mon, 15 Jun 2020 11:27:23 +0000 (UTC) Subject: Re: [RFC PATCH 1/1] OvmfPkg: Introduce LSI 53C895A SCSI Controller Driver To: Gary Lin Cc: devel@edk2.groups.io, Jordan Justen , Ard Biesheuvel References: <20200612100451.12832-1-glin@suse.com> From: "Laszlo Ersek" Message-ID: <06577314-ae06-ed01-8c59-f533466efecc@redhat.com> Date: Mon, 15 Jun 2020 13:27:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200612100451.12832-1-glin@suse.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 Hello Gary, On 06/12/20 12:04, Gary Lin wrote: > This commit introduces the driver for LSI 53C895A SCSI Controller > which, so that OVMF can access the devices attached to the emulated > "lsi" SCSI controller. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Gary Lin > --- > Although lsi is now considered obsolete in QEMU, I wrote this driver > mainly for learning the UEFI SCSI driver and ... FUN! The majority of > the code is actually borrowed from VirtioScsci and MptScsi, and I > mainly focus on LsiScsiPassThru(). > > If it's fine to add this driver into OvmfPkg, I'll start to split this > patch into smaller pieces to make it easier to review. (1) Do we have an official deprecation notice for this SCSI controller in QEMU? If we do, then (AIUI) the controller will be removed in QEMU in one or two releases, so this code would become effectively dead in the mid term. I wouldn't like to review and/or carry code that's soon to be dead. (2) If there is no official deprecation notice in QEMU, then I agree it makes sense to include this driver. In that case, I have another question: Do you intend this driver for production purposes? I.e., do you expect users or "layered products" (libvirt, proxmox, openstack, ...) to use this SCSI controller for some well-defined purpose? (The MPT SCSI and PV SCSI drivers had a clear product-oriented modivation: https://edk2.groups.io/g/devel/message/55620 http://mid.mail-archive.com/a96b6b74-c35d-e291-2122-9d77f1d5f89c@oracle.com ) (2a) If this driver is not meant for a production environment, then LSI_SCSI_ENABLE should be FALSE by default (and I'll do a lighter review). (2b) If the driver is meant for production, then LSI_SCSI_ENABLE should indeed be TRUE, and I'll have to be more diligent in reviewing this. For either (2a) or (2b), please do split up the driver into smaller patches, and please also add yourself to Maintainers.txt as the designated reviewer of the new driver. Thanks Laszlo