diff mbox series

[v3,12/12] xen/arm: List static shared memory regions as /memory nodes

Message ID 20240418073652.3622828-13-luca.fancellu@arm.com (mailing list archive)
State New
Headers show
Series Static shared memory followup v2 - pt1 | expand

Commit Message

Luca Fancellu April 18, 2024, 7:36 a.m. UTC
Currently Xen is not exporting the static shared memory regions
to the device tree as /memory node, this commit is fixing this
issue.

Given that now make_memory_node needs a parameter 'struct kernel_info'
in order to call the new function shm_mem_node_fill_reg_range,
take the occasion to remove the unused struct domain parameter.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v3:
 - removed previous patch that was merging the intervals, rebase
   changes.
v2:
 - try to use make_memory_node, don't add overlapping ranges of
   memory, commit message changed.
v1:
 - new patch
---
---
 xen/arch/arm/dom0less-build.c           |  2 +-
 xen/arch/arm/domain_build.c             | 34 +++++++++++++++++--------
 xen/arch/arm/include/asm/domain_build.h |  2 +-
 xen/arch/arm/include/asm/static-shmem.h | 15 +++++++++++
 xen/arch/arm/static-shmem.c             | 23 +++++++++++++++++
 5 files changed, 63 insertions(+), 13 deletions(-)

Comments

Michal Orzel April 22, 2024, 7:55 a.m. UTC | #1
Hi Luca,

On 18/04/2024 09:36, Luca Fancellu wrote:
> 
> 
> Currently Xen is not exporting the static shared memory regions
> to the device tree as /memory node, this commit is fixing this
> issue.
> 
> Given that now make_memory_node needs a parameter 'struct kernel_info'
> in order to call the new function shm_mem_node_fill_reg_range,
> take the occasion to remove the unused struct domain parameter.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v3:
>  - removed previous patch that was merging the intervals, rebase
>    changes.
> v2:
>  - try to use make_memory_node, don't add overlapping ranges of
>    memory, commit message changed.
> v1:
>  - new patch
> ---
> ---
>  xen/arch/arm/dom0less-build.c           |  2 +-
>  xen/arch/arm/domain_build.c             | 34 +++++++++++++++++--------
>  xen/arch/arm/include/asm/domain_build.h |  2 +-
>  xen/arch/arm/include/asm/static-shmem.h | 15 +++++++++++
>  xen/arch/arm/static-shmem.c             | 23 +++++++++++++++++
>  5 files changed, 63 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 51cf03221d56..74f053c242f4 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -642,7 +642,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>      if ( ret )
>          goto err;
> 
> -    ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
> +    ret = make_memory_node(kinfo, addrcells, sizecells,
>                             kernel_info_get_mem(kinfo));
>      if ( ret )
>          goto err;
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 68532ddc084c..15f888169c4e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -756,15 +756,14 @@ int __init domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit)
>      return fdt_begin_node(fdt, buf);
>  }
> 
> -int __init make_memory_node(const struct domain *d,
> -                            void *fdt,
> -                            int addrcells, int sizecells,
> -                            const struct membanks *mem)
> +int __init make_memory_node(const struct kernel_info *kinfo, int addrcells,
> +                            int sizecells, const struct membanks *mem)
>  {
> +    void *fdt = kinfo->fdt;
>      unsigned int i;
>      int res, reg_size = addrcells + sizecells;
>      int nr_cells = 0;
> -    __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
> +    __be32 reg[DT_MEM_NODE_REG_RANGE_SIZE];
>      __be32 *cells;
> 
>      if ( mem->nr_banks == 0 )
> @@ -797,14 +796,28 @@ int __init make_memory_node(const struct domain *d,
>          if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN )
>              continue;
> 
> -        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> -                   i, start, start + size);
> -
>          nr_cells += reg_size;
>          BUG_ON(nr_cells >= ARRAY_SIZE(reg));
>          dt_child_set_range(&cells, addrcells, sizecells, start, size);
>      }
> 
> +    /*
> +     * static shared memory banks need to be listed as /memory node, so when
> +     * this function is handling the normal memory, add the banks.
> +     */
> +    if ( mem == kernel_info_get_mem(kinfo) )
> +        shm_mem_node_fill_reg_range(kinfo, reg, &nr_cells, addrcells,
> +                                    sizecells);
> +
> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
> +    {
> +        u64 start = dt_read_number(cells, addrcells);
We should no longer use Linux derived types like u64. Use uint64_t.

> +        u64 size = dt_read_number(cells + addrcells, sizecells);
> +
> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> +                   i, start, start + size);
i is unsigned so the correct format specifier should be %u

> +    }
> +
>      dt_dprintk("(reg size %d, nr cells %d)\n", reg_size, nr_cells);
> 
>      res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg));
> @@ -1783,7 +1796,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>          if ( res )
>              return res;
> 
> -        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
> +        res = make_memory_node(kinfo, addrcells, sizecells,
>                                 kernel_info_get_mem(kinfo));
>          if ( res )
>              return res;
> @@ -1794,8 +1807,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
>           */
>          if ( reserved_mem->nr_banks > 0 )
>          {
> -            res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
> -                                   reserved_mem);
> +            res = make_memory_node(kinfo, addrcells, sizecells, reserved_mem);
>              if ( res )
>                  return res;
>          }
> diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
> index 026d975da28e..45936212ca21 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -14,7 +14,7 @@ int make_chosen_node(const struct kernel_info *kinfo);
>  int make_cpus_node(const struct domain *d, void *fdt);
>  int make_hypervisor_node(struct domain *d, const struct kernel_info *kinfo,
>                           int addrcells, int sizecells);
> -int make_memory_node(const struct domain *d, void *fdt, int addrcells,
> +int make_memory_node(const struct kernel_info *kinfo, int addrcells,
>                       int sizecells, const struct membanks *mem);
>  int make_psci_node(void *fdt);
>  int make_timer_node(const struct kernel_info *kinfo);
> diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
> index 7495a91e7a31..3b6569e5703f 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -3,10 +3,15 @@
>  #ifndef __ASM_STATIC_SHMEM_H_
>  #define __ASM_STATIC_SHMEM_H_
> 
> +#include <xen/types.h>
>  #include <asm/kernel.h>
> +#include <asm/setup.h>
> 
>  #ifdef CONFIG_STATIC_SHM
> 
> +/* Worst case /memory node reg element: (addrcells + sizecells) */
> +#define DT_MEM_NODE_REG_RANGE_SIZE ((NR_MEM_BANKS + NR_SHMEM_BANKS) * 4)
> +
>  int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>                            int sizecells);
> 
> @@ -37,8 +42,14 @@ int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>  int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>                                int sizecells);
> 
> +void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, __be32 *reg,
> +                                 int *nr_cells, int addrcells, int sizecells);
> +
>  #else /* !CONFIG_STATIC_SHM */
> 
> +/* Worst case /memory node reg element: (addrcells + sizecells) */
> +#define DT_MEM_NODE_REG_RANGE_SIZE (NR_MEM_BANKS * 4)
> +
>  static inline int make_resv_memory_node(const struct kernel_info *kinfo,
>                                          int addrcells, int sizecells)
>  {
> @@ -86,6 +97,10 @@ static inline int make_shm_resv_memory_node(const struct kernel_info *kinfo,
>      return 0;
>  }
> 
> +static inline void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
> +                                               __be32 *reg, int *nr_cells,
> +                                               int addrcells, int sizecells) {};
> +
>  #endif /* CONFIG_STATIC_SHM */
> 
>  #endif /* __ASM_STATIC_SHMEM_H_ */
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index c85f60dd1bf7..07c93a820508 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
> 
> +#include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <xen/rangeset.h>
>  #include <xen/sched.h>
> @@ -668,6 +669,28 @@ int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>      return res;
>  }
> 
> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
> +                                        __be32 *reg, int *nr_cells,
> +                                        int addrcells, int sizecells)
> +{
> +    const struct membanks *mem = &kinfo->shm_mem.common;
> +    unsigned int i;
> +    __be32 *cells;
> +
> +    BUG_ON(!nr_cells || !reg);
> +
> +    cells = &reg[*nr_cells];
> +    for ( i = 0; i < mem->nr_banks; i++ )
> +    {
> +        u64 start = mem->bank[i].start;
ditto

Rest LGTM:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Luca Fancellu April 22, 2024, 8:07 a.m. UTC | #2
Hi Michal,

>> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
>> +    {
>> +        u64 start = dt_read_number(cells, addrcells);
> We should no longer use Linux derived types like u64. Use uint64_t.
> 
>> +        u64 size = dt_read_number(cells + addrcells, sizecells);
>> +
>> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>> +                   i, start, start + size);
> i is unsigned so the correct format specifier should be %u

Right, should have been more careful when copying the code from above

>> 
>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
>> +                                        __be32 *reg, int *nr_cells,
>> +                                        int addrcells, int sizecells)
>> +{
>> +    const struct membanks *mem = &kinfo->shm_mem.common;
>> +    unsigned int i;
>> +    __be32 *cells;
>> +
>> +    BUG_ON(!nr_cells || !reg);
>> +
>> +    cells = &reg[*nr_cells];
>> +    for ( i = 0; i < mem->nr_banks; i++ )
>> +    {
>> +        u64 start = mem->bank[i].start;
> ditto

Will fix, here paddr_t should be ok isn’t it?

> 
> Rest LGTM:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks, I will send the next one shortly.

Cheers,
Luca
Michal Orzel April 22, 2024, 9:26 a.m. UTC | #3
On 22/04/2024 10:07, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>>> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
>>> +    {
>>> +        u64 start = dt_read_number(cells, addrcells);
>> We should no longer use Linux derived types like u64. Use uint64_t.
>>
>>> +        u64 size = dt_read_number(cells + addrcells, sizecells);
>>> +
>>> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>>> +                   i, start, start + size);
>> i is unsigned so the correct format specifier should be %u
> 
> Right, should have been more careful when copying the code from above
> 
>>>
>>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
>>> +                                        __be32 *reg, int *nr_cells,
>>> +                                        int addrcells, int sizecells)
>>> +{
>>> +    const struct membanks *mem = &kinfo->shm_mem.common;
>>> +    unsigned int i;
>>> +    __be32 *cells;
>>> +
>>> +    BUG_ON(!nr_cells || !reg);
>>> +
>>> +    cells = &reg[*nr_cells];
>>> +    for ( i = 0; i < mem->nr_banks; i++ )
>>> +    {
>>> +        u64 start = mem->bank[i].start;
>> ditto
> 
> Will fix, here paddr_t should be ok isn’t it?
yes

> 
>>
>> Rest LGTM:
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> Thanks, I will send the next one shortly.
I don't think there is a need to respin the whole series just for these fixes.
You should wait for the committers opinion.

~Michal
Julien Grall April 22, 2024, 10:24 a.m. UTC | #4
Hi,

On 22/04/2024 10:26, Michal Orzel wrote:
> 
> 
> On 22/04/2024 10:07, Luca Fancellu wrote:
>>
>>
>> Hi Michal,
>>
>>>> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
>>>> +    {
>>>> +        u64 start = dt_read_number(cells, addrcells);
>>> We should no longer use Linux derived types like u64. Use uint64_t.
>>>
>>>> +        u64 size = dt_read_number(cells + addrcells, sizecells);
>>>> +
>>>> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>>>> +                   i, start, start + size);
>>> i is unsigned so the correct format specifier should be %u
>>
>> Right, should have been more careful when copying the code from above
>>
>>>>
>>>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
>>>> +                                        __be32 *reg, int *nr_cells,
>>>> +                                        int addrcells, int sizecells)
>>>> +{
>>>> +    const struct membanks *mem = &kinfo->shm_mem.common;
>>>> +    unsigned int i;
>>>> +    __be32 *cells;
>>>> +
>>>> +    BUG_ON(!nr_cells || !reg);
>>>> +
>>>> +    cells = &reg[*nr_cells];
>>>> +    for ( i = 0; i < mem->nr_banks; i++ )
>>>> +    {
>>>> +        u64 start = mem->bank[i].start;
>>> ditto
>>
>> Will fix, here paddr_t should be ok isn’t it?
> yes
> 
>>
>>>
>>> Rest LGTM:
>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> Thanks, I will send the next one shortly.
> I don't think there is a need to respin the whole series just for these fixes.
> You should wait for the committers opinion.

AFAICT, there are multiple changes requested in various line. So I would 
rather prefer if this is respinned.

If this is the only patch that requires to change. You could send a new 
one in reply-to this patch. I think b4 is clever enough to pick up the 
new version in that case.

Cheers,
Luca Fancellu April 22, 2024, 10:39 a.m. UTC | #5
> On 22 Apr 2024, at 11:24, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 22/04/2024 10:26, Michal Orzel wrote:
>> On 22/04/2024 10:07, Luca Fancellu wrote:
>>> 
>>> 
>>> Hi Michal,
>>> 
>>>>> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
>>>>> +    {
>>>>> +        u64 start = dt_read_number(cells, addrcells);
>>>> We should no longer use Linux derived types like u64. Use uint64_t.
>>>> 
>>>>> +        u64 size = dt_read_number(cells + addrcells, sizecells);
>>>>> +
>>>>> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>>>>> +                   i, start, start + size);
>>>> i is unsigned so the correct format specifier should be %u
>>> 
>>> Right, should have been more careful when copying the code from above
>>> 
>>>>> 
>>>>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
>>>>> +                                        __be32 *reg, int *nr_cells,
>>>>> +                                        int addrcells, int sizecells)
>>>>> +{
>>>>> +    const struct membanks *mem = &kinfo->shm_mem.common;
>>>>> +    unsigned int i;
>>>>> +    __be32 *cells;
>>>>> +
>>>>> +    BUG_ON(!nr_cells || !reg);
>>>>> +
>>>>> +    cells = &reg[*nr_cells];
>>>>> +    for ( i = 0; i < mem->nr_banks; i++ )
>>>>> +    {
>>>>> +        u64 start = mem->bank[i].start;
>>>> ditto
>>> 
>>> Will fix, here paddr_t should be ok isn’t it?
>> yes
>>> 
>>>> 
>>>> Rest LGTM:
>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>> 
>>> Thanks, I will send the next one shortly.
>> I don't think there is a need to respin the whole series just for these fixes.
>> You should wait for the committers opinion.
> 
> AFAICT, there are multiple changes requested in various line. So I would rather prefer if this is respinned.
> 
> If this is the only patch that requires to change. You could send a new one in reply-to this patch. I think b4 is clever enough to pick up the new version in that case.

All the other patches are already reviewed by Michal, if you agree with his review it’s ok for me to respin only this one

Cheers,
Luca

> 
> Cheers,
> 
> -- 
> Julien Grall
Julien Grall April 22, 2024, 10:45 a.m. UTC | #6
Hi Luca,

On 22/04/2024 11:39, Luca Fancellu wrote:
> 
> 
>> On 22 Apr 2024, at 11:24, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 22/04/2024 10:26, Michal Orzel wrote:
>>> On 22/04/2024 10:07, Luca Fancellu wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>>>> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
>>>>>> +    {
>>>>>> +        u64 start = dt_read_number(cells, addrcells);
>>>>> We should no longer use Linux derived types like u64. Use uint64_t.
>>>>>
>>>>>> +        u64 size = dt_read_number(cells + addrcells, sizecells);
>>>>>> +
>>>>>> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>>>>>> +                   i, start, start + size);
>>>>> i is unsigned so the correct format specifier should be %u
>>>>
>>>> Right, should have been more careful when copying the code from above
>>>>
>>>>>>
>>>>>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
>>>>>> +                                        __be32 *reg, int *nr_cells,
>>>>>> +                                        int addrcells, int sizecells)
>>>>>> +{
>>>>>> +    const struct membanks *mem = &kinfo->shm_mem.common;
>>>>>> +    unsigned int i;
>>>>>> +    __be32 *cells;
>>>>>> +
>>>>>> +    BUG_ON(!nr_cells || !reg);
>>>>>> +
>>>>>> +    cells = &reg[*nr_cells];
>>>>>> +    for ( i = 0; i < mem->nr_banks; i++ )
>>>>>> +    {
>>>>>> +        u64 start = mem->bank[i].start;
>>>>> ditto
>>>>
>>>> Will fix, here paddr_t should be ok isn’t it?
>>> yes
>>>>
>>>>>
>>>>> Rest LGTM:
>>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>>
>>>> Thanks, I will send the next one shortly.
>>> I don't think there is a need to respin the whole series just for these fixes.
>>> You should wait for the committers opinion.
>>
>> AFAICT, there are multiple changes requested in various line. So I would rather prefer if this is respinned.
>>
>> If this is the only patch that requires to change. You could send a new one in reply-to this patch. I think b4 is clever enough to pick up the new version in that case.
> 
> All the other patches are already reviewed by Michal, if you agree with his review it’s ok for me to respin only this one

I didn't review the patch in details. But I agree with his comments on it.

Cheers,
Julien Grall April 24, 2024, 10:19 a.m. UTC | #7
Hi,

On 22/04/2024 11:24, Julien Grall wrote:
> Hi,
> 
> On 22/04/2024 10:26, Michal Orzel wrote:
>>
>>
>> On 22/04/2024 10:07, Luca Fancellu wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>>>> +    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells 
>>>>> += reg_size )
>>>>> +    {
>>>>> +        u64 start = dt_read_number(cells, addrcells);
>>>> We should no longer use Linux derived types like u64. Use uint64_t.
>>>>
>>>>> +        u64 size = dt_read_number(cells + addrcells, sizecells);
>>>>> +
>>>>> +        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>>>>> +                   i, start, start + size);
>>>> i is unsigned so the correct format specifier should be %u
>>>
>>> Right, should have been more careful when copying the code from above
>>>
>>>>>
>>>>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info 
>>>>> *kinfo,
>>>>> +                                        __be32 *reg, int *nr_cells,
>>>>> +                                        int addrcells, int sizecells)
>>>>> +{
>>>>> +    const struct membanks *mem = &kinfo->shm_mem.common;
>>>>> +    unsigned int i;
>>>>> +    __be32 *cells;
>>>>> +
>>>>> +    BUG_ON(!nr_cells || !reg);
>>>>> +
>>>>> +    cells = &reg[*nr_cells];
>>>>> +    for ( i = 0; i < mem->nr_banks; i++ )
>>>>> +    {
>>>>> +        u64 start = mem->bank[i].start;
>>>> ditto
>>>
>>> Will fix, here paddr_t should be ok isn’t it?
>> yes
>>
>>>
>>>>
>>>> Rest LGTM:
>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>
>>> Thanks, I will send the next one shortly.
>> I don't think there is a need to respin the whole series just for 
>> these fixes.
>> You should wait for the committers opinion.
> 
> AFAICT, there are multiple changes requested in various line. So I would 
> rather prefer if this is respinned.
> 
> If this is the only patch that requires to change. You could send a new 
> one in reply-to this patch. I think b4 is clever enough to pick up the 
> new version in that case.

I was wrong. b4 didn't picked up the new version. Anyway, I have applied 
the new patch and send to gitlab for testing. I will merge it once it 
passes.

Cheers,
Luca Fancellu April 24, 2024, 10:25 a.m. UTC | #8
Hi Julien,

>>>> 
>>>>> 
>>>>> Rest LGTM:
>>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>> 
>>>> Thanks, I will send the next one shortly.
>>> I don't think there is a need to respin the whole series just for these fixes.
>>> You should wait for the committers opinion.
>> AFAICT, there are multiple changes requested in various line. So I would rather prefer if this is respinned.
>> If this is the only patch that requires to change. You could send a new one in reply-to this patch. I think b4 is clever enough to pick up the new version in that case.
> 
> I was wrong. b4 didn't picked up the new version. Anyway, I have applied the new patch and send to gitlab for testing. I will merge it once it passes.

Thanks a lot for that!

Cheers,
Luca
diff mbox series

Patch

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 51cf03221d56..74f053c242f4 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -642,7 +642,7 @@  static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     if ( ret )
         goto err;
 
-    ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+    ret = make_memory_node(kinfo, addrcells, sizecells,
                            kernel_info_get_mem(kinfo));
     if ( ret )
         goto err;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 68532ddc084c..15f888169c4e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -756,15 +756,14 @@  int __init domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit)
     return fdt_begin_node(fdt, buf);
 }
 
-int __init make_memory_node(const struct domain *d,
-                            void *fdt,
-                            int addrcells, int sizecells,
-                            const struct membanks *mem)
+int __init make_memory_node(const struct kernel_info *kinfo, int addrcells,
+                            int sizecells, const struct membanks *mem)
 {
+    void *fdt = kinfo->fdt;
     unsigned int i;
     int res, reg_size = addrcells + sizecells;
     int nr_cells = 0;
-    __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
+    __be32 reg[DT_MEM_NODE_REG_RANGE_SIZE];
     __be32 *cells;
 
     if ( mem->nr_banks == 0 )
@@ -797,14 +796,28 @@  int __init make_memory_node(const struct domain *d,
         if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN )
             continue;
 
-        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
-                   i, start, start + size);
-
         nr_cells += reg_size;
         BUG_ON(nr_cells >= ARRAY_SIZE(reg));
         dt_child_set_range(&cells, addrcells, sizecells, start, size);
     }
 
+    /*
+     * static shared memory banks need to be listed as /memory node, so when
+     * this function is handling the normal memory, add the banks.
+     */
+    if ( mem == kernel_info_get_mem(kinfo) )
+        shm_mem_node_fill_reg_range(kinfo, reg, &nr_cells, addrcells,
+                                    sizecells);
+
+    for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
+    {
+        u64 start = dt_read_number(cells, addrcells);
+        u64 size = dt_read_number(cells + addrcells, sizecells);
+
+        dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
+                   i, start, start + size);
+    }
+
     dt_dprintk("(reg size %d, nr cells %d)\n", reg_size, nr_cells);
 
     res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg));
@@ -1783,7 +1796,7 @@  static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
         if ( res )
             return res;
 
-        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+        res = make_memory_node(kinfo, addrcells, sizecells,
                                kernel_info_get_mem(kinfo));
         if ( res )
             return res;
@@ -1794,8 +1807,7 @@  static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
          */
         if ( reserved_mem->nr_banks > 0 )
         {
-            res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
-                                   reserved_mem);
+            res = make_memory_node(kinfo, addrcells, sizecells, reserved_mem);
             if ( res )
                 return res;
         }
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index 026d975da28e..45936212ca21 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -14,7 +14,7 @@  int make_chosen_node(const struct kernel_info *kinfo);
 int make_cpus_node(const struct domain *d, void *fdt);
 int make_hypervisor_node(struct domain *d, const struct kernel_info *kinfo,
                          int addrcells, int sizecells);
-int make_memory_node(const struct domain *d, void *fdt, int addrcells,
+int make_memory_node(const struct kernel_info *kinfo, int addrcells,
                      int sizecells, const struct membanks *mem);
 int make_psci_node(void *fdt);
 int make_timer_node(const struct kernel_info *kinfo);
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 7495a91e7a31..3b6569e5703f 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -3,10 +3,15 @@ 
 #ifndef __ASM_STATIC_SHMEM_H_
 #define __ASM_STATIC_SHMEM_H_
 
+#include <xen/types.h>
 #include <asm/kernel.h>
+#include <asm/setup.h>
 
 #ifdef CONFIG_STATIC_SHM
 
+/* Worst case /memory node reg element: (addrcells + sizecells) */
+#define DT_MEM_NODE_REG_RANGE_SIZE ((NR_MEM_BANKS + NR_SHMEM_BANKS) * 4)
+
 int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
                           int sizecells);
 
@@ -37,8 +42,14 @@  int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
 int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
                               int sizecells);
 
+void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, __be32 *reg,
+                                 int *nr_cells, int addrcells, int sizecells);
+
 #else /* !CONFIG_STATIC_SHM */
 
+/* Worst case /memory node reg element: (addrcells + sizecells) */
+#define DT_MEM_NODE_REG_RANGE_SIZE (NR_MEM_BANKS * 4)
+
 static inline int make_resv_memory_node(const struct kernel_info *kinfo,
                                         int addrcells, int sizecells)
 {
@@ -86,6 +97,10 @@  static inline int make_shm_resv_memory_node(const struct kernel_info *kinfo,
     return 0;
 }
 
+static inline void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
+                                               __be32 *reg, int *nr_cells,
+                                               int addrcells, int sizecells) {};
+
 #endif /* CONFIG_STATIC_SHM */
 
 #endif /* __ASM_STATIC_SHMEM_H_ */
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index c85f60dd1bf7..07c93a820508 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -1,5 +1,6 @@ 
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/rangeset.h>
 #include <xen/sched.h>
@@ -668,6 +669,28 @@  int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
     return res;
 }
 
+void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
+                                        __be32 *reg, int *nr_cells,
+                                        int addrcells, int sizecells)
+{
+    const struct membanks *mem = &kinfo->shm_mem.common;
+    unsigned int i;
+    __be32 *cells;
+
+    BUG_ON(!nr_cells || !reg);
+
+    cells = &reg[*nr_cells];
+    for ( i = 0; i < mem->nr_banks; i++ )
+    {
+        u64 start = mem->bank[i].start;
+        u64 size = mem->bank[i].size;
+
+        *nr_cells += addrcells + sizecells;
+        BUG_ON(*nr_cells >= DT_MEM_NODE_REG_RANGE_SIZE);
+        dt_child_set_range(&cells, addrcells, sizecells, start, size);
+    }
+}
+
 /*
  * Local variables:
  * mode: C