From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by mx.groups.io with SMTP id smtpd.web10.9964.1585065625258307274 for ; Tue, 24 Mar 2020 09:00:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=IdLVfV5a; spf=pass (domain: redhat.com, ip: 216.205.24.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585065624; 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=VI0I5Qvh6en1EEnxfC936cPNdm0/xSrHJTNudck6wYo=; b=IdLVfV5aE5AaOTH5H78WW7Vp2Hacr39G8ILk6qF1Aa3rc+yGAYx832KWZlVwVj5xCYMs7n Df9PsdE0Z1xeAQH2eNt1Y8l2U9oigexn96ksq4/2Mr62axCLpvkh6KLSXzizr5tBlVmwn8 KNC+4bEpMjxXN88LzajvRLxsrNne7W8= 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-409-ARk8uBZaO5GyWH-Uo5CKnQ-1; Tue, 24 Mar 2020 12:00:19 -0400 X-MC-Unique: ARk8uBZaO5GyWH-Uo5CKnQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 22DFEDB62; Tue, 24 Mar 2020 16:00:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-139.ams2.redhat.com [10.36.115.139]) by smtp.corp.redhat.com (Postfix) with ESMTP id E929BCDBC5; Tue, 24 Mar 2020 16:00:12 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init To: devel@edk2.groups.io, liran.alon@oracle.com Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200316150113.104630-1-liran.alon@oracle.com> <20200316150113.104630-13-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 24 Mar 2020 17:00:10 +0100 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: <20200316150113.104630-13-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 a bit more superficial comments, for now: On 03/16/20 16:01, Liran Alon wrote: > The following commits will complete the implementation of > device initialization. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567 > Reviewed-by: Nikita Leshenko > Signed-off-by: Liran Alon > --- > OvmfPkg/PvScsiDxe/PvScsi.c | 77 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 77 insertions(+) > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index ff6b50b7020f..fb2407d2adb2 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -29,6 +29,76 @@ > // Ext SCSI Pass Thru utilities > // > > + > +// > +// Writes a 32-bit value into BAR0 using MMIO > +// (1) Please use the /** **/ comment style for top-level function comment blocks. > +STATIC > +EFI_STATUS > +PvScsiMmioWrite32 ( > + IN CONST PVSCSI_DEV *Dev, > + IN UINT64 Offset, > + IN UINT32 Value > + ) > +{ > + return Dev->PciIo->Mem.Write( > + Dev->PciIo, > + EfiPciIoWidthUint32, > + 0, // BarIndex (2) Style improvement: please use the PCI_BAR_IDX0 macro. > + Offset, > + 1, // Count > + &Value > + ); > +} > + > +// > +// Send PVSCSI command to device > +// (3) Same as (1). > +STATIC > +EFI_STATUS > +PvScsiWriteCmdDesc ( > + IN CONST PVSCSI_DEV *Dev, > + IN UINT32 Cmd, > + IN VOID *Desc, > + IN UINTN Length > + ) > +{ > + EFI_STATUS Status; > + UINTN LengthInWords; > + UINT8 *WordPtr; > + UINT8 *DescEndPtr; > + UINT32 Word; > + > + LengthInWords = Length / sizeof (UINT32); (4) What guarantees that "Length" is a whole multiple of sizeof (UINT32)? In this review I have not insisted on including full-blown interface contracts in the top-level function comment blocks (with @param[in] and @retval etc). But, for this function, it really is unclear. > + > + if (LengthInWords > PVSCSI_MAX_CMD_DATA_WORDS) { > + return EFI_INVALID_PARAMETER; > + } > + > + Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND, Cmd); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + WordPtr = Desc; > + DescEndPtr = WordPtr + Length; > + > + while (WordPtr != DescEndPtr) { > + // > + // CopyMem() is used to avoid strict-aliasing issues > + // (5) In edk2, we -- completely intentionally -- disable the enforcement of the effective type rules / strict aliasing rules. See "-fno-strict-aliasing" in "BaseTools/Conf/tools_def.template". > + CopyMem (&Word, WordPtr, sizeof (UINT32)); > + > + Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND_DATA, Word); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + WordPtr += sizeof (UINT32); > + } > + > + return EFI_SUCCESS; > +} (6) I think the open-coded loop is suboptimal -- the PciIo protocol seems to offer EfiPciIoWidthFifoUint32 for exactly this purpose (= advance in the memory buffer, while accessing the same offset in the BAR). Have you perhaps tried that? (I can imagine that you ruled it out, due to "Desc" being unaligned. The UEFI spec does say, "The caller is responsible for any alignment and I/O width issues which the bus, device, platform, or type of I/O might require.) > // > // Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and > // EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized > @@ -348,6 +418,13 @@ PvScsiInit ( > return Status; > } > > + // > + // Reset adapter > + // > + Status = PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0); > + if (EFI_ERROR (Status)) { > + return Status; > + } > // > // Populate the exported interface's attributes > // > OK, so this ties back to my comments on resource management, under patch#9. Let me quote the PvScsiInit() function in full, after the present patch is applied: > STATIC > EFI_STATUS > PvScsiInit ( > IN OUT PVSCSI_DEV *Dev > ) > { > EFI_STATUS Status; > > // > // Init configuration > // > Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit); > Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit); > > // > // Set PCI Attributes > // > Status = PvScsiSetPCIAttributes (Dev); > if (EFI_ERROR (Status)) { > return Status; > } > > // > // Reset adapter > // > Status = PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0); > if (EFI_ERROR (Status)) { > return Status; > } (7) So this is precisely the spot where we have to jump to a new error handling label -- called "RestorePciAttributes" --, to be introduced at the end of the function. And we need to call PvScsiRestorePciAttributes() there. Otherwise, the original PCI attributes will never be restored. Thanks! Laszlo On 03/16/20 16:01, Liran Alon wrote: > // > // Populate the exported interface's attributes > // > Dev->PassThru.Mode = &Dev->PassThruMode; > Dev->PassThru.PassThru = &PvScsiPassThru; > Dev->PassThru.GetNextTargetLun = &PvScsiGetNextTargetLun; > Dev->PassThru.BuildDevicePath = &PvScsiBuildDevicePath; > Dev->PassThru.GetTargetLun = &PvScsiGetTargetLun; > Dev->PassThru.ResetChannel = &PvScsiResetChannel; > Dev->PassThru.ResetTargetLun = &PvScsiResetTargetLun; > Dev->PassThru.GetNextTarget = &PvScsiGetNextTarget; > > // > // AdapterId is a target for which no handle will be created during bus scan. > // Prevent any conflict with real devices. > // > Dev->PassThruMode.AdapterId = MAX_UINT32; > > // > // Set both physical and logical attributes for non-RAID SCSI channel > // > Dev->PassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL | > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL; > > // > // No restriction on transfer buffer alignment > // > Dev->PassThruMode.IoAlign = 0; > > return EFI_SUCCESS; > }