Re: [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 27 February 2018 at 09:59, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> On 2/27/2018 5:33 PM, Guo Heyi wrote:
>>
>> Hi Ray,
>>
>> Thanks for your comments. It seems comments 10 and 12 are the major issue;
>> let's
>> discuss that first.
>>
>> 1. In current implementation, namely PciBusDxe and PciHostBridgeDxe both
>> in
>> MdeModulePkg, Address parameter passed to RootBridgeIoMemRead comes from
>> PciIoMemRead().
>> 2. In PciIoMemRead(), Offset is only an offset within selected BAR; then
>> PciIoDevice->PciBar[BarIndex].BaseAddress is added to Offset in
>> PciIoVerifyBarAccess().
>
> Agree. I thought PciIoMemRead() gets the BAR base address from
> GetBarAttributes().
>
>> 3. So whether the "Address" passed to RootBridgeIoMemRead() is host
>> address or
>> device address depends on the property of
>> PciIoDevice->PciBar[BarIndex].BaseAddress.
>
> RootBridgeIoMemRead() can choose to accept HOST address or Device address.
> Either can be implemented.
> I am fine to your proposal to use Device Address.
>
>> 4. In PciBusDxe, we can see PciIoDevice->PciBar[BarIndex].BaseAddress
>> simply
>> comes from the value read from BAR register, which is absolutely a device
>> address.
>> 5. What we do in patch 3/3 is also to convert the device address in
>> PciBar[BarIndex].BaseAddress to host address before returning to the
>> caller of
>> PciIoGetBarAttributes().
>>
>> Please let me know your comments.
>>
>> For other comments, I have no strong opinion and I can follow the
>> conclusion of
>> the community.
>
>
> So the issue (13) (not 12) is closed.
> But what's your opinion to my comment (1): Explicitly add assertion and
> if-check to make sure the alignment meets requirement.
>
> Ard,
> I provided the detailed comments because I felt the code is almost
> finalized.
> So the detailed comments can avoid creating more versions of patches.
>
> For the EMPTY comments and STATIC modifier, I created the first version of
> this driver, so I'd like to make the coding style in this driver is
> consistent.
> But if you insist to use a different style, I am fine as long as the coding
> style rule allows.
>

Heyi is spending a lot of time to fix an important limitation in the
generic PCI host bridge driver, and so I would like to remain on
topic, that's all.

And for the comment style: I personally prefer the empty lines before
and after as well, but it is not reasonable to reject code that
follows the coding style.