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

[Guo Heyi <heyi.guo@linaro.org>]

On Thu, Feb 22, 2018 at 11:41:50AM +0100, Laszlo Ersek wrote:
> 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?

Will double check what it really is.

> 
> ... 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.)

I made the patch according to the conclusion here:
https://lists.01.org/pipermail/edk2-devel/2018-February/020960.html
if I understood that correctly :)

However, if it turns out so confusing in the code, especially to someone who
didn't participate in the discussions, I agree it may worth keeping all the
definitions consistent in EDK2 code, while being different from what in ACPI ASL
code.

Thanks,

Gary (Heyi Guo)

> 
> Thanks
> Laszlo