Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
/memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
but forgot to update the 'struct kernel_info' initialiser, while
it doesn't lead to failures because the field is not currently
used while managing kernel_info structures, it's good to have it
for completeness.

Fixes: a14593e3995a ("xen/device-tree: Allow region overlapping with /memreserve/ ranges")
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/include/asm/kernel.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 7e6e3c82a477..de3f945ae54c 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -92,7 +92,9 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
 }
 
 #ifdef CONFIG_STATIC_SHM
-#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_SHMEM_BANKS,
+#define KERNEL_INFO_SHM_MEM_INIT                \
+    .shm_mem.common.max_banks = NR_SHMEM_BANKS, \
+    .shm_mem.common.type = STATIC_SHARED_MEMORY,
 #else
 #define KERNEL_INFO_SHM_MEM_INIT
 #endif
@@ -100,6 +102,7 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
 #define KERNEL_INFO_INIT                        \
 {                                               \
     .mem.common.max_banks = NR_MEM_BANKS,       \
+    .mem.common.type = MEMORY,                  \
     KERNEL_INFO_SHM_MEM_INIT                    \
 }
 
-- 
2.34.1

On 20/12/2024 16:19, Luca Fancellu wrote:
> 
> 
> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
> but forgot to update the 'struct kernel_info' initialiser, while
> it doesn't lead to failures because the field is not currently
> used while managing kernel_info structures, it's good to have it
> for completeness.
The last part "good to have it" does not sound like we need a Fixes tag.
I'm in two minds here. At the moment we do have some consistency because
we use and therefore initialize .type member only for bootinfo related structures.

You suggest to expand this also to kernel_info. But what about other places using
struct membanks that, after all, is a useful generic structure? One example you can find
is static struct shm_heap_banks in static-shmem.c for which you do not set a type. Another
example is allocate_memory() or find_host_extended_regions() where we have to convert gnttab
region into struct membanks and there is no need to use the type at all. So, do you suggest
we should initialize (explicitly) .type only for *meminfo based structures or all the structures
using struct membanks?

> 
> Fixes: a14593e3995a ("xen/device-tree: Allow region overlapping with /memreserve/ ranges")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/arch/arm/include/asm/kernel.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> index 7e6e3c82a477..de3f945ae54c 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -92,7 +92,9 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
>  }
> 
>  #ifdef CONFIG_STATIC_SHM
> -#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_SHMEM_BANKS,
> +#define KERNEL_INFO_SHM_MEM_INIT                \
> +    .shm_mem.common.max_banks = NR_SHMEM_BANKS, \
> +    .shm_mem.common.type = STATIC_SHARED_MEMORY,
>  #else
>  #define KERNEL_INFO_SHM_MEM_INIT
>  #endif
> @@ -100,6 +102,7 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
>  #define KERNEL_INFO_INIT                        \
>  {                                               \
>      .mem.common.max_banks = NR_MEM_BANKS,       \
> +    .mem.common.type = MEMORY,                  \
>      KERNEL_INFO_SHM_MEM_INIT                    \
>  }
> 
> --
> 2.34.1
> 

~Michal

Hi Michal,

On 23/12/2024 07:58, Michal Orzel wrote:
> 
> 
> On 20/12/2024 16:19, Luca Fancellu wrote:
>>
>>
>> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
>> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
>> but forgot to update the 'struct kernel_info' initialiser, while
>> it doesn't lead to failures because the field is not currently
>> used while managing kernel_info structures, it's good to have it
>> for completeness.
> The last part "good to have it" does not sound like we need a Fixes tag.

Reading the discussion, it sounds like ".type" is not always set and 
this is not properly documented. This means that in the future we may 
write a patch that requires to use ".type" and needs backported.

But this would depend on this patch which was not tagged appropriately. 
Therefore, I would argue it needs a fixes tag (even though this is a 
latent bug) or at least a backport request.

Cheers,

-- 
Julien Grall

On 23/12/2024 11:06, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 23/12/2024 07:58, Michal Orzel wrote:
>>
>>
>> On 20/12/2024 16:19, Luca Fancellu wrote:
>>>
>>>
>>> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
>>> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
>>> but forgot to update the 'struct kernel_info' initialiser, while
>>> it doesn't lead to failures because the field is not currently
>>> used while managing kernel_info structures, it's good to have it
>>> for completeness.
>> The last part "good to have it" does not sound like we need a Fixes tag.
> 
> Reading the discussion, it sounds like ".type" is not always set and
> this is not properly documented. This means that in the future we may
> write a patch that requires to use ".type" and needs backported.
> 
> But this would depend on this patch which was not tagged appropriately.
> Therefore, I would argue it needs a fixes tag (even though this is a
> latent bug) or at least a backport request.
Ok, sounds good. But first, let's settle on what we actually want to do here, if at all.
I would be ok with just documenting that .type field is for now only used with bootinfo
related structures (after all, today it's used only in one bootfdt function). Setting
explicitly a type for structures not requiring it, does not seem beneficial for me.

~Michal

Hi,

On 23/12/2024 10:42, Michal Orzel wrote:
> 
> 
> On 23/12/2024 11:06, Julien Grall wrote:
>>
>>
>> Hi Michal,
>>
>> On 23/12/2024 07:58, Michal Orzel wrote:
>>>
>>>
>>> On 20/12/2024 16:19, Luca Fancellu wrote:
>>>>
>>>>
>>>> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
>>>> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
>>>> but forgot to update the 'struct kernel_info' initialiser, while
>>>> it doesn't lead to failures because the field is not currently
>>>> used while managing kernel_info structures, it's good to have it
>>>> for completeness.
>>> The last part "good to have it" does not sound like we need a Fixes tag.
>>
>> Reading the discussion, it sounds like ".type" is not always set and
>> this is not properly documented. This means that in the future we may
>> write a patch that requires to use ".type" and needs backported.
>>
>> But this would depend on this patch which was not tagged appropriately.
>> Therefore, I would argue it needs a fixes tag (even though this is a
>> latent bug) or at least a backport request.
> Setting explicitly a type for structures not requiring it, does not seem beneficial for me.

I have to disagree. If we set type everywhere, then the developer 
doesn't need to think whether ".type" is garbagge or not.

Cheers,

-- 
Julien Grall

On 23/12/2024 11:45, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 23/12/2024 10:42, Michal Orzel wrote:
>>
>>
>> On 23/12/2024 11:06, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 23/12/2024 07:58, Michal Orzel wrote:
>>>>
>>>>
>>>> On 20/12/2024 16:19, Luca Fancellu wrote:
>>>>>
>>>>>
>>>>> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
>>>>> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
>>>>> but forgot to update the 'struct kernel_info' initialiser, while
>>>>> it doesn't lead to failures because the field is not currently
>>>>> used while managing kernel_info structures, it's good to have it
>>>>> for completeness.
>>>> The last part "good to have it" does not sound like we need a Fixes tag.
>>>
>>> Reading the discussion, it sounds like ".type" is not always set and
>>> this is not properly documented. This means that in the future we may
>>> write a patch that requires to use ".type" and needs backported.
>>>
>>> But this would depend on this patch which was not tagged appropriately.
>>> Therefore, I would argue it needs a fixes tag (even though this is a
>>> latent bug) or at least a backport request.
>> Setting explicitly a type for structures not requiring it, does not seem beneficial for me.
> 
> I have to disagree. If we set type everywhere, then the developer
> doesn't need to think whether ".type" is garbagge or not.
I see, I have nothing against this proposal. But in that case I do think that the same needs to apply to max_banks
member.

~Michal

Hi Michal,

On 23/12/2024 11:43, Michal Orzel wrote:
> 
> 
> On 23/12/2024 11:45, Julien Grall wrote:
>>
>>
>> Hi,
>>
>> On 23/12/2024 10:42, Michal Orzel wrote:
>>>
>>>
>>> On 23/12/2024 11:06, Julien Grall wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>> On 23/12/2024 07:58, Michal Orzel wrote:
>>>>>
>>>>>
>>>>> On 20/12/2024 16:19, Luca Fancellu wrote:
>>>>>>
>>>>>>
>>>>>> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
>>>>>> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
>>>>>> but forgot to update the 'struct kernel_info' initialiser, while
>>>>>> it doesn't lead to failures because the field is not currently
>>>>>> used while managing kernel_info structures, it's good to have it
>>>>>> for completeness.
>>>>> The last part "good to have it" does not sound like we need a Fixes tag.
>>>>
>>>> Reading the discussion, it sounds like ".type" is not always set and
>>>> this is not properly documented. This means that in the future we may
>>>> write a patch that requires to use ".type" and needs backported.
>>>>
>>>> But this would depend on this patch which was not tagged appropriately.
>>>> Therefore, I would argue it needs a fixes tag (even though this is a
>>>> latent bug) or at least a backport request.
>>> Setting explicitly a type for structures not requiring it, does not seem beneficial for me.
>>
>> I have to disagree. If we set type everywhere, then the developer
>> doesn't need to think whether ".type" is garbagge or not.
> I see, I have nothing against this proposal. But in that case I do think that the same needs to apply to max_banks
> member.

I am fine with that.

Cheers,

-- 
Julien Grall

Hi Julien and Michal,

> On 23 Dec 2024, at 10:45, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 23/12/2024 10:42, Michal Orzel wrote:
>> On 23/12/2024 11:06, Julien Grall wrote:
>>> 
>>> 
>>> Hi Michal,
>>> 
>>> On 23/12/2024 07:58, Michal Orzel wrote:
>>>> 
>>>> 
>>>> On 20/12/2024 16:19, Luca Fancellu wrote:
>>>>> 
>>>>> 
>>>>> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
>>>>> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
>>>>> but forgot to update the 'struct kernel_info' initialiser, while
>>>>> it doesn't lead to failures because the field is not currently
>>>>> used while managing kernel_info structures, it's good to have it
>>>>> for completeness.
>>>> The last part "good to have it" does not sound like we need a Fixes tag.
>>> 
>>> Reading the discussion, it sounds like ".type" is not always set and
>>> this is not properly documented. This means that in the future we may
>>> write a patch that requires to use ".type" and needs backported.
>>> 
>>> But this would depend on this patch which was not tagged appropriately.
>>> Therefore, I would argue it needs a fixes tag (even though this is a
>>> latent bug) or at least a backport request.
>> Setting explicitly a type for structures not requiring it, does not seem beneficial for me.
> 
> I have to disagree. If we set type everywhere, then the developer doesn't need to think whether ".type" is garbagge or not.

So, my thought was to at least initialise it on the structures that goes around in the codebase,
gnttab in find_host_extended_regions and shm_heap_banks in static-shmem.c usage are less
spreaded.

However I have no objection to always initialise them all, so that anyone sending patches that
use them, don’t need to think if the field is initialised or not.

I’m currently on leave, is it ok to wait until new year if any change is required?

Cheers,
Luca

On 23/12/2024 11:52, Luca Fancellu wrote:
> 
> 
> Hi Julien and Michal,
> 
>> On 23 Dec 2024, at 10:45, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 23/12/2024 10:42, Michal Orzel wrote:
>>> On 23/12/2024 11:06, Julien Grall wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>> On 23/12/2024 07:58, Michal Orzel wrote:
>>>>>
>>>>>
>>>>> On 20/12/2024 16:19, Luca Fancellu wrote:
>>>>>>
>>>>>>
>>>>>> Commit a14593e3995a ("xen/device-tree: Allow region overlapping with
>>>>>> /memreserve/ ranges") introduced a type in the 'struct membanks_hdr'
>>>>>> but forgot to update the 'struct kernel_info' initialiser, while
>>>>>> it doesn't lead to failures because the field is not currently
>>>>>> used while managing kernel_info structures, it's good to have it
>>>>>> for completeness.
>>>>> The last part "good to have it" does not sound like we need a Fixes tag.
>>>>
>>>> Reading the discussion, it sounds like ".type" is not always set and
>>>> this is not properly documented. This means that in the future we may
>>>> write a patch that requires to use ".type" and needs backported.
>>>>
>>>> But this would depend on this patch which was not tagged appropriately.
>>>> Therefore, I would argue it needs a fixes tag (even though this is a
>>>> latent bug) or at least a backport request.
>>> Setting explicitly a type for structures not requiring it, does not seem beneficial for me.
>>
>> I have to disagree. If we set type everywhere, then the developer doesn't need to think whether ".type" is garbagge or not.
> 
> So, my thought was to at least initialise it on the structures that goes around in the codebase,
> gnttab in find_host_extended_regions and shm_heap_banks in static-shmem.c usage are less
> spreaded.
> 
> However I have no objection to always initialise them all, so that anyone sending patches that
> use them, don’t need to think if the field is initialised or not.
> 
> I’m currently on leave, is it ok to wait until new year if any change is required?
Hi Luca, yes, please send a new revision once you're back.

~Michal