mbox series

[edk2,edk2-platforms,00/12] Hisilicon/D0x: Switch to generic PciHostBridge

Message ID 1521594198-52523-1-git-send-email-heyi.guo@linaro.org
Headers show
Series Hisilicon/D0x: Switch to generic PciHostBridge | expand

Message

gary guo March 21, 2018, 1:03 a.m. UTC
For BAR address translation support was added to edk2 generic PciHostBridge by
commit 74d0a33, now we can also use it for D03/D05 platforms.
This series of patches include 3 parts of change:
- Preparation for the switch, moving platform specific code out of PciHostBridge
  driver.
- Add depending libraries and protocol implementations, like PciHostBridgeLib,
  PciSegmentLib and CpuIo2 Protocol.
- Other enhancement and refinement.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Haojian Zhuang <haojian.zhuang@linaro.org>

Heyi Guo (12):
  Hisilicon: Enable WARN and INFO debug message
  Hisilicon/D05/PlatformPciLib: fix misuse of macro
  Hisilicon/Pci: move ATU configuration to PcieInitDxe
  Hisilicon/Pci: Merge PciPlatform into PcieInit Driver
  Hisilicon/Pci: Move EnlargeAtuConfig0() to PcieInitDxe
  Hisilicon/PlatformPciLib: add segment for each root bridge
  Hisilicon: add PciHostBridgeLib
  Hisilicon: add PciCpuIo2Dxe
  Hisilicon: add PciSegmentLib for Hi161x
  Hisilicon/D0x: Switch to generic PciHostBridge driver
  Hisilicon: remove platform specific PciHostBridge
  Hisilicon/PlatformPciLib: clear redundant felds in RESOURCE_APPETURE

 Silicon/Hisilicon/Hisilicon.dsc.inc                                                                         |    8 +-
 Platform/Hisilicon/D03/D03.dsc                                                                              |    7 +-
 Platform/Hisilicon/D05/D05.dsc                                                                              |    7 +-
 Platform/Hisilicon/D03/D03.fdf                                                                              |    4 +-
 Platform/Hisilicon/D05/D05.fdf                                                                              |    4 +-
 Platform/Hisilicon/D03/Drivers/PciPlatform/PciPlatform.inf                                                  |   53 -
 Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.inf                                            |   51 +
 Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf                                               |   48 +
 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf                                             |   74 -
 Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitDxe.inf                                               |    9 +-
 Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/Hi161xPciSegmentLib.inf                                |   36 +
 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h                                                  |  528 -----
 {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.h |    0
 Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.h                                                 |   13 +
 Silicon/Hisilicon/Include/Library/PlatformPciLib.h                                                          |    3 +-
 Platform/Hisilicon/D03/Library/PlatformPciLib/PlatformPciLib.c                                              |   24 +-
 Platform/Hisilicon/D05/Library/PlatformPciLib/PlatformPciLib.c                                              |   66 +-
 Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.c                                              |  304 +++
 Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.c                                                 |  557 +++++
 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c                                                  | 1659 --------------
 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c                                                | 2404 --------------------
 {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.c |   12 +
 Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInit.c                                                    |    7 +-
 Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c                                                 |  309 +++
 Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/PciSegmentLib.c                                        | 1503 ++++++++++++
 25 files changed, 2897 insertions(+), 4793 deletions(-)
 delete mode 100644 Platform/Hisilicon/D03/Drivers/PciPlatform/PciPlatform.inf
 create mode 100644 Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.inf
 create mode 100644 Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf
 delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf
 create mode 100644 Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/Hi161xPciSegmentLib.inf
 delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h
 rename {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.h (100%)
 create mode 100644 Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.c
 create mode 100644 Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.c
 delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c
 delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
 rename {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.c (93%)
 create mode 100644 Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c
 create mode 100644 Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/PciSegmentLib.c

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

gary guo March 28, 2018, 1:05 a.m. UTC | #1
Hi Leif, Ard,

Any comments for this series of patches?

Thanks,
Heyi

On Wed, Mar 21, 2018 at 09:03:06AM +0800, Heyi Guo wrote:
> For BAR address translation support was added to edk2 generic PciHostBridge by

> commit 74d0a33, now we can also use it for D03/D05 platforms.

> This series of patches include 3 parts of change:

> - Preparation for the switch, moving platform specific code out of PciHostBridge

>   driver.

> - Add depending libraries and protocol implementations, like PciHostBridgeLib,

>   PciSegmentLib and CpuIo2 Protocol.

> - Other enhancement and refinement.

> 

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>

> Cc: Haojian Zhuang <haojian.zhuang@linaro.org>

> 

> Heyi Guo (12):

>   Hisilicon: Enable WARN and INFO debug message

>   Hisilicon/D05/PlatformPciLib: fix misuse of macro

>   Hisilicon/Pci: move ATU configuration to PcieInitDxe

>   Hisilicon/Pci: Merge PciPlatform into PcieInit Driver

>   Hisilicon/Pci: Move EnlargeAtuConfig0() to PcieInitDxe

>   Hisilicon/PlatformPciLib: add segment for each root bridge

>   Hisilicon: add PciHostBridgeLib

>   Hisilicon: add PciCpuIo2Dxe

>   Hisilicon: add PciSegmentLib for Hi161x

>   Hisilicon/D0x: Switch to generic PciHostBridge driver

>   Hisilicon: remove platform specific PciHostBridge

>   Hisilicon/PlatformPciLib: clear redundant felds in RESOURCE_APPETURE

> 

>  Silicon/Hisilicon/Hisilicon.dsc.inc                                                                         |    8 +-

>  Platform/Hisilicon/D03/D03.dsc                                                                              |    7 +-

>  Platform/Hisilicon/D05/D05.dsc                                                                              |    7 +-

>  Platform/Hisilicon/D03/D03.fdf                                                                              |    4 +-

>  Platform/Hisilicon/D05/D05.fdf                                                                              |    4 +-

>  Platform/Hisilicon/D03/Drivers/PciPlatform/PciPlatform.inf                                                  |   53 -

>  Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.inf                                            |   51 +

>  Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf                                               |   48 +

>  Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf                                             |   74 -

>  Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitDxe.inf                                               |    9 +-

>  Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/Hi161xPciSegmentLib.inf                                |   36 +

>  Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h                                                  |  528 -----

>  {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.h |    0

>  Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitLib.h                                                 |   13 +

>  Silicon/Hisilicon/Include/Library/PlatformPciLib.h                                                          |    3 +-

>  Platform/Hisilicon/D03/Library/PlatformPciLib/PlatformPciLib.c                                              |   24 +-

>  Platform/Hisilicon/D05/Library/PlatformPciLib/PlatformPciLib.c                                              |   66 +-

>  Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.c                                              |  304 +++

>  Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.c                                                 |  557 +++++

>  Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c                                                  | 1659 --------------

>  Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c                                                | 2404 --------------------

>  {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.c |   12 +

>  Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInit.c                                                    |    7 +-

>  Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c                                                 |  309 +++

>  Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/PciSegmentLib.c                                        | 1503 ++++++++++++

>  25 files changed, 2897 insertions(+), 4793 deletions(-)

>  delete mode 100644 Platform/Hisilicon/D03/Drivers/PciPlatform/PciPlatform.inf

>  create mode 100644 Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.inf

>  create mode 100644 Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf

>  delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridgeDxe.inf

>  create mode 100644 Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/Hi161xPciSegmentLib.inf

>  delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.h

>  rename {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.h (100%)

>  create mode 100644 Platform/Hisilicon/Library/PciHostBridgeLib/PciHostBridgeLib.c

>  create mode 100644 Silicon/Hisilicon/Drivers/ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.c

>  delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciHostBridge.c

>  delete mode 100644 Silicon/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c

>  rename {Platform/Hisilicon/D03/Drivers/PciPlatform => Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610}/PciPlatform.c (93%)

>  create mode 100644 Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c

>  create mode 100644 Silicon/Hisilicon/Hi1610/Library/Hi161xPciSegmentLib/PciSegmentLib.c

> 

> -- 

> 2.7.4

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 28, 2018, 9:43 a.m. UTC | #2
On 28 March 2018 at 02:05, Guo Heyi <heyi.guo@linaro.org> wrote:
> Hi Leif, Ard,

>

> Any comments for this series of patches?

>


Hello Heyi,

Thanks for sending these patches. Leif is at the plugfest, but I will
look at these before the end of the week.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 29, 2018, 12:20 a.m. UTC | #3
On Wed, Mar 28, 2018 at 10:43:41AM +0100, Ard Biesheuvel wrote:
> On 28 March 2018 at 02:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> > Hi Leif, Ard,

> >

> > Any comments for this series of patches?

> >

> 

> Hello Heyi,

> 

> Thanks for sending these patches. Leif is at the plugfest, but I will

> look at these before the end of the week.


Forgot the plugfest as I am not attending :)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 30, 2018, 3:40 p.m. UTC | #4
On 29 March 2018 at 02:20, Guo Heyi <heyi.guo@linaro.org> wrote:
> On Wed, Mar 28, 2018 at 10:43:41AM +0100, Ard Biesheuvel wrote:

>> On 28 March 2018 at 02:05, Guo Heyi <heyi.guo@linaro.org> wrote:

>> > Hi Leif, Ard,

>> >

>> > Any comments for this series of patches?

>> >

>>

>> Hello Heyi,

>>

>> Thanks for sending these patches. Leif is at the plugfest, but I will

>> look at these before the end of the week.

>

> Forgot the plugfest as I am not attending :)


Hello Heyi,

I think the series looks mostly fine in general, but there are two
things that I'd like you to change (as noted in my replies):
- please split the PCI to CPU I/O translation from the CPU I/O to CPU
MMIO translation
- please fix the APPETURE spelling (in whichever way is most
convenient for you: separate patch at the beginning, at the end, etc
etc)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 31, 2018, 1:37 a.m. UTC | #5
Hi Ard,

Thanks for your time of reviewing the patches.
Please see my opinions below.

On Fri, Mar 30, 2018 at 05:40:20PM +0200, Ard Biesheuvel wrote:
> On 29 March 2018 at 02:20, Guo Heyi <heyi.guo@linaro.org> wrote:

> > On Wed, Mar 28, 2018 at 10:43:41AM +0100, Ard Biesheuvel wrote:

> >> On 28 March 2018 at 02:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> >> > Hi Leif, Ard,

> >> >

> >> > Any comments for this series of patches?

> >> >

> >>

> >> Hello Heyi,

> >>

> >> Thanks for sending these patches. Leif is at the plugfest, but I will

> >> look at these before the end of the week.

> >

> > Forgot the plugfest as I am not attending :)

> 

> Hello Heyi,

> 

> I think the series looks mostly fine in general, but there are two

> things that I'd like you to change (as noted in my replies):

> - please split the PCI to CPU I/O translation from the CPU I/O to CPU

> MMIO translation


I heard that OS did the same as you indicated, but the reasons of why I
translated IO address into memory address in PCI host bridge are like below:

1. If we add an intermediate level of "CPU IO address space", it makes things a
little more complicated but I don't see any real benefit. If we just make a
simple policy that on ARM/AARCH64 CPU IO address is equal to CPU memory address,
we can even use a unified CPU IO driver for all ARM/AARCH64 platforms, while the
translation is covered by PCI host bridge driver.

2. From hardware perspective, the translation is done by PCIe ATU; for IO bar
access, the address from CPU to ATU is a CPU memory address, not an intermediate
CPU IO address; the intermediate CPU IO address is a totally logical concept,
and it also splits ATU function into separate drivers (CPU IO protocol driver
also sees part of ATU function).

Please let me know if my understanding is not right.

> - please fix the APPETURE spelling (in whichever way is most

> convenient for you: separate patch at the beginning, at the end, etc

> etc)


Nice catch :) will fix that.

Thanks,

Heyi
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo April 13, 2018, 2:05 a.m. UTC | #6
Hi Ard,

Any comments?

Anyway we can modify the code if you insist on using an intermediate CPU IO
address space.

Thanks,

Heyi

On Sat, Mar 31, 2018 at 09:37:47AM +0800, Guo Heyi wrote:
> Hi Ard,

> 

> Thanks for your time of reviewing the patches.

> Please see my opinions below.

> 

> On Fri, Mar 30, 2018 at 05:40:20PM +0200, Ard Biesheuvel wrote:

> > On 29 March 2018 at 02:20, Guo Heyi <heyi.guo@linaro.org> wrote:

> > > On Wed, Mar 28, 2018 at 10:43:41AM +0100, Ard Biesheuvel wrote:

> > >> On 28 March 2018 at 02:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> > >> > Hi Leif, Ard,

> > >> >

> > >> > Any comments for this series of patches?

> > >> >

> > >>

> > >> Hello Heyi,

> > >>

> > >> Thanks for sending these patches. Leif is at the plugfest, but I will

> > >> look at these before the end of the week.

> > >

> > > Forgot the plugfest as I am not attending :)

> > 

> > Hello Heyi,

> > 

> > I think the series looks mostly fine in general, but there are two

> > things that I'd like you to change (as noted in my replies):

> > - please split the PCI to CPU I/O translation from the CPU I/O to CPU

> > MMIO translation

> 

> I heard that OS did the same as you indicated, but the reasons of why I

> translated IO address into memory address in PCI host bridge are like below:

> 

> 1. If we add an intermediate level of "CPU IO address space", it makes things a

> little more complicated but I don't see any real benefit. If we just make a

> simple policy that on ARM/AARCH64 CPU IO address is equal to CPU memory address,

> we can even use a unified CPU IO driver for all ARM/AARCH64 platforms, while the

> translation is covered by PCI host bridge driver.

> 

> 2. From hardware perspective, the translation is done by PCIe ATU; for IO bar

> access, the address from CPU to ATU is a CPU memory address, not an intermediate

> CPU IO address; the intermediate CPU IO address is a totally logical concept,

> and it also splits ATU function into separate drivers (CPU IO protocol driver

> also sees part of ATU function).

> 

> Please let me know if my understanding is not right.

> 

> > - please fix the APPETURE spelling (in whichever way is most

> > convenient for you: separate patch at the beginning, at the end, etc

> > etc)

> 

> Nice catch :) will fix that.

> 

> Thanks,

> 

> Heyi

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 13, 2018, 7:19 a.m. UTC | #7
On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:
> Hi Ard,

>

> Any comments?

>


Apologies for the delay. I have been travelling and am behind on email.

> Anyway we can modify the code if you insist on using an intermediate CPU IO

> address space.

>


I have not made up my mind yet, to be honest. I agree there is a
certain elegance to merging both translations, but I am concerned that
existing EDK2 code may deal poorly with I/O addresses that require
more than 32 bits to express.

Did you try the mm command in the shell for instance? As you know, I
recently removed an artificial address range limit there, but I wonder
if it uses 64-bit variables for I/O ports.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo April 16, 2018, 1:57 p.m. UTC | #8
Thanks, I will test mm command and let you know the result.

Regards,

Heyi

On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:
> On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> > Hi Ard,

> >

> > Any comments?

> >

> 

> Apologies for the delay. I have been travelling and am behind on email.

> 

> > Anyway we can modify the code if you insist on using an intermediate CPU IO

> > address space.

> >

> 

> I have not made up my mind yet, to be honest. I agree there is a

> certain elegance to merging both translations, but I am concerned that

> existing EDK2 code may deal poorly with I/O addresses that require

> more than 32 bits to express.

> 

> Did you try the mm command in the shell for instance? As you know, I

> recently removed an artificial address range limit there, but I wonder

> if it uses 64-bit variables for I/O ports.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo April 17, 2018, 1:20 a.m. UTC | #9
Hi Ard,

I tested mm -io on D05, for root bridge 4 with CPU IO address starting from
0x8_abff0000, and it worked; both mm -io 0x8abff0000 and mm 0x8abff0000 provided
the same output. It seems there is no other limit for 64bit IO address after you
fixed the issue in EFI shell mm command.

Thanks and regards,

Heyi

On Mon, Apr 16, 2018 at 09:57:09PM +0800, Guo Heyi wrote:
> Thanks, I will test mm command and let you know the result.

> 

> Regards,

> 

> Heyi

> 

> On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:

> > On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> > > Hi Ard,

> > >

> > > Any comments?

> > >

> > 

> > Apologies for the delay. I have been travelling and am behind on email.

> > 

> > > Anyway we can modify the code if you insist on using an intermediate CPU IO

> > > address space.

> > >

> > 

> > I have not made up my mind yet, to be honest. I agree there is a

> > certain elegance to merging both translations, but I am concerned that

> > existing EDK2 code may deal poorly with I/O addresses that require

> > more than 32 bits to express.

> > 

> > Did you try the mm command in the shell for instance? As you know, I

> > recently removed an artificial address range limit there, but I wonder

> > if it uses 64-bit variables for I/O ports.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo April 17, 2018, 1:44 a.m. UTC | #10
BTW, there is actually a bug with ATU configuration which will cause IO access
failure, and we need to apply an additional patch (this patch is generated after
PCI host bridge patch series) as attached to fix this.

Regards,
Heyi

On Tue, Apr 17, 2018 at 09:20:44AM +0800, Guo Heyi wrote:
> Hi Ard,

> 

> I tested mm -io on D05, for root bridge 4 with CPU IO address starting from

> 0x8_abff0000, and it worked; both mm -io 0x8abff0000 and mm 0x8abff0000 provided

> the same output. It seems there is no other limit for 64bit IO address after you

> fixed the issue in EFI shell mm command.

> 

> Thanks and regards,

> 

> Heyi

> 

> On Mon, Apr 16, 2018 at 09:57:09PM +0800, Guo Heyi wrote:

> > Thanks, I will test mm command and let you know the result.

> > 

> > Regards,

> > 

> > Heyi

> > 

> > On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:

> > > On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> > > > Hi Ard,

> > > >

> > > > Any comments?

> > > >

> > > 

> > > Apologies for the delay. I have been travelling and am behind on email.

> > > 

> > > > Anyway we can modify the code if you insist on using an intermediate CPU IO

> > > > address space.

> > > >

> > > 

> > > I have not made up my mind yet, to be honest. I agree there is a

> > > certain elegance to merging both translations, but I am concerned that

> > > existing EDK2 code may deal poorly with I/O addresses that require

> > > more than 32 bits to express.

> > > 

> > > Did you try the mm command in the shell for instance? As you know, I

> > > recently removed an artificial address range limit there, but I wonder

> > > if it uses 64-bit variables for I/O ports.
From guoheyi@huawei.com Tue Apr 17 09:40:07 2018
Delivered-To: heyi.guo@linaro.org
Received: by 10.103.107.2 with SMTP id g2csp1147351vsc;
        Mon, 16 Apr 2018 18:40:07 -0700 (PDT)
X-Google-Smtp-Source: AIpwx49xa2EjXi3IIuqoYaJ9ZR+KlZePkYWmAvMpJl534IXT0zWt/Jd5UHxMjyCgas2Aluws+S26
X-Received: by 10.101.97.165 with SMTP id i5mr102113pgv.449.1523929207660;
        Mon, 16 Apr 2018 18:40:07 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1523929207; cv=none;
        d=google.com; s=arc-20160816;
        b=JVhwQqc2tHJH/nbb9J4Q5EAe7efNCva8XlNdyeM1AjecnvMtalXxRiIcVSIp2CTTUb
         RCfB400z6ay0s7SW96MDtN0D8jezfXLZXyuMQTqYAvpXu7rxEmXGJtKblXaJ303GTvgS
         kavhZQ7QawTzXzWTjokcARGeyR+mFb9aXT8JKm3jdHVM+Ibsjl0HFYSFrMNFoeyC7EqG
         EPMbjEvcWolADxpywSCM0s0e4EhIVpGrhi1LgdVeATsyrb6GN8sEscVfb+t0b0gMj6XI
         Nqg7mMHnKZGJ4vmkJBRV5Gx+nnQbOWKsOCp8YZcVLFs8hyqWFokYsup3wsY1YtktovxU
         GQUg==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816;
        h=mime-version:message-id:date:subject:cc:to:from
         :arc-authentication-results;
        bh=MzafS17Rdgthy05RGe4B87hGASmPkGgjBWmjX1ExxLo=;
        b=fj+XbZcdH1vGFsHllnQeTc3zRavri5L0Use7rHdCRemtfNojWbY1QJKD6tMVi213fj
         J9IaKL5/Q3Uh0KLfAR848Whbuj4IiFMw1vRsT8BUvu0QjkCsdJo66ypGdQTqkysdtk2X
         fdNfNTE+Dfa1du8Yx9RiFCetXrtgCjDXljVzd/JufSlKtC41HjgWjnqC2Jka4N0MOT1W
         oiBtsqOCjRuVRyb5Rlf7RVstwZ7wz6jXbxj7GYkr85bUFe7LNG8HOKxJYIv1bLyOWw6q
         KvttRU9MKaVex3HApPoMIIgg+sXj3QD1islzlobdLA/OaRKWXCH0fnXH/NuWnKZ7J1jO
         Hl7A==
ARC-Authentication-Results: i=1; mx.google.com;
       spf=pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.32 as permitted sender) smtp.mailfrom=guoheyi@huawei.com
Return-Path: <guoheyi@huawei.com>
Received: from huawei.com ([45.249.212.32])
        by mx.google.com with ESMTPS id t3si10564595pgt.547.2018.04.16.18.40.07
        for <heyi.guo@linaro.org>
        (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
        Mon, 16 Apr 2018 18:40:07 -0700 (PDT)
Received-SPF: pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.32 as permitted sender) client-ip=45.249.212.32;
Authentication-Results: mx.google.com;
       spf=pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.32 as permitted sender) smtp.mailfrom=guoheyi@huawei.com
Received: from DGGEMS405-HUB.china.huawei.com (unknown [172.30.72.58])
	by Forcepoint Email with ESMTP id 2B03711A12683
	for <heyi.guo@linaro.org>; Tue, 17 Apr 2018 09:40:04 +0800 (CST)
Received: from HGH1000039998.huawei.com (10.184.68.188) by
 DGGEMS405-HUB.china.huawei.com (10.3.19.205) with Microsoft SMTP Server id
 14.3.361.1; Tue, 17 Apr 2018 09:39:59 +0800
From: Heyi Guo <guoheyi@huawei.com>

To: <heyi.guo@linaro.org>
CC: <phoenix.liyi@huawei.com>, <mengfanrong@huawei.com>,
	<zhangjinsong2@huawei.com>
Subject: [PATCH] Hisilicon/Hi161x/PcieInit: fix address overlap
Date: Tue, 17 Apr 2018 09:35:22 +0800
Message-ID: <1523928922-9573-1-git-send-email-guoheyi@huawei.com>
X-Mailer: git-send-email 2.8.1
MIME-Version: 1.0
Content-Type: text/plain
X-Originating-IP: [10.184.68.188]
X-CFilter-Loop: Reflected
Content-Length: 2888
Lines: 55

From: Heyi Guo <heyi.guo@linaro.org>


PCIe IO address ranges are overlapped by configuration address spaces
when we set CFG0/CFG1 address range starting from ECAM. It causes
access to IO space is routed to configuration space and returned with
wrong results.

So we limit address space for configuration type 0
starting from BusBase and type 1 from (BusBase + 2), to eliminate the
address range overlapping.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

---
 Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c b/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c
index f2365b5..9a92fea 100644
--- a/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c
+++ b/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c
@@ -96,11 +96,12 @@ SetAtuConfig0RW (
 {
   UINTN RbPciBase = Private->RbPciBar;
   UINT64 MemLimit = GetPcieCfgAddress (Private->Ecam, Private->BusBase + 1, 1, 0, 0) - 1;
+  UINT64 MemBase = GetPcieCfgAddress (Private->Ecam, Private->BusBase, 0, 0, 0);
 
 
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_VIEW_POINT, Index);
-  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)(Private->Ecam));
-  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)((UINT64)(Private->Ecam) >> 32));
+  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)MemBase);
+  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)(MemBase >> 32));
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LIMIT, (UINT32) MemLimit);
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_LOW, 0);
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_HIGH, 0);
@@ -124,12 +125,13 @@ SetAtuConfig1RW (
 {
   UINTN RbPciBase = Private->RbPciBar;
   UINT64 MemLimit = GetPcieCfgAddress (Private->Ecam, Private->BusLimit + 1, 0, 0, 0) - 1;
+  UINT64 MemBase = GetPcieCfgAddress (Private->Ecam, Private->BusBase + 2, 0, 0, 0);
 
 
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_VIEW_POINT, Index);
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_CTRL1, IATU_CTRL1_TYPE_CONFIG1);
-  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)(Private->Ecam));
-  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)((UINT64)(Private->Ecam) >> 32));
+  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)MemBase);
+  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)(MemBase >> 32));
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LIMIT, (UINT32) MemLimit);
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_LOW, 0);
   MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_HIGH, 0);
-- 
2.8.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo May 31, 2018, 1:02 a.m. UTC | #11
Hi Ard,

Have you returned from vocation? If so, could you help to continue reviewing
this patch series?

Thanks,

Heyi

On Tue, Apr 17, 2018 at 09:44:46AM +0800, Guo Heyi wrote:
> BTW, there is actually a bug with ATU configuration which will cause IO access

> failure, and we need to apply an additional patch (this patch is generated after

> PCI host bridge patch series) as attached to fix this.

> 

> Regards,

> Heyi

> 

> On Tue, Apr 17, 2018 at 09:20:44AM +0800, Guo Heyi wrote:

> > Hi Ard,

> > 

> > I tested mm -io on D05, for root bridge 4 with CPU IO address starting from

> > 0x8_abff0000, and it worked; both mm -io 0x8abff0000 and mm 0x8abff0000 provided

> > the same output. It seems there is no other limit for 64bit IO address after you

> > fixed the issue in EFI shell mm command.

> > 

> > Thanks and regards,

> > 

> > Heyi

> > 

> > On Mon, Apr 16, 2018 at 09:57:09PM +0800, Guo Heyi wrote:

> > > Thanks, I will test mm command and let you know the result.

> > > 

> > > Regards,

> > > 

> > > Heyi

> > > 

> > > On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:

> > > > On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> > > > > Hi Ard,

> > > > >

> > > > > Any comments?

> > > > >

> > > > 

> > > > Apologies for the delay. I have been travelling and am behind on email.

> > > > 

> > > > > Anyway we can modify the code if you insist on using an intermediate CPU IO

> > > > > address space.

> > > > >

> > > > 

> > > > I have not made up my mind yet, to be honest. I agree there is a

> > > > certain elegance to merging both translations, but I am concerned that

> > > > existing EDK2 code may deal poorly with I/O addresses that require

> > > > more than 32 bits to express.

> > > > 

> > > > Did you try the mm command in the shell for instance? As you know, I

> > > > recently removed an artificial address range limit there, but I wonder

> > > > if it uses 64-bit variables for I/O ports.


> From guoheyi@huawei.com Tue Apr 17 09:40:07 2018

> Delivered-To: heyi.guo@linaro.org

> Received: by 10.103.107.2 with SMTP id g2csp1147351vsc;

>         Mon, 16 Apr 2018 18:40:07 -0700 (PDT)

> X-Google-Smtp-Source: AIpwx49xa2EjXi3IIuqoYaJ9ZR+KlZePkYWmAvMpJl534IXT0zWt/Jd5UHxMjyCgas2Aluws+S26

> X-Received: by 10.101.97.165 with SMTP id i5mr102113pgv.449.1523929207660;

>         Mon, 16 Apr 2018 18:40:07 -0700 (PDT)

> ARC-Seal: i=1; a=rsa-sha256; t=1523929207; cv=none;

>         d=google.com; s=arc-20160816;

>         b=JVhwQqc2tHJH/nbb9J4Q5EAe7efNCva8XlNdyeM1AjecnvMtalXxRiIcVSIp2CTTUb

>          RCfB400z6ay0s7SW96MDtN0D8jezfXLZXyuMQTqYAvpXu7rxEmXGJtKblXaJ303GTvgS

>          kavhZQ7QawTzXzWTjokcARGeyR+mFb9aXT8JKm3jdHVM+Ibsjl0HFYSFrMNFoeyC7EqG

>          EPMbjEvcWolADxpywSCM0s0e4EhIVpGrhi1LgdVeATsyrb6GN8sEscVfb+t0b0gMj6XI

>          Nqg7mMHnKZGJ4vmkJBRV5Gx+nnQbOWKsOCp8YZcVLFs8hyqWFokYsup3wsY1YtktovxU

>          GQUg==

> ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816;

>         h=mime-version:message-id:date:subject:cc:to:from

>          :arc-authentication-results;

>         bh=MzafS17Rdgthy05RGe4B87hGASmPkGgjBWmjX1ExxLo=;

>         b=fj+XbZcdH1vGFsHllnQeTc3zRavri5L0Use7rHdCRemtfNojWbY1QJKD6tMVi213fj

>          J9IaKL5/Q3Uh0KLfAR848Whbuj4IiFMw1vRsT8BUvu0QjkCsdJo66ypGdQTqkysdtk2X

>          fdNfNTE+Dfa1du8Yx9RiFCetXrtgCjDXljVzd/JufSlKtC41HjgWjnqC2Jka4N0MOT1W

>          oiBtsqOCjRuVRyb5Rlf7RVstwZ7wz6jXbxj7GYkr85bUFe7LNG8HOKxJYIv1bLyOWw6q

>          KvttRU9MKaVex3HApPoMIIgg+sXj3QD1islzlobdLA/OaRKWXCH0fnXH/NuWnKZ7J1jO

>          Hl7A==

> ARC-Authentication-Results: i=1; mx.google.com;

>        spf=pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.32 as permitted sender) smtp.mailfrom=guoheyi@huawei.com

> Return-Path: <guoheyi@huawei.com>

> Received: from huawei.com ([45.249.212.32])

>         by mx.google.com with ESMTPS id t3si10564595pgt.547.2018.04.16.18.40.07

>         for <heyi.guo@linaro.org>

>         (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);

>         Mon, 16 Apr 2018 18:40:07 -0700 (PDT)

> Received-SPF: pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.32 as permitted sender) client-ip=45.249.212.32;

> Authentication-Results: mx.google.com;

>        spf=pass (google.com: domain of guoheyi@huawei.com designates 45.249.212.32 as permitted sender) smtp.mailfrom=guoheyi@huawei.com

> Received: from DGGEMS405-HUB.china.huawei.com (unknown [172.30.72.58])

> 	by Forcepoint Email with ESMTP id 2B03711A12683

> 	for <heyi.guo@linaro.org>; Tue, 17 Apr 2018 09:40:04 +0800 (CST)

> Received: from HGH1000039998.huawei.com (10.184.68.188) by

>  DGGEMS405-HUB.china.huawei.com (10.3.19.205) with Microsoft SMTP Server id

>  14.3.361.1; Tue, 17 Apr 2018 09:39:59 +0800

> From: Heyi Guo <guoheyi@huawei.com>

> To: <heyi.guo@linaro.org>

> CC: <phoenix.liyi@huawei.com>, <mengfanrong@huawei.com>,

> 	<zhangjinsong2@huawei.com>

> Subject: [PATCH] Hisilicon/Hi161x/PcieInit: fix address overlap

> Date: Tue, 17 Apr 2018 09:35:22 +0800

> Message-ID: <1523928922-9573-1-git-send-email-guoheyi@huawei.com>

> X-Mailer: git-send-email 2.8.1

> MIME-Version: 1.0

> Content-Type: text/plain

> X-Originating-IP: [10.184.68.188]

> X-CFilter-Loop: Reflected

> Content-Length: 2888

> Lines: 55

> 

> From: Heyi Guo <heyi.guo@linaro.org>

> 

> PCIe IO address ranges are overlapped by configuration address spaces

> when we set CFG0/CFG1 address range starting from ECAM. It causes

> access to IO space is routed to configuration space and returned with

> wrong results.

> 

> So we limit address space for configuration type 0

> starting from BusBase and type 1 from (BusBase + 2), to eliminate the

> address range overlapping.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> ---

>  Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c | 10 ++++++----

>  1 file changed, 6 insertions(+), 4 deletions(-)

> 

> diff --git a/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c b/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c

> index f2365b5..9a92fea 100644

> --- a/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c

> +++ b/Silicon/Hisilicon/Hi1610/Drivers/PcieInit1610/PcieInitAtu.c

> @@ -96,11 +96,12 @@ SetAtuConfig0RW (

>  {

>    UINTN RbPciBase = Private->RbPciBar;

>    UINT64 MemLimit = GetPcieCfgAddress (Private->Ecam, Private->BusBase + 1, 1, 0, 0) - 1;

> +  UINT64 MemBase = GetPcieCfgAddress (Private->Ecam, Private->BusBase, 0, 0, 0);

>  

>  

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_VIEW_POINT, Index);

> -  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)(Private->Ecam));

> -  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)((UINT64)(Private->Ecam) >> 32));

> +  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)MemBase);

> +  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)(MemBase >> 32));

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LIMIT, (UINT32) MemLimit);

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_LOW, 0);

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_HIGH, 0);

> @@ -124,12 +125,13 @@ SetAtuConfig1RW (

>  {

>    UINTN RbPciBase = Private->RbPciBar;

>    UINT64 MemLimit = GetPcieCfgAddress (Private->Ecam, Private->BusLimit + 1, 0, 0, 0) - 1;

> +  UINT64 MemBase = GetPcieCfgAddress (Private->Ecam, Private->BusBase + 2, 0, 0, 0);

>  

>  

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_VIEW_POINT, Index);

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_CTRL1, IATU_CTRL1_TYPE_CONFIG1);

> -  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)(Private->Ecam));

> -  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)((UINT64)(Private->Ecam) >> 32));

> +  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LOW, (UINT32)MemBase);

> +  MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_HIGH, (UINT32)(MemBase >> 32));

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_BASE_LIMIT, (UINT32) MemLimit);

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_LOW, 0);

>    MmioWrite32 (RbPciBase + IATU_OFFSET + IATU_REGION_TARGET_HIGH, 0);

> -- 

> 2.8.1

> 

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 7, 2018, 11:11 a.m. UTC | #12
On 17 April 2018 at 03:20, Guo Heyi <heyi.guo@linaro.org> wrote:
> Hi Ard,

>

> I tested mm -io on D05, for root bridge 4 with CPU IO address starting from

> 0x8_abff0000, and it worked; both mm -io 0x8abff0000 and mm 0x8abff0000 provided

> the same output. It seems there is no other limit for 64bit IO address after you

> fixed the issue in EFI shell mm command.

>


OK, so I think this is fine after all, even if my uneasy feeling
hasn't gone away :-)

Could you please resend the latest rebased version of the patches?
(and include the ATU fix as well)


> On Mon, Apr 16, 2018 at 09:57:09PM +0800, Guo Heyi wrote:

>> Thanks, I will test mm command and let you know the result.

>>

>> Regards,

>>

>> Heyi

>>

>> On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:

>> > On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

>> > > Hi Ard,

>> > >

>> > > Any comments?

>> > >

>> >

>> > Apologies for the delay. I have been travelling and am behind on email.

>> >

>> > > Anyway we can modify the code if you insist on using an intermediate CPU IO

>> > > address space.

>> > >

>> >

>> > I have not made up my mind yet, to be honest. I agree there is a

>> > certain elegance to merging both translations, but I am concerned that

>> > existing EDK2 code may deal poorly with I/O addresses that require

>> > more than 32 bits to express.

>> >

>> > Did you try the mm command in the shell for instance? As you know, I

>> > recently removed an artificial address range limit there, but I wonder

>> > if it uses 64-bit variables for I/O ports.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo June 22, 2018, 12:58 p.m. UTC | #13
Sure. A little busy these days; I'll do that ASAP.

Thanks,

Heyi

On Thu, Jun 07, 2018 at 01:11:59PM +0200, Ard Biesheuvel wrote:
> On 17 April 2018 at 03:20, Guo Heyi <heyi.guo@linaro.org> wrote:

> > Hi Ard,

> >

> > I tested mm -io on D05, for root bridge 4 with CPU IO address starting from

> > 0x8_abff0000, and it worked; both mm -io 0x8abff0000 and mm 0x8abff0000 provided

> > the same output. It seems there is no other limit for 64bit IO address after you

> > fixed the issue in EFI shell mm command.

> >

> 

> OK, so I think this is fine after all, even if my uneasy feeling

> hasn't gone away :-)

> 

> Could you please resend the latest rebased version of the patches?

> (and include the ATU fix as well)

> 

> 

> > On Mon, Apr 16, 2018 at 09:57:09PM +0800, Guo Heyi wrote:

> >> Thanks, I will test mm command and let you know the result.

> >>

> >> Regards,

> >>

> >> Heyi

> >>

> >> On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:

> >> > On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

> >> > > Hi Ard,

> >> > >

> >> > > Any comments?

> >> > >

> >> >

> >> > Apologies for the delay. I have been travelling and am behind on email.

> >> >

> >> > > Anyway we can modify the code if you insist on using an intermediate CPU IO

> >> > > address space.

> >> > >

> >> >

> >> > I have not made up my mind yet, to be honest. I agree there is a

> >> > certain elegance to merging both translations, but I am concerned that

> >> > existing EDK2 code may deal poorly with I/O addresses that require

> >> > more than 32 bits to express.

> >> >

> >> > Did you try the mm command in the shell for instance? As you know, I

> >> > recently removed an artificial address range limit there, but I wonder

> >> > if it uses 64-bit variables for I/O ports.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 22, 2018, 2:08 p.m. UTC | #14
On 22 June 2018 at 14:58, gary guo <heyi.guo@linaro.org> wrote:
> Sure. A little busy these days; I'll do that ASAP.

>


No worries.

BTW I noticed that we still have a problem with option ROMs when using
the new MMIO translation code. Did you look into that at all?


> On Thu, Jun 07, 2018 at 01:11:59PM +0200, Ard Biesheuvel wrote:

>> On 17 April 2018 at 03:20, Guo Heyi <heyi.guo@linaro.org> wrote:

>> > Hi Ard,

>> >

>> > I tested mm -io on D05, for root bridge 4 with CPU IO address starting from

>> > 0x8_abff0000, and it worked; both mm -io 0x8abff0000 and mm 0x8abff0000 provided

>> > the same output. It seems there is no other limit for 64bit IO address after you

>> > fixed the issue in EFI shell mm command.

>> >

>>

>> OK, so I think this is fine after all, even if my uneasy feeling

>> hasn't gone away :-)

>>

>> Could you please resend the latest rebased version of the patches?

>> (and include the ATU fix as well)

>>

>>

>> > On Mon, Apr 16, 2018 at 09:57:09PM +0800, Guo Heyi wrote:

>> >> Thanks, I will test mm command and let you know the result.

>> >>

>> >> Regards,

>> >>

>> >> Heyi

>> >>

>> >> On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:

>> >> > On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

>> >> > > Hi Ard,

>> >> > >

>> >> > > Any comments?

>> >> > >

>> >> >

>> >> > Apologies for the delay. I have been travelling and am behind on email.

>> >> >

>> >> > > Anyway we can modify the code if you insist on using an intermediate CPU IO

>> >> > > address space.

>> >> > >

>> >> >

>> >> > I have not made up my mind yet, to be honest. I agree there is a

>> >> > certain elegance to merging both translations, but I am concerned that

>> >> > existing EDK2 code may deal poorly with I/O addresses that require

>> >> > more than 32 bits to express.

>> >> >

>> >> > Did you try the mm command in the shell for instance? As you know, I

>> >> > recently removed an artificial address range limit there, but I wonder

>> >> > if it uses 64-bit variables for I/O ports.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 24, 2018, 11:22 a.m. UTC | #15
On 22 June 2018 at 16:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 22 June 2018 at 14:58, gary guo <heyi.guo@linaro.org> wrote:

>> Sure. A little busy these days; I'll do that ASAP.

>>

>

> No worries.

>

> BTW I noticed that we still have a problem with option ROMs when using

> the new MMIO translation code. Did you look into that at all?

>


The option ROM code is fine as long as the PCI address of the MMIO32
region is > 0x0.
Not sure what is going on here, but probably not worth the effort of
digging into.

>

>> On Thu, Jun 07, 2018 at 01:11:59PM +0200, Ard Biesheuvel wrote:

>>> On 17 April 2018 at 03:20, Guo Heyi <heyi.guo@linaro.org> wrote:

>>> > Hi Ard,

>>> >

>>> > I tested mm -io on D05, for root bridge 4 with CPU IO address starting from

>>> > 0x8_abff0000, and it worked; both mm -io 0x8abff0000 and mm 0x8abff0000 provided

>>> > the same output. It seems there is no other limit for 64bit IO address after you

>>> > fixed the issue in EFI shell mm command.

>>> >

>>>

>>> OK, so I think this is fine after all, even if my uneasy feeling

>>> hasn't gone away :-)

>>>

>>> Could you please resend the latest rebased version of the patches?

>>> (and include the ATU fix as well)

>>>

>>>

>>> > On Mon, Apr 16, 2018 at 09:57:09PM +0800, Guo Heyi wrote:

>>> >> Thanks, I will test mm command and let you know the result.

>>> >>

>>> >> Regards,

>>> >>

>>> >> Heyi

>>> >>

>>> >> On Fri, Apr 13, 2018 at 09:19:53AM +0200, Ard Biesheuvel wrote:

>>> >> > On 13 April 2018 at 04:05, Guo Heyi <heyi.guo@linaro.org> wrote:

>>> >> > > Hi Ard,

>>> >> > >

>>> >> > > Any comments?

>>> >> > >

>>> >> >

>>> >> > Apologies for the delay. I have been travelling and am behind on email.

>>> >> >

>>> >> > > Anyway we can modify the code if you insist on using an intermediate CPU IO

>>> >> > > address space.

>>> >> > >

>>> >> >

>>> >> > I have not made up my mind yet, to be honest. I agree there is a

>>> >> > certain elegance to merging both translations, but I am concerned that

>>> >> > existing EDK2 code may deal poorly with I/O addresses that require

>>> >> > more than 32 bits to express.

>>> >> >

>>> >> > Did you try the mm command in the shell for instance? As you know, I

>>> >> > recently removed an artificial address range limit there, but I wonder

>>> >> > if it uses 64-bit variables for I/O ports.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel