From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.484.1587383944713118127 for ; Mon, 20 Apr 2020 04:59:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ib8Y8jck; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587383943; 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=rG4Jygjtytks8pFW/85s+pajl16l2lvexI2ZXbaD/d0=; b=ib8Y8jckMr6OlYc/fC9v3p/+SAClhjqb6/VTp0hqkc03l4//6nD5rIGSRjrFpq6YkOpo6l 7elMliqfdxaW2Cdi4pEEK8xbaoLe3jMOFGnPsg9jS5snv/ZOFIcB+6eZo+sy+FBwwoflTF tiJ//8ydPIg0m4k97rRWdy2CQx1q8CE= 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-244-djoAGgCsMDW9ybmGXWupJg-1; Mon, 20 Apr 2020 07:59:00 -0400 X-MC-Unique: djoAGgCsMDW9ybmGXWupJg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 46701DB61; Mon, 20 Apr 2020 11:58:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-228.ams2.redhat.com [10.36.114.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id ED79B99DEF; Mon, 20 Apr 2020 11:58:56 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 10/13] OvmfPkg/MptScsiDxe: Initialize hardware To: Nikita Leshenko Cc: devel@edk2.groups.io, liran.alon@oracle.com, aaron.young@oracle.com, Jordan Justen , Ard Biesheuvel References: <20200414173813.7715-1-nikita.leshchenko@oracle.com> <20200414173813.7715-11-nikita.leshchenko@oracle.com> <034ffcb9-e11b-4b31-8d62-2eba4eaef08f@redhat.com> From: "Laszlo Ersek" Message-ID: <978d788b-c4c3-9402-eca4-44abe622fd00@redhat.com> Date: Mon, 20 Apr 2020 13:58:56 +0200 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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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 04/16/20 18:00, Nikita Leshenko wrote: > > >> On 16 Apr 2020, at 12:53, Laszlo Ersek wrote: >> >> On 04/14/20 19:38, Nikita Leshenko wrote: [...] > I break out the inner structs into separate typedefs, wrap all of them in > #pragma pack (1) / pack () and then create the wrapping aligning unions? >> (7a) either send "Req.Data" (rather than "Req") to the device in this >> function, > I wasn't aware of this padding, so I will update the code to use suggestion (7a). OK, thanks! >> (11) Please repeat the statement from the commit message here -- as a >> code comment -- that the reply is only read to complete the doorbell >> function of the device, and that we intentionally ignore the contents. >> >> (BTW, if we do not parse the response even at the end of this series, >> then saving the response into the "Reply" variable looks useless -- I >> guess we could remove the "Reply" variable altogether, in that case. But >> I'll have to see the rest of the patches.) > We must read the reply fully to initialise the device (the device would reset the > interrupt status only after the host has fetched the entire reply). We could discard > the reply, but I thought it would be nicer to keep it on the stack temporary in > case someone would like to use it in later changes or in debugging (since the > reply is very small and should be hot in the cache, I don't this has any > performance impact). Makes sense. The comment will help. Thanks! >> (12) So in general I would have suggested that, in case the >> initialization fails and we take an early "return" branch in this >> function, we should first jump to an error handling label, and reset the >> device. (Because, when we return an error code from the function, we >> shouldn't leave the device in half-initialized state.) >> >> *But*, it seems like the reset operation *itself* occurs through the >> doorbell register and the MPT_REG_ISTATUS register -- and if any step in >> MptScsiInit() fails, then it's exactly those registers that are >> (apparently) in unknown / unreliable state. In other words, if >> MptScsiInit() fails, then we *can't* (expect to) reset the device >> (successfully). >> >> Can you please confirm my understanding? (Again, just a question, not a >> code change request.) > Your understanding it correct. In VirtualBox it looks like you can't write a > reset command to the doorbell while a handshake is in progress. In any case, wonder > what could realistically cause the initialization to fail. On EDK2 side, we're just > talking about a bunch of "in/out" opcodes (the API for which could fail, but I would > assume that if it fails then further "in/out"s will fail as well). On the hypervisor > side, the QEMU and VirtualBox don't even have a failure path for the init call. > So failure seems very theoretical here and I couldn't find anything realistically > helpful to do if the initialization fails in the middle. Thank you for explaining. I'm proceeding with the v4 review. Laszlo