Re: [PATCH edk2-platforms v2 00/16] Add PCIe Support

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 5/26/20 11:53 AM, Ard Biesheuvel wrote:
> On 5/26/20 10:37 AM, Wasim Khan wrote:
>> From: Wasim Khan <wasim.khan@nxp.com>
>>
>> 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 <ard.biesheuvel@arm.com>
> 
> 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.