diff mbox series

[v2] riscv: enable multi-range memory layout

Message ID 20230914053055.745507-1-fei2.wu@intel.com
State Deferred
Delegated to: Tom Rini
Headers show
Series [v2] riscv: enable multi-range memory layout | expand

Commit Message

Wu, Fei Sept. 14, 2023, 5:30 a.m. UTC
In order to enable PCIe passthrough on qemu riscv, the physical memory
range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
ram_top to 4G - 1 if the gd->ram_top is above 4G in
board_get_usable_ram_top(), but that address is not backed by ram. This
patch selects the lowest range instead.

Signed-off-by: Fei Wu <fei2.wu@intel.com>
---
 arch/riscv/cpu/generic/dram.c        | 2 +-
 configs/qemu-riscv64_defconfig       | 2 +-
 configs/qemu-riscv64_smode_defconfig | 2 +-
 configs/qemu-riscv64_spl_defconfig   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

Comments

Heinrich Schuchardt Sept. 14, 2023, 6:05 a.m. UTC | #1
Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>In order to enable PCIe passthrough on qemu riscv, the physical memory
>range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>ram_top to 4G - 1 if the gd->ram_top is above 4G in

This should move to 7GiB - 1 in your example on riscv64.

>board_get_usable_ram_top(), but that address is not backed by ram. This
>patch selects the lowest range instead.
>
>Signed-off-by: Fei Wu <fei2.wu@intel.com>
>---
> arch/riscv/cpu/generic/dram.c        | 2 +-
> configs/qemu-riscv64_defconfig       | 2 +-
> configs/qemu-riscv64_smode_defconfig | 2 +-
> configs/qemu-riscv64_spl_defconfig   | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
>index 44e11bd56c..fb53a57b4e 100644
>--- a/arch/riscv/cpu/generic/dram.c
>+++ b/arch/riscv/cpu/generic/dram.c
>@@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
> 
> int dram_init(void)
> {
>-	return fdtdec_setup_mem_size_base();
>+	return fdtdec_setup_mem_size_base_lowest();

Linaro is working on allowing to download a distro image via https and installing from a RAM disk.

We should not artificially reduce the RAM size available for U-Boot by restricting ourselfs to 1 GiB.

We must ensure that ram top is in the upper range.

Best regards

Heinrich

> }
> 
> int dram_init_banksize(void)
>diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
>index 9a8bbef192..aa55317d26 100644
>--- a/configs/qemu-riscv64_defconfig
>+++ b/configs/qemu-riscv64_defconfig
>@@ -1,6 +1,6 @@
> CONFIG_RISCV=y
> CONFIG_SYS_MALLOC_LEN=0x800000
>-CONFIG_NR_DRAM_BANKS=1
>+CONFIG_NR_DRAM_BANKS=2
> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
> CONFIG_ENV_SIZE=0x20000
>diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
>index 1d0f021ade..de08a49dab 100644
>--- a/configs/qemu-riscv64_smode_defconfig
>+++ b/configs/qemu-riscv64_smode_defconfig
>@@ -1,6 +1,6 @@
> CONFIG_RISCV=y
> CONFIG_SYS_MALLOC_LEN=0x800000
>-CONFIG_NR_DRAM_BANKS=1
>+CONFIG_NR_DRAM_BANKS=2
> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
> CONFIG_ENV_SIZE=0x20000
>diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
>index bb10145e6e..66dc2a1dd9 100644
>--- a/configs/qemu-riscv64_spl_defconfig
>+++ b/configs/qemu-riscv64_spl_defconfig
>@@ -1,6 +1,6 @@
> CONFIG_RISCV=y
> CONFIG_SYS_MALLOC_LEN=0x800000
>-CONFIG_NR_DRAM_BANKS=1
>+CONFIG_NR_DRAM_BANKS=2
> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
> CONFIG_ENV_SIZE=0x20000
Wu, Fei Sept. 14, 2023, 6:48 a.m. UTC | #2
On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
> 
> 
> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
> 
> This should move to 7GiB - 1 in your example on riscv64.
> 
I'm describing the current implementation of board_get_usable_ram_top()
in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
changed? Is the comment about 32bit DMA device still valid?

phys_size_t board_get_usable_ram_top(phys_size_t total_size)
{
        /*
         * Ensure that we run from first 4GB so that all
         * addresses used by U-Boot are 32bit addresses.
         *
         * This in-turn ensures that 32bit DMA capable
         * devices work fine because DMA mapping APIs will
         * provide 32bit DMA addresses only.
         */
        if (gd->ram_top >= SZ_4G)
                return SZ_4G - 1;

        return gd->ram_top;
}

>> board_get_usable_ram_top(), but that address is not backed by ram. This
>> patch selects the lowest range instead.
>>
>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>> ---
>> arch/riscv/cpu/generic/dram.c        | 2 +-
>> configs/qemu-riscv64_defconfig       | 2 +-
>> configs/qemu-riscv64_smode_defconfig | 2 +-
>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
>> index 44e11bd56c..fb53a57b4e 100644
>> --- a/arch/riscv/cpu/generic/dram.c
>> +++ b/arch/riscv/cpu/generic/dram.c
>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>> int dram_init(void)
>> {
>> -	return fdtdec_setup_mem_size_base();
>> +	return fdtdec_setup_mem_size_base_lowest();
> 
> Linaro is working on allowing to download a distro image via https and installing from a RAM disk.
> 
> We should not artificially reduce the RAM size available for U-Boot by restricting ourselfs to 1 GiB.
> 
> We must ensure that ram top is in the upper range.
> 
How do they handle the case of >4GB? board_get_usable_ram_top() attached
above always returns <4GB. And it seems like
fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
just picks up one, but which one is selected depending on fdt.

Thanks,
Fei.

> Best regards
> 
> Heinrich
> 
>> }
>>
>> int dram_init_banksize(void)
>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
>> index 9a8bbef192..aa55317d26 100644
>> --- a/configs/qemu-riscv64_defconfig
>> +++ b/configs/qemu-riscv64_defconfig
>> @@ -1,6 +1,6 @@
>> CONFIG_RISCV=y
>> CONFIG_SYS_MALLOC_LEN=0x800000
>> -CONFIG_NR_DRAM_BANKS=1
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>> CONFIG_ENV_SIZE=0x20000
>> diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
>> index 1d0f021ade..de08a49dab 100644
>> --- a/configs/qemu-riscv64_smode_defconfig
>> +++ b/configs/qemu-riscv64_smode_defconfig
>> @@ -1,6 +1,6 @@
>> CONFIG_RISCV=y
>> CONFIG_SYS_MALLOC_LEN=0x800000
>> -CONFIG_NR_DRAM_BANKS=1
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>> CONFIG_ENV_SIZE=0x20000
>> diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
>> index bb10145e6e..66dc2a1dd9 100644
>> --- a/configs/qemu-riscv64_spl_defconfig
>> +++ b/configs/qemu-riscv64_spl_defconfig
>> @@ -1,6 +1,6 @@
>> CONFIG_RISCV=y
>> CONFIG_SYS_MALLOC_LEN=0x800000
>> -CONFIG_NR_DRAM_BANKS=1
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>> CONFIG_ENV_SIZE=0x20000
Wu, Fei Sept. 14, 2023, 7:17 a.m. UTC | #3
On 9/14/2023 2:48 PM, Wu, Fei wrote:
> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>
>>
>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>
>> This should move to 7GiB - 1 in your example on riscv64.
>>
> I'm describing the current implementation of board_get_usable_ram_top()
> in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
> changed? Is the comment about 32bit DMA device still valid?
> 
> phys_size_t board_get_usable_ram_top(phys_size_t total_size)
> {
>         /*
>          * Ensure that we run from first 4GB so that all
>          * addresses used by U-Boot are 32bit addresses.
>          *
>          * This in-turn ensures that 32bit DMA capable
>          * devices work fine because DMA mapping APIs will
>          * provide 32bit DMA addresses only.
>          */
>         if (gd->ram_top >= SZ_4G)
>                 return SZ_4G - 1;
> 
>         return gd->ram_top;
> }
> 
>>> board_get_usable_ram_top(), but that address is not backed by ram. This
>>> patch selects the lowest range instead.
>>>
>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>> ---
>>> arch/riscv/cpu/generic/dram.c        | 2 +-
>>> configs/qemu-riscv64_defconfig       | 2 +-
>>> configs/qemu-riscv64_smode_defconfig | 2 +-
>>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
>>> index 44e11bd56c..fb53a57b4e 100644
>>> --- a/arch/riscv/cpu/generic/dram.c
>>> +++ b/arch/riscv/cpu/generic/dram.c
>>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>
>>> int dram_init(void)
>>> {
>>> -	return fdtdec_setup_mem_size_base();
>>> +	return fdtdec_setup_mem_size_base_lowest();
>>
>> Linaro is working on allowing to download a distro image via https and installing from a RAM disk.
>>
>> We should not artificially reduce the RAM size available for U-Boot by restricting ourselfs to 1 GiB.
>>
>> We must ensure that ram top is in the upper range.
>>
> How do they handle the case of >4GB? board_get_usable_ram_top() attached
> above always returns <4GB. And it seems like
> fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
> just picks up one, but which one is selected depending on fdt.
> 
The whole idea of this change is to find an address with backed ram,
because of the current implementation (or limitation) of
board_get_usable_ram_top(), the lowest range is returned as a solution.
If the Linaro work can remove that limitation, likely we are both fine.
The next question is when will this Linaro work be upstreamed. I'm
changing the memory layout for PCIe passthrough on QEMU, it's better to
see u-boot is ready before QEMU change, or the system is not able to
boot up.

Thanks,
Fei.

> Thanks,
> Fei.
> 
>> Best regards
>>
>> Heinrich
>>
>>> }
>>>
>>> int dram_init_banksize(void)
>>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
>>> index 9a8bbef192..aa55317d26 100644
>>> --- a/configs/qemu-riscv64_defconfig
>>> +++ b/configs/qemu-riscv64_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>>> diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
>>> index 1d0f021ade..de08a49dab 100644
>>> --- a/configs/qemu-riscv64_smode_defconfig
>>> +++ b/configs/qemu-riscv64_smode_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>>> diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
>>> index bb10145e6e..66dc2a1dd9 100644
>>> --- a/configs/qemu-riscv64_spl_defconfig
>>> +++ b/configs/qemu-riscv64_spl_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>
Heinrich Schuchardt Sept. 14, 2023, 7:20 a.m. UTC | #4
On 9/14/23 08:48, Wu, Fei wrote:
> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>
>>
>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>
>> This should move to 7GiB - 1 in your example on riscv64.
>>
> I'm describing the current implementation of board_get_usable_ram_top()
> in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
> changed? Is the comment about 32bit DMA device still valid?
>
> phys_size_t board_get_usable_ram_top(phys_size_t total_size)
> {
>          /*
>           * Ensure that we run from first 4GB so that all
>           * addresses used by U-Boot are 32bit addresses.
>           *
>           * This in-turn ensures that 32bit DMA capable
>           * devices work fine because DMA mapping APIs will
>           * provide 32bit DMA addresses only.
>           */
>          if (gd->ram_top >= SZ_4G)
>                  return SZ_4G - 1;
>
>          return gd->ram_top;
> }

The comment above says 32bit DMA is board specific and not architecture
specific. So it is wrong to have this board_get_usable_ram_top()
function on architecture level. It makes usage of devices with all
memory above 4 GiB impossible.

It tried to pass through a SATA controller but received an error:

# modprobe vfio-pci
# echo 0000:06:00.0 > /sys/bus/pci/drivers/ahci/unbind
# echo 1022 7091 > /sys/bus/pci/drivers/vfio-pci/new_id
# qemu-system-riscv64 -kernel u-boot.bin -nographic -M virt -m 4G
-device vfio-pci,host=0000:06:00.0
qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: VFIO_MAP_DMA
failed: Invalid argument
qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: vfio
0000:06:00.0: failed to setup container for group 5: memory listener
initialization failed: Region riscv_virt_board.ram:
vfio_dma_map(0x55adbde66f70, 0x80000000, 0x100000000, 0x7fcd6fe00000) =
-22 (Invalid argument)

With which version of QEMU were you able to use PCI pass through?

Best regards

Heinrich

>
>>> board_get_usable_ram_top(), but that address is not backed by ram. This
>>> patch selects the lowest range instead.
>>>
>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>> ---
>>> arch/riscv/cpu/generic/dram.c        | 2 +-
>>> configs/qemu-riscv64_defconfig       | 2 +-
>>> configs/qemu-riscv64_smode_defconfig | 2 +-
>>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
>>> index 44e11bd56c..fb53a57b4e 100644
>>> --- a/arch/riscv/cpu/generic/dram.c
>>> +++ b/arch/riscv/cpu/generic/dram.c
>>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>
>>> int dram_init(void)
>>> {
>>> -	return fdtdec_setup_mem_size_base();
>>> +	return fdtdec_setup_mem_size_base_lowest();
>>
>> Linaro is working on allowing to download a distro image via https and installing from a RAM disk.
>>
>> We should not artificially reduce the RAM size available for U-Boot by restricting ourselfs to 1 GiB.
>>
>> We must ensure that ram top is in the upper range.
>>
> How do they handle the case of >4GB? board_get_usable_ram_top() attached
> above always returns <4GB. And it seems like
> fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
> just picks up one, but which one is selected depending on fdt.
>
> Thanks,
> Fei.
>
>> Best regards
>>
>> Heinrich
>>
>>> }
>>>
>>> int dram_init_banksize(void)
>>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
>>> index 9a8bbef192..aa55317d26 100644
>>> --- a/configs/qemu-riscv64_defconfig
>>> +++ b/configs/qemu-riscv64_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>>> diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
>>> index 1d0f021ade..de08a49dab 100644
>>> --- a/configs/qemu-riscv64_smode_defconfig
>>> +++ b/configs/qemu-riscv64_smode_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>>> diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
>>> index bb10145e6e..66dc2a1dd9 100644
>>> --- a/configs/qemu-riscv64_spl_defconfig
>>> +++ b/configs/qemu-riscv64_spl_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>
Wu, Fei Sept. 14, 2023, 7:42 a.m. UTC | #5
On 9/14/2023 3:20 PM, Heinrich Schuchardt wrote:
> On 9/14/23 08:48, Wu, Fei wrote:
>> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>>
>>>
>>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>>>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>>
>>> This should move to 7GiB - 1 in your example on riscv64.
>>>
>> I'm describing the current implementation of board_get_usable_ram_top()
>> in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
>> changed? Is the comment about 32bit DMA device still valid?
>>
>> phys_size_t board_get_usable_ram_top(phys_size_t total_size)
>> {
>>          /*
>>           * Ensure that we run from first 4GB so that all
>>           * addresses used by U-Boot are 32bit addresses.
>>           *
>>           * This in-turn ensures that 32bit DMA capable
>>           * devices work fine because DMA mapping APIs will
>>           * provide 32bit DMA addresses only.
>>           */
>>          if (gd->ram_top >= SZ_4G)
>>                  return SZ_4G - 1;
>>
>>          return gd->ram_top;
>> }
> 
> The comment above says 32bit DMA is board specific and not architecture
> specific. So it is wrong to have this board_get_usable_ram_top()
> function on architecture level. It makes usage of devices with all
> memory above 4 GiB impossible.
> 
> It tried to pass through a SATA controller but received an error:
> 
> # modprobe vfio-pci
> # echo 0000:06:00.0 > /sys/bus/pci/drivers/ahci/unbind
> # echo 1022 7091 > /sys/bus/pci/drivers/vfio-pci/new_id
> # qemu-system-riscv64 -kernel u-boot.bin -nographic -M virt -m 4G
> -device vfio-pci,host=0000:06:00.0
> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: VFIO_MAP_DMA
> failed: Invalid argument
> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: vfio
> 0000:06:00.0: failed to setup container for group 5: memory listener
> initialization failed: Region riscv_virt_board.ram:
> vfio_dma_map(0x55adbde66f70, 0x80000000, 0x100000000, 0x7fcd6fe00000) =
> -22 (Invalid argument)
> 
> With which version of QEMU were you able to use PCI pass through?
> 
Please try this:

base: ccb86f079a9e
patch:
https://lore.kernel.org/all/CAKmqyKMtAzt5saCUMd4vXYfgAQibpzQJAhtTSuSb+yeKhcYpfw@mail.gmail.com/T/

Thanks,
Fei.

> Best regards
> 
> Heinrich
> 
>>
>>>> board_get_usable_ram_top(), but that address is not backed by ram. This
>>>> patch selects the lowest range instead.
>>>>
>>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>>> ---
>>>> arch/riscv/cpu/generic/dram.c        | 2 +-
>>>> configs/qemu-riscv64_defconfig       | 2 +-
>>>> configs/qemu-riscv64_smode_defconfig | 2 +-
>>>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>>>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/cpu/generic/dram.c
>>>> b/arch/riscv/cpu/generic/dram.c
>>>> index 44e11bd56c..fb53a57b4e 100644
>>>> --- a/arch/riscv/cpu/generic/dram.c
>>>> +++ b/arch/riscv/cpu/generic/dram.c
>>>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> int dram_init(void)
>>>> {
>>>> -    return fdtdec_setup_mem_size_base();
>>>> +    return fdtdec_setup_mem_size_base_lowest();
>>>
>>> Linaro is working on allowing to download a distro image via https
>>> and installing from a RAM disk.
>>>
>>> We should not artificially reduce the RAM size available for U-Boot
>>> by restricting ourselfs to 1 GiB.
>>>
>>> We must ensure that ram top is in the upper range.
>>>
>> How do they handle the case of >4GB? board_get_usable_ram_top() attached
>> above always returns <4GB. And it seems like
>> fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
>> just picks up one, but which one is selected depending on fdt.
>>
>> Thanks,
>> Fei.
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> }
>>>>
>>>> int dram_init_banksize(void)
>>>> diff --git a/configs/qemu-riscv64_defconfig
>>>> b/configs/qemu-riscv64_defconfig
>>>> index 9a8bbef192..aa55317d26 100644
>>>> --- a/configs/qemu-riscv64_defconfig
>>>> +++ b/configs/qemu-riscv64_defconfig
>>>> @@ -1,6 +1,6 @@
>>>> CONFIG_RISCV=y
>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>> -CONFIG_NR_DRAM_BANKS=1
>>>> +CONFIG_NR_DRAM_BANKS=2
>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>> CONFIG_ENV_SIZE=0x20000
>>>> diff --git a/configs/qemu-riscv64_smode_defconfig
>>>> b/configs/qemu-riscv64_smode_defconfig
>>>> index 1d0f021ade..de08a49dab 100644
>>>> --- a/configs/qemu-riscv64_smode_defconfig
>>>> +++ b/configs/qemu-riscv64_smode_defconfig
>>>> @@ -1,6 +1,6 @@
>>>> CONFIG_RISCV=y
>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>> -CONFIG_NR_DRAM_BANKS=1
>>>> +CONFIG_NR_DRAM_BANKS=2
>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>> CONFIG_ENV_SIZE=0x20000
>>>> diff --git a/configs/qemu-riscv64_spl_defconfig
>>>> b/configs/qemu-riscv64_spl_defconfig
>>>> index bb10145e6e..66dc2a1dd9 100644
>>>> --- a/configs/qemu-riscv64_spl_defconfig
>>>> +++ b/configs/qemu-riscv64_spl_defconfig
>>>> @@ -1,6 +1,6 @@
>>>> CONFIG_RISCV=y
>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>> -CONFIG_NR_DRAM_BANKS=1
>>>> +CONFIG_NR_DRAM_BANKS=2
>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>> CONFIG_ENV_SIZE=0x20000
>>
>
Heinrich Schuchardt Sept. 14, 2023, 10:21 a.m. UTC | #6
On 9/14/23 09:42, Wu, Fei wrote:
> On 9/14/2023 3:20 PM, Heinrich Schuchardt wrote:
>> On 9/14/23 08:48, Wu, Fei wrote:
>>> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>>>
>>>>
>>>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>>>>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>>>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>>>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>>>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>>>
>>>> This should move to 7GiB - 1 in your example on riscv64.
>>>>
>>> I'm describing the current implementation of board_get_usable_ram_top()
>>> in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
>>> changed? Is the comment about 32bit DMA device still valid?
>>>
>>> phys_size_t board_get_usable_ram_top(phys_size_t total_size)
>>> {
>>>           /*
>>>            * Ensure that we run from first 4GB so that all
>>>            * addresses used by U-Boot are 32bit addresses.
>>>            *
>>>            * This in-turn ensures that 32bit DMA capablehe
>>>            * devices work fine because DMA mapping APIs will
>>>            * provide 32bit DMA addresses only.
>>>            */
>>>           if (gd->ram_top >= SZ_4G)
>>>                   return SZ_4G - 1;
>>>
>>>           return gd->ram_top;
>>> }
>>
>> The comment above says 32bit DMA is board specific and not architecture
>> specific. So it is wrong to have this board_get_usable_ram_top()
>> function on architecture level. It makes usage of devices with all
>> memory above 4 GiB impossible.

@Anup:

The generic code was introduced by your patch 26f4fd1cb4f6 ("riscv:
generic: Ensure that U-Boot runs within 4GB for 64bit systems").

Which devices have that 32bit DMA issue?

fu540, fu740, jh7110 already have a cpu specific implementation and
don't need the generic one.

>>
>> It tried to pass through a SATA controller but received an error:
>>
>> # modprobe vfio-pci
>> # echo 0000:06:00.0 > /sys/bus/pci/drivers/ahci/unbind
>> # echo 1022 7091 > /sys/bus/pci/drivers/vfio-pci/new_id
>> # qemu-system-riscv64 -kernel u-boot.bin -nographic -M virt -m 4G
>> -device vfio-pci,host=0000:06:00.0
>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: VFIO_MAP_DMA
>> failed: Invalid argument
>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: vfio
>> 0000:06:00.0: failed to setup container for group 5: memory listener
>> initialization failed: Region riscv_virt_board.ram:
>> vfio_dma_map(0x55adbde66f70, 0x80000000, 0x100000000, 0x7fcd6fe00000) =
>> -22 (Invalid argument)
>>
>> With which version of QEMU were you able to use PCI pass through?
>>
> Please try this:
>
> base: ccb86f079a9e
> patch:
> https://lore.kernel.org/all/CAKmqyKMtAzt5saCUMd4vXYfgAQibpzQJAhtTSuSb+yeKhcYpfw@mail.gmail.com/T/

@Fei

With that patch I see the two memory banks in QEMU but when I attach a
vfio-pci device my whole system crashes and becomes unresponsive but
that is not a U-Boot issue.

Best regards

Heinrich

>
> Thanks,
> Fei.
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>> board_get_usable_ram_top(), but that address is not backed by ram. This
>>>>> patch selects the lowest range instead.
>>>>>
>>>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>>>> ---
>>>>> arch/riscv/cpu/generic/dram.c        | 2 +-
>>>>> configs/qemu-riscv64_defconfig       | 2 +-
>>>>> configs/qemu-riscv64_smode_defconfig | 2 +-
>>>>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>>>>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/cpu/generic/dram.c
>>>>> b/arch/riscv/cpu/generic/dram.c
>>>>> index 44e11bd56c..fb53a57b4e 100644
>>>>> --- a/arch/riscv/cpu/generic/dram.c
>>>>> +++ b/arch/riscv/cpu/generic/dram.c
>>>>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>
>>>>> int dram_init(void)
>>>>> {
>>>>> -    return fdtdec_setup_mem_size_base();
>>>>> +    return fdtdec_setup_mem_size_base_lowest();
>>>>
>>>> Linaro is working on allowing to download a distro image via https
>>>> and installing from a RAM disk.
>>>>
>>>> We should not artificially reduce the RAM size available for U-Boot
>>>> by restricting ourselfs to 1 GiB.
>>>>
>>>> We must ensure that ram top is in the upper range.
>>>>
>>> How do they handle the case of >4GB? board_get_usable_ram_top() attached
>>> above always returns <4GB. And it seems like
>>> fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
>>> just picks up one, but which one is selected depending on fdt.
>>>
>>> Thanks,
>>> Fei.
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> }
>>>>>
>>>>> int dram_init_banksize(void)
>>>>> diff --git a/configs/qemu-riscv64_defconfig
>>>>> b/configs/qemu-riscv64_defconfig
>>>>> index 9a8bbef192..aa55317d26 100644
>>>>> --- a/configs/qemu-riscv64_defconfig
>>>>> +++ b/configs/qemu-riscv64_defconfig
>>>>> @@ -1,6 +1,6 @@
>>>>> CONFIG_RISCV=y
>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>> CONFIG_ENV_SIZE=0x20000
>>>>> diff --git a/configs/qemu-riscv64_smode_defconfig
>>>>> b/configs/qemu-riscv64_smode_defconfig
>>>>> index 1d0f021ade..de08a49dab 100644
>>>>> --- a/configs/qemu-riscv64_smode_defconfig
>>>>> +++ b/configs/qemu-riscv64_smode_defconfig
>>>>> @@ -1,6 +1,6 @@
>>>>> CONFIG_RISCV=y
>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>> CONFIG_ENV_SIZE=0x20000
>>>>> diff --git a/configs/qemu-riscv64_spl_defconfig
>>>>> b/configs/qemu-riscv64_spl_defconfig
>>>>> index bb10145e6e..66dc2a1dd9 100644
>>>>> --- a/configs/qemu-riscv64_spl_defconfig
>>>>> +++ b/configs/qemu-riscv64_spl_defconfig
>>>>> @@ -1,6 +1,6 @@
>>>>> CONFIG_RISCV=y
>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>> CONFIG_ENV_SIZE=0x20000
>>>
>>
>
Heinrich Schuchardt Sept. 14, 2023, 11:19 a.m. UTC | #7
On 9/14/23 12:21, Heinrich Schuchardt wrote:
> On 9/14/23 09:42, Wu, Fei wrote:
>> On 9/14/2023 3:20 PM, Heinrich Schuchardt wrote:
<snip />
>>>
>>> With which version of QEMU were you able to use PCI pass through?
>>>
>> Please try this:
>>
>> base: ccb86f079a9e
>> patch:
>> https://lore.kernel.org/all/CAKmqyKMtAzt5saCUMd4vXYfgAQibpzQJAhtTSuSb+yeKhcYpfw@mail.gmail.com/T/
>
> @Fei
>
> With that patch I see the two memory banks in QEMU but when I attach a
> vfio-pci device my whole system crashes and becomes unresponsive but
> that is not a U-Boot issue.

This problem seems to be specific to my SATA controller. Passing through
USB or Ethernet works fine.

Best regards

Heinrich
Wu, Fei Sept. 22, 2023, 6:17 a.m. UTC | #8
On 9/14/2023 6:21 PM, Heinrich Schuchardt wrote:
> On 9/14/23 09:42, Wu, Fei wrote:
>> On 9/14/2023 3:20 PM, Heinrich Schuchardt wrote:
>>> On 9/14/23 08:48, Wu, Fei wrote:
>>>> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>>>>
>>>>>
>>>>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu
>>>>> <fei2.wu@intel.com>:
>>>>>> In order to enable PCIe passthrough on qemu riscv, the physical
>>>>>> memory
>>>>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB
>>>>>> ram,
>>>>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot
>>>>>> sets
>>>>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>>>>
>>>>> This should move to 7GiB - 1 in your example on riscv64.
>>>>>
>>>> I'm describing the current implementation of board_get_usable_ram_top()
>>>> in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
>>>> changed? Is the comment about 32bit DMA device still valid?
>>>>
>>>> phys_size_t board_get_usable_ram_top(phys_size_t total_size)
>>>> {
>>>>           /*
>>>>            * Ensure that we run from first 4GB so that all
>>>>            * addresses used by U-Boot are 32bit addresses.
>>>>            *
>>>>            * This in-turn ensures that 32bit DMA capablehe
>>>>            * devices work fine because DMA mapping APIs will
>>>>            * provide 32bit DMA addresses only.
>>>>            */
>>>>           if (gd->ram_top >= SZ_4G)
>>>>                   return SZ_4G - 1;
>>>>
>>>>           return gd->ram_top;
>>>> }
>>>
>>> The comment above says 32bit DMA is board specific and not architecture
>>> specific. So it is wrong to have this board_get_usable_ram_top()
>>> function on architecture level. It makes usage of devices with all
>>> memory above 4 GiB impossible.
> 
> @Anup:
> 
> The generic code was introduced by your patch 26f4fd1cb4f6 ("riscv:
> generic: Ensure that U-Boot runs within 4GB for 64bit systems").
> 
> Which devices have that 32bit DMA issue?
> 
> fu540, fu740, jh7110 already have a cpu specific implementation and
> don't need the generic one.
> 
@Anup, any comments? In case you missed this email.

Thanks,
Fei.

>>>
>>> It tried to pass through a SATA controller but received an error:
>>>
>>> # modprobe vfio-pci
>>> # echo 0000:06:00.0 > /sys/bus/pci/drivers/ahci/unbind
>>> # echo 1022 7091 > /sys/bus/pci/drivers/vfio-pci/new_id
>>> # qemu-system-riscv64 -kernel u-boot.bin -nographic -M virt -m 4G
>>> -device vfio-pci,host=0000:06:00.0
>>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: VFIO_MAP_DMA
>>> failed: Invalid argument
>>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: vfio
>>> 0000:06:00.0: failed to setup container for group 5: memory listener
>>> initialization failed: Region riscv_virt_board.ram:
>>> vfio_dma_map(0x55adbde66f70, 0x80000000, 0x100000000, 0x7fcd6fe00000) =
>>> -22 (Invalid argument)
>>>
>>> With which version of QEMU were you able to use PCI pass through?
>>>
>> Please try this:
>>
>> base: ccb86f079a9e
>> patch:
>> https://lore.kernel.org/all/CAKmqyKMtAzt5saCUMd4vXYfgAQibpzQJAhtTSuSb+yeKhcYpfw@mail.gmail.com/T/
> 
> @Fei
> 
> With that patch I see the two memory banks in QEMU but when I attach a
> vfio-pci device my whole system crashes and becomes unresponsive but
> that is not a U-Boot issue.
> 
> Best regards
> 
> Heinrich
> 
>>
>> Thanks,
>> Fei.
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>>>> board_get_usable_ram_top(), but that address is not backed by ram.
>>>>>> This
>>>>>> patch selects the lowest range instead.
>>>>>>
>>>>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>>>>> ---
>>>>>> arch/riscv/cpu/generic/dram.c        | 2 +-
>>>>>> configs/qemu-riscv64_defconfig       | 2 +-
>>>>>> configs/qemu-riscv64_smode_defconfig | 2 +-
>>>>>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>>>>>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/cpu/generic/dram.c
>>>>>> b/arch/riscv/cpu/generic/dram.c
>>>>>> index 44e11bd56c..fb53a57b4e 100644
>>>>>> --- a/arch/riscv/cpu/generic/dram.c
>>>>>> +++ b/arch/riscv/cpu/generic/dram.c
>>>>>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>>
>>>>>> int dram_init(void)
>>>>>> {
>>>>>> -    return fdtdec_setup_mem_size_base();
>>>>>> +    return fdtdec_setup_mem_size_base_lowest();
>>>>>
>>>>> Linaro is working on allowing to download a distro image via https
>>>>> and installing from a RAM disk.
>>>>>
>>>>> We should not artificially reduce the RAM size available for U-Boot
>>>>> by restricting ourselfs to 1 GiB.
>>>>>
>>>>> We must ensure that ram top is in the upper range.
>>>>>
>>>> How do they handle the case of >4GB? board_get_usable_ram_top()
>>>> attached
>>>> above always returns <4GB. And it seems like
>>>> fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
>>>> just picks up one, but which one is selected depending on fdt.
>>>>
>>>> Thanks,
>>>> Fei.
>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>>> }
>>>>>>
>>>>>> int dram_init_banksize(void)
>>>>>> diff --git a/configs/qemu-riscv64_defconfig
>>>>>> b/configs/qemu-riscv64_defconfig
>>>>>> index 9a8bbef192..aa55317d26 100644
>>>>>> --- a/configs/qemu-riscv64_defconfig
>>>>>> +++ b/configs/qemu-riscv64_defconfig
>>>>>> @@ -1,6 +1,6 @@
>>>>>> CONFIG_RISCV=y
>>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>>> CONFIG_ENV_SIZE=0x20000
>>>>>> diff --git a/configs/qemu-riscv64_smode_defconfig
>>>>>> b/configs/qemu-riscv64_smode_defconfig
>>>>>> index 1d0f021ade..de08a49dab 100644
>>>>>> --- a/configs/qemu-riscv64_smode_defconfig
>>>>>> +++ b/configs/qemu-riscv64_smode_defconfig
>>>>>> @@ -1,6 +1,6 @@
>>>>>> CONFIG_RISCV=y
>>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>>> CONFIG_ENV_SIZE=0x20000
>>>>>> diff --git a/configs/qemu-riscv64_spl_defconfig
>>>>>> b/configs/qemu-riscv64_spl_defconfig
>>>>>> index bb10145e6e..66dc2a1dd9 100644
>>>>>> --- a/configs/qemu-riscv64_spl_defconfig
>>>>>> +++ b/configs/qemu-riscv64_spl_defconfig
>>>>>> @@ -1,6 +1,6 @@
>>>>>> CONFIG_RISCV=y
>>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>>> CONFIG_ENV_SIZE=0x20000
>>>>
>>>
>>
>
Wu, Fei Sept. 25, 2023, 5:52 a.m. UTC | #9
On 9/14/2023 6:21 PM, Heinrich Schuchardt wrote:
> On 9/14/23 09:42, Wu, Fei wrote:
>> On 9/14/2023 3:20 PM, Heinrich Schuchardt wrote:
>>> On 9/14/23 08:48, Wu, Fei wrote:
>>>> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>>>>
>>>>>
>>>>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu
>>>>> <fei2.wu@intel.com>:
>>>>>> In order to enable PCIe passthrough on qemu riscv, the physical
>>>>>> memory
>>>>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB
>>>>>> ram,
>>>>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot
>>>>>> sets
>>>>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>>>>
>>>>> This should move to 7GiB - 1 in your example on riscv64.
>>>>>
>>>> I'm describing the current implementation of board_get_usable_ram_top()
>>>> in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
>>>> changed? Is the comment about 32bit DMA device still valid?
>>>>
>>>> phys_size_t board_get_usable_ram_top(phys_size_t total_size)
>>>> {
>>>>           /*
>>>>            * Ensure that we run from first 4GB so that all
>>>>            * addresses used by U-Boot are 32bit addresses.
>>>>            *
>>>>            * This in-turn ensures that 32bit DMA capablehe
>>>>            * devices work fine because DMA mapping APIs will
>>>>            * provide 32bit DMA addresses only.
>>>>            */
>>>>           if (gd->ram_top >= SZ_4G)
>>>>                   return SZ_4G - 1;
>>>>
>>>>           return gd->ram_top;
>>>> }
>>>
>>> The comment above says 32bit DMA is board specific and not architecture
>>> specific. So it is wrong to have this board_get_usable_ram_top()
>>> function on architecture level. It makes usage of devices with all
>>> memory above 4 GiB impossible.
> 
> @Anup:
> 
> The generic code was introduced by your patch 26f4fd1cb4f6 ("riscv:
> generic: Ensure that U-Boot runs within 4GB for 64bit systems").
> 
> Which devices have that 32bit DMA issue?
> 
> fu540, fu740, jh7110 already have a cpu specific implementation and
> don't need the generic one.
> 
Hi Heinrich,

Besides ram_top, ram_base is another one we need to consider.
setup_dest_addr() has:

#ifdef CFG_SYS_SDRAM_BASE
        gd->ram_base = CFG_SYS_SDRAM_BASE;
#endif
        gd->ram_top = gd->ram_base + get_effective_memsize();
        gd->ram_top = board_get_usable_ram_top(gd->mon_len);
        gd->relocaddr = gd->ram_top;

If ram_base is modified in #ifdef, ram_top is still not ensured to be
backed by ram. I tried another workaround w/o this patch, it boots up
successfully:

* return gd->ram_top in board_get_usable_ram_top() that's >4GB
* comment out #ifdef here. This is necessary.

Thanks,
Fei.

>>>
>>> It tried to pass through a SATA controller but received an error:
>>>
>>> # modprobe vfio-pci
>>> # echo 0000:06:00.0 > /sys/bus/pci/drivers/ahci/unbind
>>> # echo 1022 7091 > /sys/bus/pci/drivers/vfio-pci/new_id
>>> # qemu-system-riscv64 -kernel u-boot.bin -nographic -M virt -m 4G
>>> -device vfio-pci,host=0000:06:00.0
>>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: VFIO_MAP_DMA
>>> failed: Invalid argument
>>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: vfio
>>> 0000:06:00.0: failed to setup container for group 5: memory listener
>>> initialization failed: Region riscv_virt_board.ram:
>>> vfio_dma_map(0x55adbde66f70, 0x80000000, 0x100000000, 0x7fcd6fe00000) =
>>> -22 (Invalid argument)
>>>
>>> With which version of QEMU were you able to use PCI pass through?
>>>
>> Please try this:
>>
>> base: ccb86f079a9e
>> patch:
>> https://lore.kernel.org/all/CAKmqyKMtAzt5saCUMd4vXYfgAQibpzQJAhtTSuSb+yeKhcYpfw@mail.gmail.com/T/
> 
> @Fei
> 
> With that patch I see the two memory banks in QEMU but when I attach a
> vfio-pci device my whole system crashes and becomes unresponsive but
> that is not a U-Boot issue.
> 
> Best regards
> 
> Heinrich
> 
>>
>> Thanks,
>> Fei.
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>>>> board_get_usable_ram_top(), but that address is not backed by ram.
>>>>>> This
>>>>>> patch selects the lowest range instead.
>>>>>>
>>>>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>>>>> ---
>>>>>> arch/riscv/cpu/generic/dram.c        | 2 +-
>>>>>> configs/qemu-riscv64_defconfig       | 2 +-
>>>>>> configs/qemu-riscv64_smode_defconfig | 2 +-
>>>>>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>>>>>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/cpu/generic/dram.c
>>>>>> b/arch/riscv/cpu/generic/dram.c
>>>>>> index 44e11bd56c..fb53a57b4e 100644
>>>>>> --- a/arch/riscv/cpu/generic/dram.c
>>>>>> +++ b/arch/riscv/cpu/generic/dram.c
>>>>>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>>
>>>>>> int dram_init(void)
>>>>>> {
>>>>>> -    return fdtdec_setup_mem_size_base();
>>>>>> +    return fdtdec_setup_mem_size_base_lowest();
>>>>>
>>>>> Linaro is working on allowing to download a distro image via https
>>>>> and installing from a RAM disk.
>>>>>
>>>>> We should not artificially reduce the RAM size available for U-Boot
>>>>> by restricting ourselfs to 1 GiB.
>>>>>
>>>>> We must ensure that ram top is in the upper range.
>>>>>
>>>> How do they handle the case of >4GB? board_get_usable_ram_top()
>>>> attached
>>>> above always returns <4GB. And it seems like
>>>> fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
>>>> just picks up one, but which one is selected depending on fdt.
>>>>
>>>> Thanks,
>>>> Fei.
>>>>
>>>>> Best regards
>>>>>
>>>>> Heinrich
>>>>>
>>>>>> }
>>>>>>
>>>>>> int dram_init_banksize(void)
>>>>>> diff --git a/configs/qemu-riscv64_defconfig
>>>>>> b/configs/qemu-riscv64_defconfig
>>>>>> index 9a8bbef192..aa55317d26 100644
>>>>>> --- a/configs/qemu-riscv64_defconfig
>>>>>> +++ b/configs/qemu-riscv64_defconfig
>>>>>> @@ -1,6 +1,6 @@
>>>>>> CONFIG_RISCV=y
>>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>>> CONFIG_ENV_SIZE=0x20000
>>>>>> diff --git a/configs/qemu-riscv64_smode_defconfig
>>>>>> b/configs/qemu-riscv64_smode_defconfig
>>>>>> index 1d0f021ade..de08a49dab 100644
>>>>>> --- a/configs/qemu-riscv64_smode_defconfig
>>>>>> +++ b/configs/qemu-riscv64_smode_defconfig
>>>>>> @@ -1,6 +1,6 @@
>>>>>> CONFIG_RISCV=y
>>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>>> CONFIG_ENV_SIZE=0x20000
>>>>>> diff --git a/configs/qemu-riscv64_spl_defconfig
>>>>>> b/configs/qemu-riscv64_spl_defconfig
>>>>>> index bb10145e6e..66dc2a1dd9 100644
>>>>>> --- a/configs/qemu-riscv64_spl_defconfig
>>>>>> +++ b/configs/qemu-riscv64_spl_defconfig
>>>>>> @@ -1,6 +1,6 @@
>>>>>> CONFIG_RISCV=y
>>>>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>>>>> -CONFIG_NR_DRAM_BANKS=1
>>>>>> +CONFIG_NR_DRAM_BANKS=2
>>>>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>>>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>>>>> CONFIG_ENV_SIZE=0x20000
>>>>
>>>
>>
>
Anup Patel Sept. 26, 2023, 5:07 a.m. UTC | #10
On Thu, Sep 14, 2023 at 12:49 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 9/14/23 08:48, Wu, Fei wrote:
> > On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
> >>
> >>
> >> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
> >>> In order to enable PCIe passthrough on qemu riscv, the physical memory
> >>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
> >>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
> >>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
> >>
> >> This should move to 7GiB - 1 in your example on riscv64.
> >>
> > I'm describing the current implementation of board_get_usable_ram_top()
> > in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
> > changed? Is the comment about 32bit DMA device still valid?
> >
> > phys_size_t board_get_usable_ram_top(phys_size_t total_size)
> > {
> >          /*
> >           * Ensure that we run from first 4GB so that all
> >           * addresses used by U-Boot are 32bit addresses.
> >           *
> >           * This in-turn ensures that 32bit DMA capable
> >           * devices work fine because DMA mapping APIs will
> >           * provide 32bit DMA addresses only.
> >           */
> >          if (gd->ram_top >= SZ_4G)
> >                  return SZ_4G - 1;
> >
> >          return gd->ram_top;
> > }
>
> The comment above says 32bit DMA is board specific and not architecture
> specific. So it is wrong to have this board_get_usable_ram_top()
> function on architecture level. It makes usage of devices with all
> memory above 4 GiB impossible.
>
> It tried to pass through a SATA controller but received an error:
>
> # modprobe vfio-pci
> # echo 0000:06:00.0 > /sys/bus/pci/drivers/ahci/unbind
> # echo 1022 7091 > /sys/bus/pci/drivers/vfio-pci/new_id
> # qemu-system-riscv64 -kernel u-boot.bin -nographic -M virt -m 4G
> -device vfio-pci,host=0000:06:00.0
> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: VFIO_MAP_DMA
> failed: Invalid argument
> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: vfio
> 0000:06:00.0: failed to setup container for group 5: memory listener
> initialization failed: Region riscv_virt_board.ram:
> vfio_dma_map(0x55adbde66f70, 0x80000000, 0x100000000, 0x7fcd6fe00000) =
> -22 (Invalid argument)
>
> With which version of QEMU were you able to use PCI pass through?

The original S-mode SiFive unleashed board support was using the
generic CPU under arch/riscv. We have Cadance MACB ethernet
device on this board which is a 32-bit device and can't access memory
beyond 4GB.

For more details, refer "git show 26f4fd1cb4f2"

I think we can wrap the board_get_usable_ram_top() implementation
in generic RISC-V CPU with a #ifdef Kconfig option which allows
certain boards (such as QEMU-virt) to implement their own
board_get_usable_ram_top().

Regards,
Anup

>
> Best regards
>
> Heinrich
>
> >
> >>> board_get_usable_ram_top(), but that address is not backed by ram. This
> >>> patch selects the lowest range instead.
> >>>
> >>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> >>> ---
> >>> arch/riscv/cpu/generic/dram.c        | 2 +-
> >>> configs/qemu-riscv64_defconfig       | 2 +-
> >>> configs/qemu-riscv64_smode_defconfig | 2 +-
> >>> configs/qemu-riscv64_spl_defconfig   | 2 +-
> >>> 4 files changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
> >>> index 44e11bd56c..fb53a57b4e 100644
> >>> --- a/arch/riscv/cpu/generic/dram.c
> >>> +++ b/arch/riscv/cpu/generic/dram.c
> >>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
> >>>
> >>> int dram_init(void)
> >>> {
> >>> -   return fdtdec_setup_mem_size_base();
> >>> +   return fdtdec_setup_mem_size_base_lowest();
> >>
> >> Linaro is working on allowing to download a distro image via https and installing from a RAM disk.
> >>
> >> We should not artificially reduce the RAM size available for U-Boot by restricting ourselfs to 1 GiB.
> >>
> >> We must ensure that ram top is in the upper range.
> >>
> > How do they handle the case of >4GB? board_get_usable_ram_top() attached
> > above always returns <4GB. And it seems like
> > fdtdec_setup_mem_size_base() cannot ensure the upper one is picked, it
> > just picks up one, but which one is selected depending on fdt.
> >
> > Thanks,
> > Fei.
> >
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> }
> >>>
> >>> int dram_init_banksize(void)
> >>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
> >>> index 9a8bbef192..aa55317d26 100644
> >>> --- a/configs/qemu-riscv64_defconfig
> >>> +++ b/configs/qemu-riscv64_defconfig
> >>> @@ -1,6 +1,6 @@
> >>> CONFIG_RISCV=y
> >>> CONFIG_SYS_MALLOC_LEN=0x800000
> >>> -CONFIG_NR_DRAM_BANKS=1
> >>> +CONFIG_NR_DRAM_BANKS=2
> >>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> >>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
> >>> CONFIG_ENV_SIZE=0x20000
> >>> diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
> >>> index 1d0f021ade..de08a49dab 100644
> >>> --- a/configs/qemu-riscv64_smode_defconfig
> >>> +++ b/configs/qemu-riscv64_smode_defconfig
> >>> @@ -1,6 +1,6 @@
> >>> CONFIG_RISCV=y
> >>> CONFIG_SYS_MALLOC_LEN=0x800000
> >>> -CONFIG_NR_DRAM_BANKS=1
> >>> +CONFIG_NR_DRAM_BANKS=2
> >>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> >>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
> >>> CONFIG_ENV_SIZE=0x20000
> >>> diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
> >>> index bb10145e6e..66dc2a1dd9 100644
> >>> --- a/configs/qemu-riscv64_spl_defconfig
> >>> +++ b/configs/qemu-riscv64_spl_defconfig
> >>> @@ -1,6 +1,6 @@
> >>> CONFIG_RISCV=y
> >>> CONFIG_SYS_MALLOC_LEN=0x800000
> >>> -CONFIG_NR_DRAM_BANKS=1
> >>> +CONFIG_NR_DRAM_BANKS=2
> >>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> >>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
> >>> CONFIG_ENV_SIZE=0x20000
> >
>
Heinrich Schuchardt Sept. 26, 2023, 7:04 a.m. UTC | #11
On 9/26/23 07:07, Anup Patel wrote:
> On Thu, Sep 14, 2023 at 12:49 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 9/14/23 08:48, Wu, Fei wrote:
>>> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>>>
>>>>
>>>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>>>>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>>>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>>>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>>>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>>>
>>>> This should move to 7GiB - 1 in your example on riscv64.
>>>>
>>> I'm describing the current implementation of board_get_usable_ram_top()
>>> in ./arch/riscv/cpu/generic/dram.c. Do you mean this function should be
>>> changed? Is the comment about 32bit DMA device still valid?
>>>
>>> phys_size_t board_get_usable_ram_top(phys_size_t total_size)
>>> {
>>>           /*
>>>            * Ensure that we run from first 4GB so that all
>>>            * addresses used by U-Boot are 32bit addresses.
>>>            *
>>>            * This in-turn ensures that 32bit DMA capable
>>>            * devices work fine because DMA mapping APIs will
>>>            * provide 32bit DMA addresses only.
>>>            */
>>>           if (gd->ram_top >= SZ_4G)
>>>                   return SZ_4G - 1;
>>>
>>>           return gd->ram_top;
>>> }
>>
>> The comment above says 32bit DMA is board specific and not architecture
>> specific. So it is wrong to have this board_get_usable_ram_top()
>> function on architecture level. It makes usage of devices with all
>> memory above 4 GiB impossible.
>>
>> It tried to pass through a SATA controller but received an error:
>>
>> # modprobe vfio-pci
>> # echo 0000:06:00.0 > /sys/bus/pci/drivers/ahci/unbind
>> # echo 1022 7091 > /sys/bus/pci/drivers/vfio-pci/new_id
>> # qemu-system-riscv64 -kernel u-boot.bin -nographic -M virt -m 4G
>> -device vfio-pci,host=0000:06:00.0
>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: VFIO_MAP_DMA
>> failed: Invalid argument
>> qemu-system-riscv64: -device vfio-pci,host=0000:06:00.0: vfio
>> 0000:06:00.0: failed to setup container for group 5: memory listener
>> initialization failed: Region riscv_virt_board.ram:
>> vfio_dma_map(0x55adbde66f70, 0x80000000, 0x100000000, 0x7fcd6fe00000) =
>> -22 (Invalid argument)
>>
>> With which version of QEMU were you able to use PCI pass through?
> 
> The original S-mode SiFive unleashed board support was using the
> generic CPU under arch/riscv. We have Cadance MACB ethernet
> device on this board which is a 32-bit device and can't access memory
> beyond 4GB.
> 
> For more details, refer "git show 26f4fd1cb4f2"
> 
> I think we can wrap the board_get_usable_ram_top() implementation
> in generic RISC-V CPU with a #ifdef Kconfig option which allows
> certain boards (such as QEMU-virt) to implement their own
> board_get_usable_ram_top().
> 
> Regards,
> Anup

Thanks Anup for clarification.

As of today sifive_unleashed_defconfig builds 
arch/riscv/cpu/fu540/dram.c which contains the cpu specific 
implementation of board_get_usable_ram_top().

We already have a weak implementation

common/board_f.c:327:
__weak phys_addr_t board_get_usable_ram_top(phys_size_t total_size)

So we don't need a Kconfig option. We just can drop the implementation 
in arch/riscv/cpu/generic/dram.c.

QEMU should not need an implementation of the function but provide the 
implemented RAM banks via its device-tree.

Best regards

Heinrich
Wu, Fei Oct. 9, 2023, 10:04 a.m. UTC | #12
On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
> 
> 
> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
> 
> This should move to 7GiB - 1 in your example on riscv64.
> 
>> board_get_usable_ram_top(), but that address is not backed by ram. This
>> patch selects the lowest range instead.
>>
>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>> ---
>> arch/riscv/cpu/generic/dram.c        | 2 +-
>> configs/qemu-riscv64_defconfig       | 2 +-
>> configs/qemu-riscv64_smode_defconfig | 2 +-
>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
>> index 44e11bd56c..fb53a57b4e 100644
>> --- a/arch/riscv/cpu/generic/dram.c
>> +++ b/arch/riscv/cpu/generic/dram.c
>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>> int dram_init(void)
>> {
>> -	return fdtdec_setup_mem_size_base();
>> +	return fdtdec_setup_mem_size_base_lowest();
> 
> Linaro is working on allowing to download a distro image via https and installing from a RAM disk.
> 
It's okay if we don't do this installation and pcie passthrough together
on risc-v, nothing is changed if there is single-range memory, 1GB is
large enough for other cases.

> We should not artificially reduce the RAM size available for U-Boot by restricting ourselfs to 1 GiB.
> 
> We must ensure that ram top is in the upper range.
> 
You mentioned a generic solution for all architectures in another
thread, it looks not trivial for me with little background on u-boot,
e.g. something is confusing to me, when a board should define
CFG_SYS_SDRAM_BASE, what happens if gd->ram_base != CFG_SYS_SDRAM_BASE,
I'm not sure if you or anyone can help drive this together. btw, the
good part of this patch is that it doesn't change the semantics of
ram_base, and the best choice is the largest range, the upper range
might not.

Thanks,
Fei.

> Best regards
> 
> Heinrich
> 
>> }
>>
>> int dram_init_banksize(void)
>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
>> index 9a8bbef192..aa55317d26 100644
>> --- a/configs/qemu-riscv64_defconfig
>> +++ b/configs/qemu-riscv64_defconfig
>> @@ -1,6 +1,6 @@
>> CONFIG_RISCV=y
>> CONFIG_SYS_MALLOC_LEN=0x800000
>> -CONFIG_NR_DRAM_BANKS=1
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>> CONFIG_ENV_SIZE=0x20000
>> diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
>> index 1d0f021ade..de08a49dab 100644
>> --- a/configs/qemu-riscv64_smode_defconfig
>> +++ b/configs/qemu-riscv64_smode_defconfig
>> @@ -1,6 +1,6 @@
>> CONFIG_RISCV=y
>> CONFIG_SYS_MALLOC_LEN=0x800000
>> -CONFIG_NR_DRAM_BANKS=1
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>> CONFIG_ENV_SIZE=0x20000
>> diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
>> index bb10145e6e..66dc2a1dd9 100644
>> --- a/configs/qemu-riscv64_spl_defconfig
>> +++ b/configs/qemu-riscv64_spl_defconfig
>> @@ -1,6 +1,6 @@
>> CONFIG_RISCV=y
>> CONFIG_SYS_MALLOC_LEN=0x800000
>> -CONFIG_NR_DRAM_BANKS=1
>> +CONFIG_NR_DRAM_BANKS=2
>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>> CONFIG_ENV_SIZE=0x20000
Wu, Fei Oct. 11, 2023, 10:05 a.m. UTC | #13
On 10/9/2023 6:04 PM, Wu, Fei wrote:
> On 9/14/2023 2:05 PM, Heinrich Schuchardt wrote:
>>
>>
>> Am 14. September 2023 07:30:55 MESZ schrieb Fei Wu <fei2.wu@intel.com>:
>>> In order to enable PCIe passthrough on qemu riscv, the physical memory
>>> range between 3GB and 4GB is reserved. Therefore if guest has 4GB ram,
>>> two ranges are created as [2G, 3G) and [4G, 7G), currently u-boot sets
>>> ram_top to 4G - 1 if the gd->ram_top is above 4G in
>>
>> This should move to 7GiB - 1 in your example on riscv64.
>>
>>> board_get_usable_ram_top(), but that address is not backed by ram. This
>>> patch selects the lowest range instead.
>>>
>>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>>> ---
>>> arch/riscv/cpu/generic/dram.c        | 2 +-
>>> configs/qemu-riscv64_defconfig       | 2 +-
>>> configs/qemu-riscv64_smode_defconfig | 2 +-
>>> configs/qemu-riscv64_spl_defconfig   | 2 +-
>>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
>>> index 44e11bd56c..fb53a57b4e 100644
>>> --- a/arch/riscv/cpu/generic/dram.c
>>> +++ b/arch/riscv/cpu/generic/dram.c
>>> @@ -13,7 +13,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>
>>> int dram_init(void)
>>> {
>>> -	return fdtdec_setup_mem_size_base();
>>> +	return fdtdec_setup_mem_size_base_lowest();
>>
>> Linaro is working on allowing to download a distro image via https and installing from a RAM disk.
>>
> It's okay if we don't do this installation and pcie passthrough together
> on risc-v, nothing is changed if there is single-range memory, 1GB is
> large enough for other cases.
> 
So far there are the following methods:

1. use fdtdec_setup_mem_size_base_lowest as this one. This is a change
to generic riscv iff the multi-range memory is applied, which is not the
case for all the current riscv boards. If there are such memory layout
riscv boards in the future, it can have its specialized version.

2. specialize dram_init() for qemu-riscv, it limits the impact only to
qemu-riscv, even if there is a need to download image, it doesn't need
to enable pcie-passthru at the same time, so no impact from this
perspective, the drawback is that qemu cannot reuse the generic logic.

3. a complete solution for multi-range memory layout to select the
largest range for all architectures.

Are #1 or #2 acceptable at this stage? If we go #3, can someone help me
drive this together to reduce the email ping-pongs?

Thanks,
Fei.


>> We should not artificially reduce the RAM size available for U-Boot by restricting ourselfs to 1 GiB.
>>
>> We must ensure that ram top is in the upper range.
>>
> You mentioned a generic solution for all architectures in another
> thread, it looks not trivial for me with little background on u-boot,
> e.g. something is confusing to me, when a board should define
> CFG_SYS_SDRAM_BASE, what happens if gd->ram_base != CFG_SYS_SDRAM_BASE,
> I'm not sure if you or anyone can help drive this together. btw, the
> good part of this patch is that it doesn't change the semantics of
> ram_base, and the best choice is the largest range, the upper range
> might not.
> 
> Thanks,
> Fei.
> 
>> Best regards
>>
>> Heinrich
>>
>>> }
>>>
>>> int dram_init_banksize(void)
>>> diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
>>> index 9a8bbef192..aa55317d26 100644
>>> --- a/configs/qemu-riscv64_defconfig
>>> +++ b/configs/qemu-riscv64_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>>> diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
>>> index 1d0f021ade..de08a49dab 100644
>>> --- a/configs/qemu-riscv64_smode_defconfig
>>> +++ b/configs/qemu-riscv64_smode_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>>> diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
>>> index bb10145e6e..66dc2a1dd9 100644
>>> --- a/configs/qemu-riscv64_spl_defconfig
>>> +++ b/configs/qemu-riscv64_spl_defconfig
>>> @@ -1,6 +1,6 @@
>>> CONFIG_RISCV=y
>>> CONFIG_SYS_MALLOC_LEN=0x800000
>>> -CONFIG_NR_DRAM_BANKS=1
>>> +CONFIG_NR_DRAM_BANKS=2
>>> CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
>>> CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
>>> CONFIG_ENV_SIZE=0x20000
>
diff mbox series

Patch

diff --git a/arch/riscv/cpu/generic/dram.c b/arch/riscv/cpu/generic/dram.c
index 44e11bd56c..fb53a57b4e 100644
--- a/arch/riscv/cpu/generic/dram.c
+++ b/arch/riscv/cpu/generic/dram.c
@@ -13,7 +13,7 @@  DECLARE_GLOBAL_DATA_PTR;
 
 int dram_init(void)
 {
-	return fdtdec_setup_mem_size_base();
+	return fdtdec_setup_mem_size_base_lowest();
 }
 
 int dram_init_banksize(void)
diff --git a/configs/qemu-riscv64_defconfig b/configs/qemu-riscv64_defconfig
index 9a8bbef192..aa55317d26 100644
--- a/configs/qemu-riscv64_defconfig
+++ b/configs/qemu-riscv64_defconfig
@@ -1,6 +1,6 @@ 
 CONFIG_RISCV=y
 CONFIG_SYS_MALLOC_LEN=0x800000
-CONFIG_NR_DRAM_BANKS=1
+CONFIG_NR_DRAM_BANKS=2
 CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
 CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
 CONFIG_ENV_SIZE=0x20000
diff --git a/configs/qemu-riscv64_smode_defconfig b/configs/qemu-riscv64_smode_defconfig
index 1d0f021ade..de08a49dab 100644
--- a/configs/qemu-riscv64_smode_defconfig
+++ b/configs/qemu-riscv64_smode_defconfig
@@ -1,6 +1,6 @@ 
 CONFIG_RISCV=y
 CONFIG_SYS_MALLOC_LEN=0x800000
-CONFIG_NR_DRAM_BANKS=1
+CONFIG_NR_DRAM_BANKS=2
 CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
 CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
 CONFIG_ENV_SIZE=0x20000
diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig
index bb10145e6e..66dc2a1dd9 100644
--- a/configs/qemu-riscv64_spl_defconfig
+++ b/configs/qemu-riscv64_spl_defconfig
@@ -1,6 +1,6 @@ 
 CONFIG_RISCV=y
 CONFIG_SYS_MALLOC_LEN=0x800000
-CONFIG_NR_DRAM_BANKS=1
+CONFIG_NR_DRAM_BANKS=2
 CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
 CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x80200000
 CONFIG_ENV_SIZE=0x20000