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.web10.19.1585601209563851698 for ; Mon, 30 Mar 2020 13:46:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=OYqZrYmL; 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=1585601208; 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=zhYzZvNLiXmvurLxsABc6KvCAZeEQQet7aTqs1WCjP0=; b=OYqZrYmLSk/e9RfHzyCspI2m08g+/+NCJuqQxNLlNbg+OKAPl3rO4sRknY0pXQlWWEI+RP +US41ltvkZf8ax/Az+L++nxvK+++s3nSZD/Ebhct3VclMe474KPjFD1846CAC8KDZ8Hf+v 8zIeP4JOr1RX7y+wiPXPlt5BoCXXrII= 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-448-DYxWsn3YMayp_0IgX1C6Tw-1; Mon, 30 Mar 2020 16:46:44 -0400 X-MC-Unique: DYxWsn3YMayp_0IgX1C6Tw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 99A49800D50; Mon, 30 Mar 2020 20:46:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-191.ams2.redhat.com [10.36.112.191]) by smtp.corp.redhat.com (Postfix) with ESMTP id F41C019C58; Mon, 30 Mar 2020 20:46:41 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings 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: <20200328200100.60786-1-liran.alon@oracle.com> <20200328200100.60786-14-liran.alon@oracle.com> <82d2a754-21cd-9032-b063-7a056775592d@oracle.com> From: "Laszlo Ersek" Message-ID: <8c97d491-7e0d-986b-d3af-e7b065c5c4ba@redhat.com> Date: Mon, 30 Mar 2020 22:46:41 +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: <82d2a754-21cd-9032-b063-7a056775592d@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/30/20 19:24, Liran Alon wrote: > > On 30/03/2020 18:54, Laszlo Ersek wrote: >> On 03/28/20 21:00, Liran Alon wrote: >>> @@ -509,6 +720,14 @@ PvScsiUninit ( >>> IN OUT PVSCSI_DEV *Dev >>> ) >>> { >>> + // >>> + // Reset device to stop device usage of the rings. >>> + // This is required to safely free the rings. >>> + // >>> + PvScsiResetAdapter (Dev); >> If I understand correctly (at the end of the series): >> >> in order to save one of the (ultimately) two reset calls in >> PvScsiUninit(), namely >> - one for releasing the DMA buf >> - and the other (internal to PvScsiFreeRings()) for releasing the >> rings, you unnested the latter reset call from PvScsiFreeRings(), and >> added it manually to the error path of PvScsiInit(). > Right. >> To me, that's more brittle and more difficult to reason about than >> what I proposed, because now PvScsiFreeRings() does not completely >> undo PvScsiInitRings(). > Hmm right. Because PvScsiInitRings() also setup the ring to the device > with a command. Haven't thought about it. >> I specifically requested that we please not try to save on the two >> (seemingly) redundant reset calls in PvScsiUninit() -- see the >> paragraph containing "bad idea" here: >> >> [...] >> >> (Note: we could be tempted to somehow "centralize" all of these >> Reset operations into a single spot. Bad idea. We are revoking >> the device's access rights to different resources, so the >> revocation operations will show up in different spots. It's a >> mere circumstance that the revocations all happen to be Reset >> operations.) >> >> I might be paranoid of course -- I just feel that >> maybe-superfluous reset operations on error paths are much >> better than silently corrupted guest memory and/or disk >> contents. >> >> You reacted to that message of mine, but not this paragraph >> specifically (it was snipped from your followup) -- so I thought you >> were OK with it. I'm a bit disappointed that you disagreed with my >> request *silently*. > Oh now I got what you meant! I misinterpreted it. Not silently ignored > it of course! > I can change this in a v4 patch-series if you would like that. Sorry > for the misunderstanding. > > I do agree with your statement that PvScsiFreeRings() is not > completely an undo of PvScsiInitRings(). > And maybe for making code a little bit easier to reason about, it's > preferred to just do one additional unnecessary device reset. >> To summarize: technically, your solution is correct; from a code >> hygiene and resource ownership question, I'm convinced it is wrong. > I agree. >> But, I'll live with it. >> Reviewed-by: Laszlo Ersek > I can send a v4 patch-series just changing that if you would like. > Or submit a patch to change it after it will be merged. > Or that you will change it when applying. Or leave it as is. > > I don't have any objection to any of the above. OK. That's a relief to me, thank you! And I apologize for insinuating that you deliberately & silently ignored said feedback from me. I tend to be pedantic about email (it's not natural for me to think that someone simply missed a comment) -- sorry about that! Given that I've already merged v3 -- can you please post a followup patch? Something like: > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index 0a66c98421a9..3066b83b96fc 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -1093,6 +1093,12 @@ PvScsiFreeRings ( > IN OUT PVSCSI_DEV *Dev > ) > { > + // > + // Reset device to stop device usage of the rings. > + // This is required to safely free the rings. > + // > + PvScsiResetAdapter (Dev); > + > PvScsiFreeSharedPages ( > Dev, > 1, > @@ -1199,12 +1205,6 @@ PvScsiInit ( > return EFI_SUCCESS; > > FreeRings: > - // > - // Reset device to stop device usage of the rings. > - // This is required to safely free the rings. > - // > - PvScsiResetAdapter (Dev); > - > PvScsiFreeRings (Dev); > > RestorePciAttributes: > @@ -1220,12 +1220,8 @@ PvScsiUninit ( > ) > { > // > - // Reset device to: > - // - Make device stop processing all requests. > - // - Stop device usage of the rings. > - // > - // This is required to safely free the DMA communication buffer > - // and the rings. > + // Reset device to make device stop processing all requests. > + // This is required to safely free the DMA communication buffer. > // > PvScsiResetAdapter (Dev); Thank you! Laszlo