From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web11.50.1585693363863960680 for ; Tue, 31 Mar 2020 15:22:44 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=Vks08Orb; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02VM9Www182664; Tue, 31 Mar 2020 22:22:37 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=SPbEtQG6G3eox66IbQ3Mjn+8S7XuX6tNsEu4fkaO5fU=; b=Vks08OrbK2MzkPhpGOMheASBlXNofdng7CDzJlkvJ5vjFkhzfJrojGNVLFHDiY7hYcd1 ZyO6JESCs5Refmt1jgFKBMHfq7CofpjV7regXYR0SOiUupQXOmW6Z+qEpiWmfs7jXvIE OTfq+zjmfaWALkZRGbRMSpmcqIgcFyNhq6U1LBHb+l1fk3SWr6OwTunzl+HdjAdqm1+P hJVwDvLtEs1gSY7g7aCuxxLIKPQMvt1dfDSRfO1b8ZeDhLi90BvUKNea/5vKa+SJ7CwZ DYeFEEuxed135vOsCrWz6F86R9CfkPK6w7ykWjqUebraibBG0SKwXsF9PpEyim/nXCcc /w== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by aserp2120.oracle.com with ESMTP id 303yun4uj9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:22:37 +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 02VM774g108208; Tue, 31 Mar 2020 22:22:36 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by aserp3020.oracle.com with ESMTP id 302gcdvqvh-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:22:36 +0000 Received: from abhmp0011.oracle.com (abhmp0011.oracle.com [141.146.116.17]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 02VMMacc002539; Tue, 31 Mar 2020 22:22:36 GMT Received: from [192.168.14.112] (/79.180.216.197) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 31 Mar 2020 15:22:36 -0700 Subject: Re: [PATCH] OvmfPkg/PvScsiDxe: Refactor setup of rings to separate function 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: <20200331114730.62947-1-liran.alon@oracle.com> From: "Liran Alon" Message-ID: Date: Wed, 1 Apr 2020 01:22:33 +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=9577 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 adultscore=0 suspectscore=2 phishscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003310180 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9577 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 lowpriorityscore=0 malwarescore=0 adultscore=0 priorityscore=1501 mlxlogscore=999 bulkscore=0 suspectscore=2 mlxscore=0 spamscore=0 impostorscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003310180 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US On 01/04/2020 1:19, Laszlo Ersek wrote: > Hi Liran, > > On 03/31/20 13:47, Liran Alon wrote: >> Previous to this change, PvScsiFreeRings() was not undoing all >> operations that was done by PvScsiInitRings(). >> This is because PvScsiInitRings() was both preparing rings (Allocate >> memory and map it for device DMA) and setup the rings against device by >> issueing a device command. While PvScsiFreeRings() only unmaps the rings >> and free their memory. >> >> Driver do not have a functional error as it makes sure to reset device >> before every call site to PvScsiFreeRings(). However, this is not >> intuitive. >> >> Therefore, prefer to refactor the setup of the ring against device to a >> separate function than PvScsiInitRings(). >> >> Signed-off-by: Liran Alon >> --- >> OvmfPkg/PvScsiDxe/PvScsi.c | 150 +++++++++++++++++++------------------ >> 1 file changed, 76 insertions(+), 74 deletions(-) > Thanks for the patch. > > I didn't expect this approach -- hoisting the rings exposure from > SetupRings to Init --, but it turns out that I like it more than my own > previous suggestion -- pushing ResetAdapter into FreeRings :) Yeah I thought about it later and liked it to. Hoped you will think the same ;) > > May I just trouble you with one further request? > >> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c >> index 1ca50390c0e5..5b7fdcbda10b 100644 >> --- a/OvmfPkg/PvScsiDxe/PvScsi.c >> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c >> @@ -991,13 +991,6 @@ PvScsiInitRings ( >> ) >> { >> EFI_STATUS Status; >> - union { >> - PVSCSI_CMD_DESC_SETUP_RINGS Cmd; >> - UINT32 Uint32; >> - } AlignedCmd; >> - PVSCSI_CMD_DESC_SETUP_RINGS *Cmd; >> - >> - Cmd = &AlignedCmd.Cmd; >> >> Status = PvScsiAllocateSharedPages ( >> Dev, >> @@ -1032,6 +1025,69 @@ PvScsiInitRings ( >> } >> ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE); >> >> + return EFI_SUCCESS; >> + >> +FreeRingReqs: >> + PvScsiFreeSharedPages ( >> + Dev, >> + 1, >> + Dev->RingDesc.RingReqs, >> + &Dev->RingDesc.RingReqsDmaDesc >> + ); >> + >> +FreeRingState: >> + PvScsiFreeSharedPages ( >> + Dev, >> + 1, >> + Dev->RingDesc.RingState, >> + &Dev->RingDesc.RingStateDmaDesc >> + ); >> + >> + return Status; >> +} >> + >> +STATIC >> +VOID >> +PvScsiFreeRings ( >> + IN OUT PVSCSI_DEV *Dev >> + ) >> +{ >> + PvScsiFreeSharedPages ( >> + Dev, >> + 1, >> + Dev->RingDesc.RingCmps, >> + &Dev->RingDesc.RingCmpsDmaDesc >> + ); >> + >> + PvScsiFreeSharedPages ( >> + Dev, >> + 1, >> + Dev->RingDesc.RingReqs, >> + &Dev->RingDesc.RingReqsDmaDesc >> + ); >> + >> + PvScsiFreeSharedPages ( >> + Dev, >> + 1, >> + Dev->RingDesc.RingState, >> + &Dev->RingDesc.RingStateDmaDesc >> + ); >> +} >> + >> +STATIC >> +EFI_STATUS >> +PvScsiSetupRings ( >> + IN OUT PVSCSI_DEV *Dev >> + ) >> +{ >> + union { >> + PVSCSI_CMD_DESC_SETUP_RINGS Cmd; >> + UINT32 Uint32; >> + } AlignedCmd; >> + PVSCSI_CMD_DESC_SETUP_RINGS *Cmd; >> + >> + Cmd = &AlignedCmd.Cmd; >> + >> ZeroMem (Cmd, sizeof (*Cmd)); >> Cmd->ReqRingNumPages = 1; >> Cmd->CmpRingNumPages = 1; >> @@ -1052,71 +1108,12 @@ PvScsiInitRings ( >> sizeof (*Cmd) % sizeof (UINT32) == 0, >> "Cmd must be multiple of 32-bit words" >> ); >> - Status = PvScsiWriteCmdDesc ( >> - Dev, >> - PvScsiCmdSetupRings, >> - (UINT32 *)Cmd, >> - sizeof (*Cmd) / sizeof (UINT32) >> - ); >> - if (EFI_ERROR (Status)) { >> - goto FreeRingCmps; >> - } >> - >> - return EFI_SUCCESS; >> - >> -FreeRingCmps: >> - PvScsiFreeSharedPages ( >> - Dev, >> - 1, >> - Dev->RingDesc.RingCmps, >> - &Dev->RingDesc.RingCmpsDmaDesc >> - ); >> - >> -FreeRingReqs: >> - PvScsiFreeSharedPages ( >> - Dev, >> - 1, >> - Dev->RingDesc.RingReqs, >> - &Dev->RingDesc.RingReqsDmaDesc >> - ); >> - >> -FreeRingState: >> - PvScsiFreeSharedPages ( >> - Dev, >> - 1, >> - Dev->RingDesc.RingState, >> - &Dev->RingDesc.RingStateDmaDesc >> - ); >> - >> - return Status; >> -} >> - >> -STATIC >> -VOID >> -PvScsiFreeRings ( >> - IN OUT PVSCSI_DEV *Dev >> - ) >> -{ >> - PvScsiFreeSharedPages ( >> - Dev, >> - 1, >> - Dev->RingDesc.RingCmps, >> - &Dev->RingDesc.RingCmpsDmaDesc >> - ); >> - >> - PvScsiFreeSharedPages ( >> - Dev, >> - 1, >> - Dev->RingDesc.RingReqs, >> - &Dev->RingDesc.RingReqsDmaDesc >> - ); >> - >> - PvScsiFreeSharedPages ( >> - Dev, >> - 1, >> - Dev->RingDesc.RingState, >> - &Dev->RingDesc.RingStateDmaDesc >> - ); >> + return PvScsiWriteCmdDesc ( >> + Dev, >> + PvScsiCmdSetupRings, >> + (UINT32 *)Cmd, >> + sizeof (*Cmd) / sizeof (UINT32) >> + ); >> } >> >> STATIC >> @@ -1157,6 +1154,10 @@ PvScsiInit ( >> if (EFI_ERROR (Status)) { >> goto RestorePciAttributes; >> } >> + Status = PvScsiSetupRings (Dev); >> + if (EFI_ERROR (Status)) { >> + goto FreeRings; >> + } > If you could move the PvScsiSetupRings() call *below* the DMA buffer > allocation (and rework the error path accordingly -- basically replacing > the "UnsetupRings" action with a "FreeDmaBuffer" action), then this > patch would be *perfect*. > > Because then, the PvScsiUninit() order of actions would be a 100% mirror > for PvScsiInit()'s; namely with PvScsiResetAdapter() at the top of > PvScsiUninit() perfectly matching PvScsiSetupRings() at the end of > PvScsiInit()! > > I don't want to annoy you too much, so if you really want to stick with > this version, I'll take it as well. Good suggestion. Let me submit a v2 of this patch. -Liran > > Thanks! > Laszlo > >> >> // >> // Allocate DMA communication buffer >> @@ -1168,7 +1169,7 @@ PvScsiInit ( >> &Dev->DmaBufDmaDesc >> ); >> if (EFI_ERROR (Status)) { >> - goto FreeRings; >> + goto UnsetupRings; >> } >> >> // >> @@ -1202,13 +1203,14 @@ PvScsiInit ( >> >> return EFI_SUCCESS; >> >> -FreeRings: >> +UnsetupRings: >> // >> // Reset device to stop device usage of the rings. >> // This is required to safely free the rings. >> // >> PvScsiResetAdapter (Dev); >> >> +FreeRings: >> PvScsiFreeRings (Dev); >> >> RestorePciAttributes: >>