diff mbox series

[edk2,edk2-platforms,2/5] Silicon/SynQuacer: tweak PCI I/O windows for ACPI/Linux support

Message ID 20180227092017.23617-3-ard.biesheuvel@linaro.org
State Superseded
Headers show
Series SynQuacer ACPI support | expand

Commit Message

Ard Biesheuvel Feb. 27, 2018, 9:20 a.m. UTC 
ACPI is not able to describe PCI resource windows that involve both type
and address translation (i.e., for I/O windows on architectures that do
not support port I/O natively), and so the ACPI/Linux code has a hard
time performing the resource allocation in such cases. For instance, the
secondary I/O window we implement on SynQuacer:

   I/O  0x10000 ... 0x1ffff -> 0x77f00000

is misinterpreted by Linux, and results in the MMIO range starting at
0x77f10000 to be mapped for I/O port access to this range.

This can be mitigated by using the same PCI range for I/O port access
on both RCs., i.e., 0x0 ... 0xffff. This configuration can be represented
using both DT and ACPI, and will work as expected in Linux, since it only
involves type translation and not address translation.

However, there is a downside: EDK2 does not cope with I/O address
translation in the generic PCI host bridge driver, and so it does
not allow two regions 0x0 ... 0xffff to be configured.

So in addition, let's reduce the windows declared to the UEFI PCI layer
to 0x0 ... 0x7fff and 0x8000 ... 0xffff. This leaves ample room for I/O
BARs (which nobody uses anymore anyway), and allows UEFI and the OS to
share the same static configuration of the PCIe BAR windows.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
Heyi Gui is currently implementing support for address translation in
the generic PCI host bridge driver, so hopefully, limiting the I/O
ranges in UEFI is something we can revert shortly.

 Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                                                |  2 +-
 Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h                                                  | 18 +++++++++++-------
 Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c |  4 ++--
 3 files changed, 14 insertions(+), 10 deletions(-)

-- 
2.11.0

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

Comments

Leif Lindholm Feb. 28, 2018, 4:26 p.m. UTC | #1 
On Tue, Feb 27, 2018 at 09:20:14AM +0000, Ard Biesheuvel wrote:
> ACPI is not able to describe PCI resource windows that involve both type

> and address translation (i.e., for I/O windows on architectures that do

> not support port I/O natively), and so the ACPI/Linux code has a hard

> time performing the resource allocation in such cases. For instance, the

> secondary I/O window we implement on SynQuacer:

> 

>    I/O  0x10000 ... 0x1ffff -> 0x77f00000

> 

> is misinterpreted by Linux, and results in the MMIO range starting at

> 0x77f10000 to be mapped for I/O port access to this range.

> 

> This can be mitigated by using the same PCI range for I/O port access

> on both RCs., i.e., 0x0 ... 0xffff. This configuration can be represented

> using both DT and ACPI, and will work as expected in Linux, since it only

> involves type translation and not address translation.

> 

> However, there is a downside: EDK2 does not cope with I/O address

> translation in the generic PCI host bridge driver, and so it does

> not allow two regions 0x0 ... 0xffff to be configured.

> 

> So in addition, let's reduce the windows declared to the UEFI PCI layer

> to 0x0 ... 0x7fff and 0x8000 ... 0xffff.


", able to cover windows witha single 64KB page."?

> This leaves ample room for I/O

> BARs (which nobody uses anymore anyway), and allows UEFI and the OS to

> share the same static configuration of the PCIe BAR windows.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

> Heyi Gui is currently implementing support for address translation in

> the generic PCI host bridge driver, so hopefully, limiting the I/O

> ranges in UEFI is something we can revert shortly.

> 

>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                                                |  2 +-

>  Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h                                                  | 18 +++++++++++-------

>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c |  4 ++--

>  3 files changed, 14 insertions(+), 10 deletions(-)

> 

> diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> index 05d1673a5c2b..6eb5fd9430cb 100644

> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> @@ -483,7 +483,7 @@

>          bus-range = <0x0 0x7e>;

>          #address-cells = <3>;

>          #size-cells = <2>;

> -        ranges = <0x1000000 0x00 0x00010000 0x00 0x77f00000 0x0 0x00010000>,

> +        ranges = <0x1000000 0x00 0x00000000 0x00 0x77f00000 0x0 0x00010000>,

>                   <0x2000000 0x00 0x78000000 0x00 0x78000000 0x0 0x08000000>,

>                   <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>;

>  

> diff --git a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

> index 2d3d5cd91be0..ee57377ac3be 100644

> --- a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

> +++ b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

> @@ -23,12 +23,12 @@

>  

>  #define SYNQUACER_PCI_SEG0_BUSNUM_MIN       0x0

>  #define SYNQUACER_PCI_SEG0_BUSNUM_MAX       0x7e

> +#define SYNQUACER_PCI_SEG0_BUSNUM_RANGE     0x7f

>  

>  #define SYNQUACER_PCI_SEG0_PORTIO_MIN       0x0

> -#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0xffff

> -#define SYNQUACER_PCI_SEG0_PORTIO_SIZE      0x10000

> +#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0x7fff

>  #define SYNQUACER_PCI_SEG0_PORTIO_MEMBASE   0x67f00000

> -#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG0_PORTIO_SIZE

> +#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   0x10000

>  

>  #define SYNQUACER_PCI_SEG0_MMIO32_MIN       0x68000000

>  #define SYNQUACER_PCI_SEG0_MMIO32_MAX       0x6fffffff

> @@ -45,12 +45,12 @@

>  

>  #define SYNQUACER_PCI_SEG1_BUSNUM_MIN       0x0

>  #define SYNQUACER_PCI_SEG1_BUSNUM_MAX       0x7e

> +#define SYNQUACER_PCI_SEG1_BUSNUM_RANGE     0x7f

>  

> -#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x10000

> -#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0x1ffff

> -#define SYNQUACER_PCI_SEG1_PORTIO_SIZE      0x10000

> +#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x8000

> +#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0xffff

>  #define SYNQUACER_PCI_SEG1_PORTIO_MEMBASE   0x77f00000

> -#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG1_PORTIO_SIZE

> +#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   0x10000

>  

>  #define SYNQUACER_PCI_SEG1_MMIO32_MIN       0x78000000

>  #define SYNQUACER_PCI_SEG1_MMIO32_MAX       0x7fffffff

> @@ -65,4 +65,8 @@

>  #define SYNQUACER_PCI_SLOT1_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 3)

>  #define SYNQUACER_PCI_SLOT2_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 7)

>  

> +#define SYNQUACER_PCI_OS_IO_BASE            0x0

> +#define SYNQUACER_PCI_OS_IO_LIMIT           0xffff

> +#define SYNQUACER_PCI_OS_IO_RANGE           0x10000


Would you be greatly opposed to describing these as

#define SYNQUACER_PCI_OS_IO_BASE   SYNQUACER_PCI_SEG0_PORTIO_MIN
#define SYNQUACER_PCI_OS_IO_LIMIT  SYNQUACER_PCI_SEG1_PORTIO_MAX
#define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE
or
#define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE

which aren't conceptually the same, but explicitly describe the
desired configuration in this case?

/
    Leif

> +

>  #endif

> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> index e4679543cc66..6a3b32f6ca55 100644

> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c

> @@ -348,8 +348,8 @@ PciInitControllerPost (

>    // Region 3: port I/O range

>    ConfigureWindow (DbiBase, 3,

>      IoMemBase,

> -    RootBridge->Io.Base,

> -    RootBridge->Io.Limit - RootBridge->Io.Base + 1,

> +    SYNQUACER_PCI_OS_IO_BASE,

> +    SYNQUACER_PCI_OS_IO_RANGE,

>      IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_IO,

>      0);

>  

> -- 

> 2.11.0

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 28, 2018, 4:30 p.m. UTC | #2 
On 28 February 2018 at 16:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Feb 27, 2018 at 09:20:14AM +0000, Ard Biesheuvel wrote:

>> ACPI is not able to describe PCI resource windows that involve both type

>> and address translation (i.e., for I/O windows on architectures that do

>> not support port I/O natively), and so the ACPI/Linux code has a hard

>> time performing the resource allocation in such cases. For instance, the

>> secondary I/O window we implement on SynQuacer:

>>

>>    I/O  0x10000 ... 0x1ffff -> 0x77f00000

>>

>> is misinterpreted by Linux, and results in the MMIO range starting at

>> 0x77f10000 to be mapped for I/O port access to this range.

>>

>> This can be mitigated by using the same PCI range for I/O port access

>> on both RCs., i.e., 0x0 ... 0xffff. This configuration can be represented

>> using both DT and ACPI, and will work as expected in Linux, since it only

>> involves type translation and not address translation.

>>

>> However, there is a downside: EDK2 does not cope with I/O address

>> translation in the generic PCI host bridge driver, and so it does

>> not allow two regions 0x0 ... 0xffff to be configured.

>>

>> So in addition, let's reduce the windows declared to the UEFI PCI layer

>> to 0x0 ... 0x7fff and 0x8000 ... 0xffff.

>

> ", able to cover windows witha single 64KB page."?

>


??

>> This leaves ample room for I/O

>> BARs (which nobody uses anymore anyway), and allows UEFI and the OS to

>> share the same static configuration of the PCIe BAR windows.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>> Heyi Gui is currently implementing support for address translation in

>> the generic PCI host bridge driver, so hopefully, limiting the I/O

>> ranges in UEFI is something we can revert shortly.

>>

>>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                                                |  2 +-

>>  Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h                                                  | 18 +++++++++++-------

>>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c |  4 ++--

>>  3 files changed, 14 insertions(+), 10 deletions(-)

>>

>> diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> index 05d1673a5c2b..6eb5fd9430cb 100644

>> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> @@ -483,7 +483,7 @@

>>          bus-range = <0x0 0x7e>;

>>          #address-cells = <3>;

>>          #size-cells = <2>;

>> -        ranges = <0x1000000 0x00 0x00010000 0x00 0x77f00000 0x0 0x00010000>,

>> +        ranges = <0x1000000 0x00 0x00000000 0x00 0x77f00000 0x0 0x00010000>,

>>                   <0x2000000 0x00 0x78000000 0x00 0x78000000 0x0 0x08000000>,

>>                   <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>;

>>

>> diff --git a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

>> index 2d3d5cd91be0..ee57377ac3be 100644

>> --- a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

>> +++ b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

>> @@ -23,12 +23,12 @@

>>

>>  #define SYNQUACER_PCI_SEG0_BUSNUM_MIN       0x0

>>  #define SYNQUACER_PCI_SEG0_BUSNUM_MAX       0x7e

>> +#define SYNQUACER_PCI_SEG0_BUSNUM_RANGE     0x7f

>>

>>  #define SYNQUACER_PCI_SEG0_PORTIO_MIN       0x0

>> -#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0xffff

>> -#define SYNQUACER_PCI_SEG0_PORTIO_SIZE      0x10000

>> +#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0x7fff

>>  #define SYNQUACER_PCI_SEG0_PORTIO_MEMBASE   0x67f00000

>> -#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG0_PORTIO_SIZE

>> +#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   0x10000

>>

>>  #define SYNQUACER_PCI_SEG0_MMIO32_MIN       0x68000000

>>  #define SYNQUACER_PCI_SEG0_MMIO32_MAX       0x6fffffff

>> @@ -45,12 +45,12 @@

>>

>>  #define SYNQUACER_PCI_SEG1_BUSNUM_MIN       0x0

>>  #define SYNQUACER_PCI_SEG1_BUSNUM_MAX       0x7e

>> +#define SYNQUACER_PCI_SEG1_BUSNUM_RANGE     0x7f

>>

>> -#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x10000

>> -#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0x1ffff

>> -#define SYNQUACER_PCI_SEG1_PORTIO_SIZE      0x10000

>> +#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x8000

>> +#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0xffff

>>  #define SYNQUACER_PCI_SEG1_PORTIO_MEMBASE   0x77f00000

>> -#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG1_PORTIO_SIZE

>> +#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   0x10000

>>

>>  #define SYNQUACER_PCI_SEG1_MMIO32_MIN       0x78000000

>>  #define SYNQUACER_PCI_SEG1_MMIO32_MAX       0x7fffffff

>> @@ -65,4 +65,8 @@

>>  #define SYNQUACER_PCI_SLOT1_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 3)

>>  #define SYNQUACER_PCI_SLOT2_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 7)

>>

>> +#define SYNQUACER_PCI_OS_IO_BASE            0x0

>> +#define SYNQUACER_PCI_OS_IO_LIMIT           0xffff

>> +#define SYNQUACER_PCI_OS_IO_RANGE           0x10000

>

> Would you be greatly opposed to describing these as

>

> #define SYNQUACER_PCI_OS_IO_BASE   SYNQUACER_PCI_SEG0_PORTIO_MIN

> #define SYNQUACER_PCI_OS_IO_LIMIT  SYNQUACER_PCI_SEG1_PORTIO_MAX

> #define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE

> or

> #define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE

>

> which aren't conceptually the same, but explicitly describe the

> desired configuration in this case?

>


Sure. And as a bonus, these will still be correct after Heyi's
PciHostbridgeDxe patches get merged and we can move to 0x0..0xffff for
both regions.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Feb. 28, 2018, 4:43 p.m. UTC | #3 
On Wed, Feb 28, 2018 at 04:30:50PM +0000, Ard Biesheuvel wrote:
> On 28 February 2018 at 16:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Tue, Feb 27, 2018 at 09:20:14AM +0000, Ard Biesheuvel wrote:

> >> ACPI is not able to describe PCI resource windows that involve both type

> >> and address translation (i.e., for I/O windows on architectures that do

> >> not support port I/O natively), and so the ACPI/Linux code has a hard

> >> time performing the resource allocation in such cases. For instance, the

> >> secondary I/O window we implement on SynQuacer:

> >>

> >>    I/O  0x10000 ... 0x1ffff -> 0x77f00000

> >>

> >> is misinterpreted by Linux, and results in the MMIO range starting at

> >> 0x77f10000 to be mapped for I/O port access to this range.

> >>

> >> This can be mitigated by using the same PCI range for I/O port access

> >> on both RCs., i.e., 0x0 ... 0xffff. This configuration can be represented

> >> using both DT and ACPI, and will work as expected in Linux, since it only

> >> involves type translation and not address translation.

> >>

> >> However, there is a downside: EDK2 does not cope with I/O address

> >> translation in the generic PCI host bridge driver, and so it does

> >> not allow two regions 0x0 ... 0xffff to be configured.

> >>

> >> So in addition, let's reduce the windows declared to the UEFI PCI layer

> >> to 0x0 ... 0x7fff and 0x8000 ... 0xffff.

> >

> > ", able to cover windows witha single 64KB page."?

> 

> ??


Or why was that size picked?

Seems consistent with our "no conflicting mappings within a 64KB
region" rule.

> >> This leaves ample room for I/O

> >> BARs (which nobody uses anymore anyway), and allows UEFI and the OS to

> >> share the same static configuration of the PCIe BAR windows.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.1

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> ---

> >> Heyi Gui is currently implementing support for address translation in

> >> the generic PCI host bridge driver, so hopefully, limiting the I/O

> >> ranges in UEFI is something we can revert shortly.

> >>

> >>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                                                |  2 +-

> >>  Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h                                                  | 18 +++++++++++-------

> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c |  4 ++--

> >>  3 files changed, 14 insertions(+), 10 deletions(-)

> >>

> >> diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> >> index 05d1673a5c2b..6eb5fd9430cb 100644

> >> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> >> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

> >> @@ -483,7 +483,7 @@

> >>          bus-range = <0x0 0x7e>;

> >>          #address-cells = <3>;

> >>          #size-cells = <2>;

> >> -        ranges = <0x1000000 0x00 0x00010000 0x00 0x77f00000 0x0 0x00010000>,

> >> +        ranges = <0x1000000 0x00 0x00000000 0x00 0x77f00000 0x0 0x00010000>,

> >>                   <0x2000000 0x00 0x78000000 0x00 0x78000000 0x0 0x08000000>,

> >>                   <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>;

> >>

> >> diff --git a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

> >> index 2d3d5cd91be0..ee57377ac3be 100644

> >> --- a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

> >> +++ b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

> >> @@ -23,12 +23,12 @@

> >>

> >>  #define SYNQUACER_PCI_SEG0_BUSNUM_MIN       0x0

> >>  #define SYNQUACER_PCI_SEG0_BUSNUM_MAX       0x7e

> >> +#define SYNQUACER_PCI_SEG0_BUSNUM_RANGE     0x7f

> >>

> >>  #define SYNQUACER_PCI_SEG0_PORTIO_MIN       0x0

> >> -#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0xffff

> >> -#define SYNQUACER_PCI_SEG0_PORTIO_SIZE      0x10000

> >> +#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0x7fff

> >>  #define SYNQUACER_PCI_SEG0_PORTIO_MEMBASE   0x67f00000

> >> -#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG0_PORTIO_SIZE

> >> +#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   0x10000

> >>

> >>  #define SYNQUACER_PCI_SEG0_MMIO32_MIN       0x68000000

> >>  #define SYNQUACER_PCI_SEG0_MMIO32_MAX       0x6fffffff

> >> @@ -45,12 +45,12 @@

> >>

> >>  #define SYNQUACER_PCI_SEG1_BUSNUM_MIN       0x0

> >>  #define SYNQUACER_PCI_SEG1_BUSNUM_MAX       0x7e

> >> +#define SYNQUACER_PCI_SEG1_BUSNUM_RANGE     0x7f

> >>

> >> -#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x10000

> >> -#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0x1ffff

> >> -#define SYNQUACER_PCI_SEG1_PORTIO_SIZE      0x10000

> >> +#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x8000

> >> +#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0xffff

> >>  #define SYNQUACER_PCI_SEG1_PORTIO_MEMBASE   0x77f00000

> >> -#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG1_PORTIO_SIZE

> >> +#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   0x10000

> >>

> >>  #define SYNQUACER_PCI_SEG1_MMIO32_MIN       0x78000000

> >>  #define SYNQUACER_PCI_SEG1_MMIO32_MAX       0x7fffffff

> >> @@ -65,4 +65,8 @@

> >>  #define SYNQUACER_PCI_SLOT1_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 3)

> >>  #define SYNQUACER_PCI_SLOT2_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 7)

> >>

> >> +#define SYNQUACER_PCI_OS_IO_BASE            0x0

> >> +#define SYNQUACER_PCI_OS_IO_LIMIT           0xffff

> >> +#define SYNQUACER_PCI_OS_IO_RANGE           0x10000

> >

> > Would you be greatly opposed to describing these as

> >

> > #define SYNQUACER_PCI_OS_IO_BASE   SYNQUACER_PCI_SEG0_PORTIO_MIN

> > #define SYNQUACER_PCI_OS_IO_LIMIT  SYNQUACER_PCI_SEG1_PORTIO_MAX

> > #define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE

> > or

> > #define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE

> >

> > which aren't conceptually the same, but explicitly describe the

> > desired configuration in this case?

> 

> Sure. And as a bonus, these will still be correct after Heyi's

> PciHostbridgeDxe patches get merged and we can move to 0x0..0xffff for

> both regions.


\o/

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 28, 2018, 4:46 p.m. UTC | #4 
On 28 February 2018 at 16:43, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Feb 28, 2018 at 04:30:50PM +0000, Ard Biesheuvel wrote:

>> On 28 February 2018 at 16:26, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>> > On Tue, Feb 27, 2018 at 09:20:14AM +0000, Ard Biesheuvel wrote:

>> >> ACPI is not able to describe PCI resource windows that involve both type

>> >> and address translation (i.e., for I/O windows on architectures that do

>> >> not support port I/O natively), and so the ACPI/Linux code has a hard

>> >> time performing the resource allocation in such cases. For instance, the

>> >> secondary I/O window we implement on SynQuacer:

>> >>

>> >>    I/O  0x10000 ... 0x1ffff -> 0x77f00000

>> >>

>> >> is misinterpreted by Linux, and results in the MMIO range starting at

>> >> 0x77f10000 to be mapped for I/O port access to this range.

>> >>

>> >> This can be mitigated by using the same PCI range for I/O port access

>> >> on both RCs., i.e., 0x0 ... 0xffff. This configuration can be represented

>> >> using both DT and ACPI, and will work as expected in Linux, since it only

>> >> involves type translation and not address translation.

>> >>

>> >> However, there is a downside: EDK2 does not cope with I/O address

>> >> translation in the generic PCI host bridge driver, and so it does

>> >> not allow two regions 0x0 ... 0xffff to be configured.

>> >>

>> >> So in addition, let's reduce the windows declared to the UEFI PCI layer

>> >> to 0x0 ... 0x7fff and 0x8000 ... 0xffff.

>> >

>> > ", able to cover windows witha single 64KB page."?

>>

>> ??

>

> Or why was that size picked?

>

> Seems consistent with our "no conflicting mappings within a 64KB

> region" rule.

>


Not really. I think 16 bits of I/O space is rather common, but we
could also increase both I/O windows to 0x0 .. 0x1ffff, and split them
into 0x0..0xffff and 0x10000..0x1ffff on the UEFI side. Also, they
don't even have to be adjacent, as long as we use a 1:1 mapped slice
for each.

>> >> This leaves ample room for I/O

>> >> BARs (which nobody uses anymore anyway), and allows UEFI and the OS to

>> >> share the same static configuration of the PCIe BAR windows.

>> >>

>> >> Contributed-under: TianoCore Contribution Agreement 1.1

>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> >> ---

>> >> Heyi Gui is currently implementing support for address translation in

>> >> the generic PCI host bridge driver, so hopefully, limiting the I/O

>> >> ranges in UEFI is something we can revert shortly.

>> >>

>> >>  Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi                                                |  2 +-

>> >>  Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h                                                  | 18 +++++++++++-------

>> >>  Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c |  4 ++--

>> >>  3 files changed, 14 insertions(+), 10 deletions(-)

>> >>

>> >> diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> >> index 05d1673a5c2b..6eb5fd9430cb 100644

>> >> --- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> >> +++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi

>> >> @@ -483,7 +483,7 @@

>> >>          bus-range = <0x0 0x7e>;

>> >>          #address-cells = <3>;

>> >>          #size-cells = <2>;

>> >> -        ranges = <0x1000000 0x00 0x00010000 0x00 0x77f00000 0x0 0x00010000>,

>> >> +        ranges = <0x1000000 0x00 0x00000000 0x00 0x77f00000 0x0 0x00010000>,

>> >>                   <0x2000000 0x00 0x78000000 0x00 0x78000000 0x0 0x08000000>,

>> >>                   <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>;

>> >>

>> >> diff --git a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

>> >> index 2d3d5cd91be0..ee57377ac3be 100644

>> >> --- a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

>> >> +++ b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h

>> >> @@ -23,12 +23,12 @@

>> >>

>> >>  #define SYNQUACER_PCI_SEG0_BUSNUM_MIN       0x0

>> >>  #define SYNQUACER_PCI_SEG0_BUSNUM_MAX       0x7e

>> >> +#define SYNQUACER_PCI_SEG0_BUSNUM_RANGE     0x7f

>> >>

>> >>  #define SYNQUACER_PCI_SEG0_PORTIO_MIN       0x0

>> >> -#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0xffff

>> >> -#define SYNQUACER_PCI_SEG0_PORTIO_SIZE      0x10000

>> >> +#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0x7fff

>> >>  #define SYNQUACER_PCI_SEG0_PORTIO_MEMBASE   0x67f00000

>> >> -#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG0_PORTIO_SIZE

>> >> +#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   0x10000

>> >>

>> >>  #define SYNQUACER_PCI_SEG0_MMIO32_MIN       0x68000000

>> >>  #define SYNQUACER_PCI_SEG0_MMIO32_MAX       0x6fffffff

>> >> @@ -45,12 +45,12 @@

>> >>

>> >>  #define SYNQUACER_PCI_SEG1_BUSNUM_MIN       0x0

>> >>  #define SYNQUACER_PCI_SEG1_BUSNUM_MAX       0x7e

>> >> +#define SYNQUACER_PCI_SEG1_BUSNUM_RANGE     0x7f

>> >>

>> >> -#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x10000

>> >> -#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0x1ffff

>> >> -#define SYNQUACER_PCI_SEG1_PORTIO_SIZE      0x10000

>> >> +#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x8000

>> >> +#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0xffff

>> >>  #define SYNQUACER_PCI_SEG1_PORTIO_MEMBASE   0x77f00000

>> >> -#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG1_PORTIO_SIZE

>> >> +#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   0x10000

>> >>

>> >>  #define SYNQUACER_PCI_SEG1_MMIO32_MIN       0x78000000

>> >>  #define SYNQUACER_PCI_SEG1_MMIO32_MAX       0x7fffffff

>> >> @@ -65,4 +65,8 @@

>> >>  #define SYNQUACER_PCI_SLOT1_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 3)

>> >>  #define SYNQUACER_PCI_SLOT2_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 7)

>> >>

>> >> +#define SYNQUACER_PCI_OS_IO_BASE            0x0

>> >> +#define SYNQUACER_PCI_OS_IO_LIMIT           0xffff

>> >> +#define SYNQUACER_PCI_OS_IO_RANGE           0x10000

>> >

>> > Would you be greatly opposed to describing these as

>> >

>> > #define SYNQUACER_PCI_OS_IO_BASE   SYNQUACER_PCI_SEG0_PORTIO_MIN

>> > #define SYNQUACER_PCI_OS_IO_LIMIT  SYNQUACER_PCI_SEG1_PORTIO_MAX

>> > #define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE

>> > or

>> > #define SYNQUACER_PCI_OS_IO_RANGE  SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE

>> >

>> > which aren't conceptually the same, but explicitly describe the

>> > desired configuration in this case?

>>

>> Sure. And as a bonus, these will still be correct after Heyi's

>> PciHostbridgeDxe patches get merged and we can move to 0x0..0xffff for

>> both regions.

>

> \o/

>

> /

>     Leif

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

Patch

diff --git a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
index 05d1673a5c2b..6eb5fd9430cb 100644
--- a/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
+++ b/Silicon/Socionext/SynQuacer/DeviceTree/SynQuacer.dtsi
@@ -483,7 +483,7 @@ 
         bus-range = <0x0 0x7e>;
         #address-cells = <3>;
         #size-cells = <2>;
-        ranges = <0x1000000 0x00 0x00010000 0x00 0x77f00000 0x0 0x00010000>,
+        ranges = <0x1000000 0x00 0x00000000 0x00 0x77f00000 0x0 0x00010000>,
                  <0x2000000 0x00 0x78000000 0x00 0x78000000 0x0 0x08000000>,
                  <0x3000000 0x3f 0x00000000 0x3f 0x00000000 0x1 0x00000000>;
 
diff --git a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h
index 2d3d5cd91be0..ee57377ac3be 100644
--- a/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h
+++ b/Silicon/Socionext/SynQuacer/Include/Platform/Pcie.h
@@ -23,12 +23,12 @@ 
 
 #define SYNQUACER_PCI_SEG0_BUSNUM_MIN       0x0
 #define SYNQUACER_PCI_SEG0_BUSNUM_MAX       0x7e
+#define SYNQUACER_PCI_SEG0_BUSNUM_RANGE     0x7f
 
 #define SYNQUACER_PCI_SEG0_PORTIO_MIN       0x0
-#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0xffff
-#define SYNQUACER_PCI_SEG0_PORTIO_SIZE      0x10000
+#define SYNQUACER_PCI_SEG0_PORTIO_MAX       0x7fff
 #define SYNQUACER_PCI_SEG0_PORTIO_MEMBASE   0x67f00000
-#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG0_PORTIO_SIZE
+#define SYNQUACER_PCI_SEG0_PORTIO_MEMSIZE   0x10000
 
 #define SYNQUACER_PCI_SEG0_MMIO32_MIN       0x68000000
 #define SYNQUACER_PCI_SEG0_MMIO32_MAX       0x6fffffff
@@ -45,12 +45,12 @@ 
 
 #define SYNQUACER_PCI_SEG1_BUSNUM_MIN       0x0
 #define SYNQUACER_PCI_SEG1_BUSNUM_MAX       0x7e
+#define SYNQUACER_PCI_SEG1_BUSNUM_RANGE     0x7f
 
-#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x10000
-#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0x1ffff
-#define SYNQUACER_PCI_SEG1_PORTIO_SIZE      0x10000
+#define SYNQUACER_PCI_SEG1_PORTIO_MIN       0x8000
+#define SYNQUACER_PCI_SEG1_PORTIO_MAX       0xffff
 #define SYNQUACER_PCI_SEG1_PORTIO_MEMBASE   0x77f00000
-#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   SYNQUACER_PCI_SEG1_PORTIO_SIZE
+#define SYNQUACER_PCI_SEG1_PORTIO_MEMSIZE   0x10000
 
 #define SYNQUACER_PCI_SEG1_MMIO32_MIN       0x78000000
 #define SYNQUACER_PCI_SEG1_MMIO32_MAX       0x7fffffff
@@ -65,4 +65,8 @@ 
 #define SYNQUACER_PCI_SLOT1_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 3)
 #define SYNQUACER_PCI_SLOT2_LOCATION        SYNQUACER_PCI_LOCATION(0, 1, 7)
 
+#define SYNQUACER_PCI_OS_IO_BASE            0x0
+#define SYNQUACER_PCI_OS_IO_LIMIT           0xffff
+#define SYNQUACER_PCI_OS_IO_RANGE           0x10000
+
 #endif
diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c
index e4679543cc66..6a3b32f6ca55 100644
--- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c
+++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPciHostBridgeLib/SynQuacerPciHostBridgeLibConstructor.c
@@ -348,8 +348,8 @@  PciInitControllerPost (
   // Region 3: port I/O range
   ConfigureWindow (DbiBase, 3,
     IoMemBase,
-    RootBridge->Io.Base,
-    RootBridge->Io.Limit - RootBridge->Io.Base + 1,
+    SYNQUACER_PCI_OS_IO_BASE,
+    SYNQUACER_PCI_OS_IO_RANGE,
     IATU_REGION_CTRL_1_OFF_OUTBOUND_0_TYPE_IO,
     0);