Re: [RFC v2 2/2] MdeModulePkg/PciBus: return CPU address for GetBarAttributes

[Laszlo Ersek <lersek@redhat.com>]

On 02/22/18 07:54, Heyi Guo wrote:
> PciIo::GetBarAttributes should return CPU view address according to
> UEFI spec 2.7, so we change the implementation to follow the spec.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index 190f4b0..0aafcba 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -1814,8 +1814,8 @@ GetMmioAddressTranslationOffset (
>  
>    while (Configuration->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR) {
>      if ((Configuration->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) &&
> -        (Configuration->AddrRangeMin <= AddrRangeMin) &&
> -        (Configuration->AddrRangeMin + Configuration->AddrLen >= AddrRangeMin + AddrLen)
> +        (Configuration->AddrRangeMin + Configuration->AddrTranslationOffset <= AddrRangeMin) &&
> +        (Configuration->AddrRangeMin + Configuration->AddrLen + Configuration->AddrTranslationOffset >= AddrRangeMin + AddrLen)
>          ) {
>        return Configuration->AddrTranslationOffset;
>      }
> @@ -1968,6 +1968,11 @@ PciIoGetBarAttributes (
>          return EFI_UNSUPPORTED;
>        }
>      }
> +
> +    // According to UEFI spec 2.7, we need return CPU view address for PciIo::GetBarAttributes,
> +    // and PCI view = CPU view + translation
> +    Descriptor->AddrRangeMin -= Descriptor->AddrTranslationOffset;
> +    Descriptor->AddrRangeMax -= Descriptor->AddrTranslationOffset;
>    }
>  
>    return EFI_SUCCESS;
> 

According to your blurb -- which should be instead part of the commit
message of patch #1, as discussed before --, we have the following
interpretations:

* internal: host = device + ATO
* external: device = host + ATO

The GetMmioAddressTranslationOffset() change looks correct, because
(according to your blurb) RootBridgeIo->Configuration() returns a host
(CPU) view. Adding the ATO we get the device view, and then we can do
the comparison against the BAR values read from the device. OK.

In PciIoGetBarAttributes(), Descriptor->AddrRangeMin is first set from
PciIoDevice, so it's a device view. I think the subtraction is correct;
the caller will re-add the ATO if it wants to return to the device view.

In PciIoGetBarAttributes(), I think the AddrRangeMax manipulation is
incorrect (possibly due to a preexistent bug in PciBusDxe). Namely,
Descriptor->AddrRangeMax is first set to the Alignment of the BAR, from
PciIoDevice, not the end address of the BAR. In order to output the
value required by the UEFI spec, we have to calculate the end address
using AddrLen. Is that right?

... To repeat myself, I find it extremely hard to reason about this
feature while using different internal and external ATO interpretations.
Can we pick one formula and stick with it everywhere? (I don't insist,
but without it, I don't think I can sensibly review future iterations of
this set.)

Thanks
Laszlo