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.52.1585695246590112283 for ; Tue, 31 Mar 2020 15:54:06 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=vRVvoBvf; 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 02VMrPnT076473; Tue, 31 Mar 2020 22:54:05 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : cc : subject : date : message-id : mime-version : content-transfer-encoding; s=corp-2020-01-29; bh=jeW9hra7U4wlEIc0ifYIQvkuhRdpdhdUMRx1WDVbjQc=; b=vRVvoBvfFZ/sh1jD7QoZOS444Yrj/fsZC7vw4SjwE3Dinu85IeE1GxxAW0xWvJ1/SEP2 FZFoZkpuffnZFdQQl4Kxk0Mq2mpfLjSAAExz9QTa6PbZzLcLed5RAmJWvuj6uxf/JVyh s+Z2YpxmwNKtoyR+0sztYatfq9w+Hk/TcM377+nf4bFsTvoRw7YSjLvHUisNTIC5Y1S6 7FNapEgdxyg3CPcybACV3qxIIErdm+92bC+lzCnL+Fy9CCPcC5dQ3wMLfOiVZPcwX9va 74+SsWQM63AZtj94jUfoBROhHn1rrWwuFD5h6mzFQtu+IRyVZtGe1Tet/UZIBlyAv4Ki Dw== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by aserp2120.oracle.com with ESMTP id 303yun4xcd-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:54:05 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02VMkqmO034505; Tue, 31 Mar 2020 22:54:04 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3030.oracle.com with ESMTP id 302g2f3c3f-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 22:54:04 +0000 Received: from abhmp0016.oracle.com (abhmp0016.oracle.com [141.146.116.22]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 02VMs3Eq008902; Tue, 31 Mar 2020 22:54:03 GMT Received: from spark.ravello.local (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 31 Mar 2020 15:54:02 -0700 From: "Liran Alon" To: devel@edk2.groups.io, lersek@redhat.com Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org, Liran Alon Subject: [PATCH v2] OvmfPkg/PvScsiDxe: Refactor setup of rings to separate function Date: Wed, 1 Apr 2020 01:56:37 +0300 Message-Id: <20200331225637.123318-1-liran.alon@oracle.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9577 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 spamscore=0 mlxscore=0 adultscore=0 phishscore=0 bulkscore=0 suspectscore=2 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003310185 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-2003310186 Content-Transfer-Encoding: 8bit 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 | 163 +++++++++++++++++++------------------ 1 file changed, 85 insertions(+), 78 deletions(-) diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c index 1ca50390c0e5..8c458ecceeb0 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 @@ -1171,6 +1168,14 @@ PvScsiInit ( goto FreeRings; } + // + // Setup rings against device + // + Status = PvScsiSetupRings (Dev); + if (EFI_ERROR (Status)) { + goto FreeDMACommBuffer; + } + // // Populate the exported interface's attributes // @@ -1202,13 +1207,15 @@ PvScsiInit ( return EFI_SUCCESS; +FreeDMACommBuffer: + PvScsiFreeSharedPages ( + Dev, + EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)), + Dev->DmaBuf, + &Dev->DmaBufDmaDesc + ); + FreeRings: - // - // Reset device to stop device usage of the rings. - // This is required to safely free the rings. - // - PvScsiResetAdapter (Dev); - PvScsiFreeRings (Dev); RestorePciAttributes: -- 2.20.1