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

[Roman Bacik <roman.bacik@broadcom.com>]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, May 14, 2018 10:35 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 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.
>

Our code has base address 0x60000000 and offset 0x200. We need Address=0x200
to be passed to PciIo->Pci.Write() and Address=0x60000200 to be stored in
Node->PciDev->PciBar[Node->Bar].BaseAddress. The code as is does not seem to
do that unless PciIo->Pci.Write() itself changes the content of the Address
from 0x200 to 0x60000200 for us.

So if we modify MMIO and the existing code gets the correct Address 0x200
being passed to PciIo->Pci.Write() then it means Address = 0x200 will be
stored into Node->PciDev->PciBar[Node->Bar].BaseAddress, which would be
wrong in our case. It appears that regardless how we change MMIO translation
the existing code would not work for us as is. Or I may be missing
something.

> >> >
> >> >
> >> > *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