From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.47753.1590488281861115561 for ; Tue, 26 May 2020 03:18:01 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 802361FB; Tue, 26 May 2020 03:18:01 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.89]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A5F903F52E; Tue, 26 May 2020 03:17:59 -0700 (PDT) Subject: Re: [PATCH edk2-platforms v2 00/16] Add PCIe Support From: "Ard Biesheuvel" To: Wasim Khan , devel@edk2.groups.io, meenakshi.aggarwal@nxp.com, vabhav.sharma@nxp.com, V.Sethi@nxp.com, leif@nuviainc.com, jon@solid-run.com Cc: Wasim Khan References: <1590482241-13132-1-git-send-email-wasim.khan@oss.nxp.com> <84d23c2d-556e-b1c1-28a2-2df0f87788e6@arm.com> Message-ID: Date: Tue, 26 May 2020 12:17:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <84d23c2d-556e-b1c1-28a2-2df0f87788e6@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 5/26/20 11:53 AM, Ard Biesheuvel wrote: > On 5/26/20 10:37 AM, Wasim Khan wrote: >> From: Wasim Khan >> >> Add PCIe Support for NXP Layerscape SoC which supports >> different PCIe controllers. >> Use generic PCIe drivers and wire up PciHostBridgeLib, >> PciSegmentLib and PciCpuIo2Dxe driver for controller >> specific implementation. >> >> V1 Series can be referred here: >> https://edk2.groups.io/g/devel/message/60116?p=,,,20,0,0,0::relevance,,PCIe+Support,20,2,0,74395799 >> >> >> >> Changes in V2: >> - Addressed review comments received on V1. >> > > Thanks Wasim > > Reviewed-by: Ard Biesheuvel > > I took some liberties with the PciSegmentLib code to get rid of the > inline functions in Pcie.h - please double check whether that code is > still correct, and rebase your code before sending new work that applies > on top of these changes. > > Also, I failed to spot this in review, but preprocessor macros that > resolve to values that are used in arithmetic expressions should really > all contain outer (), or you will be pulling your hair out figuring out > where the unexpected values are coming from. I fixed this up while > committing (all in Pcie.h) > > Pushed as 7a4035e9efd8..7121691cfcbc > One thing I realized is that this method of accessing config space is not reentrant. This could potentially cause problems, e.g., when some notification callback accesses the PCI config space, and reprograms some of these windows. If such a callback interrupts an ordinary config space access between the time it programs the window and the time it does the access, you may be accessing the wrong part of config space. The usual way of dealing with this is to raise the TPL (Thread Priority Level) to TPL_NOTIFY while performing the accesses. So please take this into consideration for a followup series.