From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2120.oracle.com (userp2120.oracle.com [156.151.31.85]) by mx.groups.io with SMTP id smtpd.web12.557.1585589226632848312 for ; Mon, 30 Mar 2020 10:27:06 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=yJp7Wgfd; spf=pass (domain: oracle.com, ip: 156.151.31.85, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02UHAfOZ037859; Mon, 30 Mar 2020 17:27:06 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=48okPpS2Kdd+aTJn1aiW7ua2H8cUMcp1jFapoFh5PXA=; b=yJp7Wgfd+06nruUCn2z3LaTA4qSjaVCTmABRCZalPptbrZo0dCI/N5+QncMZdY6KzhYP hfG2M3emYj8dUWFZbsdClF+pmrQx1uNUeGwY0b7nuxiqlYRgqGs9aVC4hAaXr7DDHNkp SPgvqLSClxZKMlpfDLDxXG/iafrwLxO/uOQyv6YumEe1BAgu1S8+haIZ5NR96Pm5SU4M wKpjt2PI0zv0s1ICbv97gKpfO4FI/dadCq8XEkfgBHGnHuruTDtzxmlvyLAVCMLxi8s2 JlG4RA38E6xaacdIaH/aLIdP/785b1EVEZUxP59qaNnelK8ka17a+N+oTF4kI0PX2IwT 0w== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2120.oracle.com with ESMTP id 303aqhbjkc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 30 Mar 2020 17:27:05 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02UHL728190805; Mon, 30 Mar 2020 17:25:05 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserp3020.oracle.com with ESMTP id 302gc9wdmw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 30 Mar 2020 17:25:05 +0000 Received: from abhmp0003.oracle.com (abhmp0003.oracle.com [141.146.116.9]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 02UHP3pi030817; Mon, 30 Mar 2020 17:25:03 GMT Received: from [192.168.14.112] (/79.180.216.197) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 30 Mar 2020 10:25:03 -0700 Subject: Re: [edk2-devel] [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings To: Laszlo Ersek , 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> From: "Liran Alon" Message-ID: <82d2a754-21cd-9032-b063-7a056775592d@oracle.com> Date: Mon, 30 Mar 2020 20:24:59 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9576 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 adultscore=0 suspectscore=0 phishscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003300153 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9576 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0 clxscore=1015 malwarescore=0 impostorscore=0 mlxlogscore=999 spamscore=0 mlxscore=0 priorityscore=1501 lowpriorityscore=0 adultscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003300153 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US On 30/03/2020 18:54, Laszlo Ersek wrote: > On 03/28/20 21:00, Liran Alon wrote: >> STATIC >> EFI_STATUS >> PvScsiInit ( >> @@ -466,6 +669,14 @@ PvScsiInit ( >> goto RestorePciAttributes; >> } >> >> + // >> + // Init PVSCSI rings >> + // >> + Status = PvScsiInitRings (Dev); >> + if (EFI_ERROR (Status)) { >> + goto RestorePciAttributes; >> + } >> + >> // >> // Populate the exported interface's attributes >> // >> @@ -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: > > https://urldefense.com/v3/__http://mid.mail-archive.com/45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-Fn-EyPU$ > https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56425__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-4bfLR8U$ > > (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. > > (Also, you didn't set up the "diff.orderFile" knob the way I requested > in point (8): I actually did set it up in my ~/.gitconfig. It seems that it doesn't work from some reason... > > https://urldefense.com/v3/__http://mid.mail-archive.com/0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-bs34O4s$ > https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56420__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-msgEmGo$ > > and so the header file changes are again at the end of the patch... > Another thing I'll have to live with, I guess.) > > Laszlo Thanks for the understanding, -Liran