Re: [PATCH] Enable using device address when programming BARs

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

On 14 May 2018 at 19:28, Roman Bacik <roman.bacik@broadcom.com> wrote:
> Ard,
>
> Thank you very much for your comment.
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Sunday, May 13, 2018 3:25 AM
>> To: Roman Bacik
>> Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
>> Subject: Re: [edk2] [PATCH] Enable using device address when programming
>> BARs
>>
>> On 9 May 2018 at 22:17, Roman Bacik <roman.bacik@broadcom.com> wrote:
>> > I will upload v2 with the corrected subject - add package name
>> > MdeModulePkg/Bus .
>> >
>>
>> I don't think this is the correct approach. Please use the address
>> translation
>> support that has been added recently to PciHostBridgeDxe and
>> PciHostBridgeLib.
>>
>
> Would you like to see this change:
>
>   Address = Base + Node->Offset;
> + if (UseDeviceAddress)
> +  Address = TO_DEVICE_ADDRESS(Address, -Base);
>
> Instead of:
>
> -  Address = Base + Node->Offset;
> +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
>

No.

Programming BARs should always involve device addresses, never CPU
addresses. If you wire up the MMIO translation support correctly, the
existing code will already do what you want.

>> >
>> >
>> > *From:* Roman Bacik [mailto:roman.bacik@broadcom.com]
>> > *Sent:* Thursday, May 3, 2018 3:55 PM
>> > *To:* edk2-devel@lists.01.org
>> > *Cc:* Ruiyu Ni; Vladimir Olovyannikov
>> > *Subject:* [edk2] [PATCH] Enable using device address when programming
>> > BARs
>> >
>> >
>> >
>> > Some SoCs require to use device address when BARs are programmed:
>> > https://bugzilla.tianocore.org/show_bug.cgi?id=948
>> >
>> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
>> > ---
>> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        | 1 +
>> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 8 +++++---
>> >  MdeModulePkg/MdeModulePkg.dec                       | 3 +++
>> >  MdeModulePkg/MdeModulePkg.dsc                       | 1 +
>> >  4 files changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > index 97608bfcf245..1368e5068574 100644
>> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
>> > @@ -110,6 +110,7 @@
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdAriSupport                  ##
>> CONSUMES
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMrIovSupport                ##
>> CONSUMES
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration    ##
>> > SOMETIMES_CONSUMES
>> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress        ##
>> CONSUMES
>> >
>> >  [UserExtensions.TianoCore."ExtraFiles"]
>> >    PciBusDxeExtra.uni
>> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > index 2f713fcee95e..a23bd1e258ef 100644
>> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
>> > @@ -1269,6 +1269,7 @@ ProgramBar (
>> >    EFI_PCI_IO_PROTOCOL *PciIo;
>> >    UINT64              Address;
>> >    UINT32              Address32;
>> > +  BOOLEAN             UseDeviceAddress;
>> >
>> >    ASSERT (Node->Bar < PCI_MAX_BAR);
>> >
>> > @@ -1282,8 +1283,9 @@ ProgramBar (
>> >
>> >    Address = 0;
>> >    PciIo   = &(Node->PciDev->PciIo);
>> > +  UseDeviceAddress = FeaturePcdGet (PcdUseDeviceAddress);
>> >
>> > -  Address = Base + Node->Offset;
>> > +  Address = UseDeviceAddress? Node->Offset: Base + Node->Offset;
>> >
>> >    //
>> >    // Indicate pci bus driver has allocated @@ -1308,7 +1310,7 @@
>> > ProgramBar (
>> >                   &Address
>> >                   );
>> >
>> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
>> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress?
>> > + Base +
>> > Address: Address;
>> >
>> >      break;
>> >
>> > @@ -1335,7 +1337,7 @@ ProgramBar (
>> >                   &Address32
>> >                   );
>> >
>> > -    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
>> > +    Node->PciDev->PciBar[Node->Bar].BaseAddress = UseDeviceAddress?
>> > + Base +
>> > Address: Address;
>> >
>> >      break;
>> >
>> > diff --git a/MdeModulePkg/MdeModulePkg.dec
>> > b/MdeModulePkg/MdeModulePkg.dec index cc397185f7b9..58425ee0d57f
>> > 100644
>> > --- a/MdeModulePkg/MdeModulePkg.dec
>> > +++ b/MdeModulePkg/MdeModulePkg.dec
>> > @@ -1005,6 +1005,9 @@
>> >    # @Prompt Enable UEFI Stack Guard.
>> >
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|FALSE|BOOLEAN|0
>> x300010
>> > 55
>> >
>> > +  ## Indicates whether the device address should be used for BAR
>> > programming
>> > +
>> >
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE|BOOLEA
>> N|0x300
>> > 01056
>> > +
>> >  [PcdsFixedAtBuild, PcdsPatchableInModule]
>> >    ## Dynamic type PCD can be registered callback function for Pcd
>> > setting action.
>> >    #  PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum
>> > number of callback function diff --git
>> a/MdeModulePkg/MdeModulePkg.dsc
>> > b/MdeModulePkg/MdeModulePkg.dsc index
>> ec24a50c7d0a..39b397cb13d9
>> > 100644
>> > --- a/MdeModulePkg/MdeModulePkg.dsc
>> > +++ b/MdeModulePkg/MdeModulePkg.dsc
>> > @@ -200,6 +200,7 @@
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|0x0
>> >    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizePopulateCapsule|0x0
>> >
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxPeiPerformanceLogEntries|28
>> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdUseDeviceAddress|FALSE
>> >
>> >  [PcdsFixedAtBuild.IPF]
>> >
>> gEfiMdePkgTokenSpaceGuid.PcdIoBlockBaseAddressForIpf|0x0ffffc000000
>> > --
>> > 1.9.1
>> > _______________________________________________
>> > edk2-devel mailing list
>> > edk2-devel@lists.01.org
>> > https://lists.01.org/mailman/listinfo/edk2-devel
>
> Thanks,
>
> Roman