Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib

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

On 24 May 2018 at 16:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 24 May 2018 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 05/24/18 10:13, Ard Biesheuvel wrote:
>>> On 23 May 2018 at 22:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> Add a library class, and a UEFI_DRIVER lib instance, that are layered on
>>>> top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend
>>>> into PciCapLib, for config space access.
>>>>
>>>> (Side note:
>>>>
>>>
>>> Again, please retain the below.
>>
>> Will do.
>>
>>>> +STATIC
>>>> +EFI_STATUS
>>>> +ProtoDevTransferConfig (
>>>> +  IN     EFI_PCI_IO_PROTOCOL        *PciIo,
>>>> +  IN     EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction,
>>>> +  IN     UINT16                     ConfigOffset,
>>>> +  IN OUT UINT8                      *Buffer,
>>>> +  IN     UINT16                     Size
>>>> +  )
>>>> +{
>>>> +  while (Size > 0) {
>>>> +    EFI_PCI_IO_PROTOCOL_WIDTH Width;
>>>> +    UINT16                    Count;
>>>> +    EFI_STATUS                Status;
>>>> +    UINT16                    Progress;
>>>> +
>>>> +    //
>>>> +    // Pick the largest access size that is allowed by the remaining transfer
>>>> +    // Size and by the alignment of ConfigOffset.
>>>> +    //
>>>> +    // When the largest access size is available, transfer as many bytes as
>>>> +    // possible in one iteration of the loop. Otherwise, transfer only one
>>>> +    // unit, to improve the alignment.
>>>> +    //
>>>> +    if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) {
>>>
>>> Ugh. Just use '4' or sizeof(UINT32).
>>>
>>>> +      Width = EfiPciIoWidthUint32;
>>>> +      Count = Size >> Width;
>>>> +    } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) {
>>>> +      Width = EfiPciIoWidthUint16;
>>>> +      Count = 1;
>>>> +    } else {
>>>> +      Width = EfiPciIoWidthUint8;
>>>> +      Count = 1;
>>>> +    }
>>
>> I used "BITx" and "(BITx - 1)" for consistency, and because they seemed
>> to convey the idea well (namely, shifting down the alignment).
>>
>> I'm fine replacing "BIT2" with "4", but then I believe I should also
>> replace "(BIT2 - 1)" with "3". Similarly, replace "BIT1" with "2", and
>> "(BIT1 -1)" with 1.
>>
>> Do you prefer the current code or the decimal constants?
>>
>
> IMHO, BITx is for bitmasks, not for numerical constants.
>
> So yes, if you think (as do I) that sizeof(UINTnn) is too wordy, just
> use the plain numbers please.


With that,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>