[PATCH 0/2] Fix MISRA regression about flexible array member not at the end

[PATCH 0/2] Fix MISRA regression about flexible array member not at the end

[Luca Fancellu]

This small serie is fixing a MISRA regression on the R1.1 due to the
introduction of a new interface that uses a flexible member array [1].

I've splitted the serie in two in order to keep the Linux imported code on a
single commit, the imported macro is later used in the second patch of the
serie.

[1] https://lore.kernel.org/all/8be082b6d22d61c0b14910680d3833a7@bugseng.com/

Luca Fancellu (2):
  xen/kernel.h: Import __struct_group from Linux
  xen/arm: Fix MISRA regression on R1.1, flexible array member not at
    the end

 xen/arch/arm/acpi/domain_build.c        |  2 +-
 xen/arch/arm/domain_build.c             |  6 +++---
 xen/arch/arm/include/asm/kernel.h       | 11 ++++++++++-
 xen/arch/arm/include/asm/setup.h        | 18 ++++++++++--------
 xen/arch/arm/include/asm/static-shmem.h | 12 ++++++++++++
 xen/arch/arm/static-shmem.c             | 19 +++++++++----------
 xen/include/xen/kernel.h                | 21 +++++++++++++++++++++
 7 files changed, 66 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Luca Fancellu]

Import __struct_group from Linux, commit 50d7bd38c3aa
("stddef: Introduce struct_group() helper macro"), in order to
allow the access through the anonymous structure to the members
without having to write also the name, e.g:

struct foo {
    int one;
    struct {
        int two;
        int three, four;
    } thing;
    int five;
};

would become:

struct foo {
    int one;
    __struct_group(/* None */, thing, /* None */,
        int two;
        int three, four;
    );
    int five;
};

Allowing the users of this structure to access the .thing members by
using .two/.three/.four on the struct foo.
This construct will become useful in order to have some generalized
interfaces that shares some common members.

Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 50d7bd38c3aa
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/include/xen/kernel.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index bb6b0f38912d..bc2440b5f96e 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -54,6 +54,27 @@
         typeof_field(type, member) *__mptr = (ptr);             \
         (type *)( (char *)__mptr - offsetof(type,member) );})
 
+/**
+ * __struct_group() - Create a mirrored named and anonyomous struct
+ *
+ * @TAG: The tag name for the named sub-struct (usually empty)
+ * @NAME: The identifier name of the mirrored sub-struct
+ * @ATTRS: Any struct attributes (usually empty)
+ * @MEMBERS: The member declarations for the mirrored structs
+ *
+ * Used to create an anonymous union of two structs with identical layout
+ * and size: one anonymous and one named. The former's members can be used
+ * normally without sub-struct naming, and the latter can be used to
+ * reason about the start, end, and size of the group of struct members.
+ * The named struct can also be explicitly tagged for layer reuse, as well
+ * as both having struct attributes appended.
+ */
+#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
+    union { \
+        struct { MEMBERS } ATTRS; \
+        struct TAG { MEMBERS } ATTRS NAME; \
+    } ATTRS
+
 /*
  * Check at compile time that something is of a particular type.
  * Always evaluates to 1 so you may use it easily in comparisons.
-- 
2.34.1

Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Jan Beulich]

On 30.04.2024 13:09, Luca Fancellu wrote:
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -54,6 +54,27 @@
>          typeof_field(type, member) *__mptr = (ptr);             \
>          (type *)( (char *)__mptr - offsetof(type,member) );})
>  
> +/**
> + * __struct_group() - Create a mirrored named and anonyomous struct
> + *
> + * @TAG: The tag name for the named sub-struct (usually empty)
> + * @NAME: The identifier name of the mirrored sub-struct
> + * @ATTRS: Any struct attributes (usually empty)
> + * @MEMBERS: The member declarations for the mirrored structs
> + *
> + * Used to create an anonymous union of two structs with identical layout
> + * and size: one anonymous and one named. The former's members can be used
> + * normally without sub-struct naming, and the latter can be used to
> + * reason about the start, end, and size of the group of struct members.
> + * The named struct can also be explicitly tagged for layer reuse, as well
> + * as both having struct attributes appended.
> + */
> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
> +    union { \
> +        struct { MEMBERS } ATTRS; \
> +        struct TAG { MEMBERS } ATTRS NAME; \
> +    } ATTRS

Besides my hesitance towards having this construct, can you explain why
ATTR needs using 3 times, i.e. also on the wrapping union?

Jan

Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Luca Fancellu]

Hi Jan,

> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 30.04.2024 13:09, Luca Fancellu wrote:
>> --- a/xen/include/xen/kernel.h
>> +++ b/xen/include/xen/kernel.h
>> @@ -54,6 +54,27 @@
>>         typeof_field(type, member) *__mptr = (ptr);             \
>>         (type *)( (char *)__mptr - offsetof(type,member) );})
>> 
>> +/**
>> + * __struct_group() - Create a mirrored named and anonyomous struct
>> + *
>> + * @TAG: The tag name for the named sub-struct (usually empty)
>> + * @NAME: The identifier name of the mirrored sub-struct
>> + * @ATTRS: Any struct attributes (usually empty)
>> + * @MEMBERS: The member declarations for the mirrored structs
>> + *
>> + * Used to create an anonymous union of two structs with identical layout
>> + * and size: one anonymous and one named. The former's members can be used
>> + * normally without sub-struct naming, and the latter can be used to
>> + * reason about the start, end, and size of the group of struct members.
>> + * The named struct can also be explicitly tagged for layer reuse, as well
>> + * as both having struct attributes appended.
>> + */
>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
>> +    union { \
>> +        struct { MEMBERS } ATTRS; \
>> +        struct TAG { MEMBERS } ATTRS NAME; \
>> +    } ATTRS
> 
> Besides my hesitance towards having this construct, can you explain why
> ATTR needs using 3 times, i.e. also on the wrapping union?

The original commit didn’t have the third ATTRS, but afterwards it was introduced due to
this:

https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045

Now, I have to say that for the Origin tag I used the SHA of the commit introducing the macro
and the SHA doing this modification is different, how are these cases handled?

Cheers,
Luca

Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Jan Beulich]

On 01.05.2024 08:54, Luca Fancellu wrote:
>> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote:
>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>> --- a/xen/include/xen/kernel.h
>>> +++ b/xen/include/xen/kernel.h
>>> @@ -54,6 +54,27 @@
>>>         typeof_field(type, member) *__mptr = (ptr);             \
>>>         (type *)( (char *)__mptr - offsetof(type,member) );})
>>>
>>> +/**
>>> + * __struct_group() - Create a mirrored named and anonyomous struct
>>> + *
>>> + * @TAG: The tag name for the named sub-struct (usually empty)
>>> + * @NAME: The identifier name of the mirrored sub-struct
>>> + * @ATTRS: Any struct attributes (usually empty)
>>> + * @MEMBERS: The member declarations for the mirrored structs
>>> + *
>>> + * Used to create an anonymous union of two structs with identical layout
>>> + * and size: one anonymous and one named. The former's members can be used
>>> + * normally without sub-struct naming, and the latter can be used to
>>> + * reason about the start, end, and size of the group of struct members.
>>> + * The named struct can also be explicitly tagged for layer reuse, as well
>>> + * as both having struct attributes appended.
>>> + */
>>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
>>> +    union { \
>>> +        struct { MEMBERS } ATTRS; \
>>> +        struct TAG { MEMBERS } ATTRS NAME; \
>>> +    } ATTRS
>>
>> Besides my hesitance towards having this construct, can you explain why
>> ATTR needs using 3 times, i.e. also on the wrapping union?
> 
> The original commit didn’t have the third ATTRS, but afterwards it was introduced due to
> this:
> 
> https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045

Hmm. Since generally packing propagates to containing compound types, I'd
say this cannot go without a code comment, or at the very least a mention
in the description. But: Do we use this old ABI in Xen at all? IOW can we
get away without this 3rd instance?

> Now, I have to say that for the Origin tag I used the SHA of the commit introducing the macro
> and the SHA doing this modification is different, how are these cases handled?

I'd say the hash of the original commit is enough even if the 3rd instance
needs keeping for whatever reason.

Jan

Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Luca Fancellu]

> On 2 May 2024, at 07:09, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 01.05.2024 08:54, Luca Fancellu wrote:
>>> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>> --- a/xen/include/xen/kernel.h
>>>> +++ b/xen/include/xen/kernel.h
>>>> @@ -54,6 +54,27 @@
>>>>        typeof_field(type, member) *__mptr = (ptr);             \
>>>>        (type *)( (char *)__mptr - offsetof(type,member) );})
>>>> 
>>>> +/**
>>>> + * __struct_group() - Create a mirrored named and anonyomous struct
>>>> + *
>>>> + * @TAG: The tag name for the named sub-struct (usually empty)
>>>> + * @NAME: The identifier name of the mirrored sub-struct
>>>> + * @ATTRS: Any struct attributes (usually empty)
>>>> + * @MEMBERS: The member declarations for the mirrored structs
>>>> + *
>>>> + * Used to create an anonymous union of two structs with identical layout
>>>> + * and size: one anonymous and one named. The former's members can be used
>>>> + * normally without sub-struct naming, and the latter can be used to
>>>> + * reason about the start, end, and size of the group of struct members.
>>>> + * The named struct can also be explicitly tagged for layer reuse, as well
>>>> + * as both having struct attributes appended.
>>>> + */
>>>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
>>>> +    union { \
>>>> +        struct { MEMBERS } ATTRS; \
>>>> +        struct TAG { MEMBERS } ATTRS NAME; \
>>>> +    } ATTRS
>>> 
>>> Besides my hesitance towards having this construct, can you explain why
>>> ATTR needs using 3 times, i.e. also on the wrapping union?
>> 
>> The original commit didn’t have the third ATTRS, but afterwards it was introduced due to
>> this:
>> 
>> https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045
> 
> Hmm. Since generally packing propagates to containing compound types, I'd
> say this cannot go without a code comment, or at the very least a mention
> in the description. But: Do we use this old ABI in Xen at all? IOW can we
> get away without this 3rd instance?

Yes, I think it won’t be a problem for Xen, is it something that can be done on commit?
Anyway in case of comments on the second patch, I’ll push the change also for this one.

Cheers,
Luca

Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Jan Beulich]

On 02.05.2024 08:23, Luca Fancellu wrote:
> 
> 
>> On 2 May 2024, at 07:09, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 01.05.2024 08:54, Luca Fancellu wrote:
>>>> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>>> --- a/xen/include/xen/kernel.h
>>>>> +++ b/xen/include/xen/kernel.h
>>>>> @@ -54,6 +54,27 @@
>>>>>        typeof_field(type, member) *__mptr = (ptr);             \
>>>>>        (type *)( (char *)__mptr - offsetof(type,member) );})
>>>>>
>>>>> +/**
>>>>> + * __struct_group() - Create a mirrored named and anonyomous struct
>>>>> + *
>>>>> + * @TAG: The tag name for the named sub-struct (usually empty)
>>>>> + * @NAME: The identifier name of the mirrored sub-struct
>>>>> + * @ATTRS: Any struct attributes (usually empty)
>>>>> + * @MEMBERS: The member declarations for the mirrored structs
>>>>> + *
>>>>> + * Used to create an anonymous union of two structs with identical layout
>>>>> + * and size: one anonymous and one named. The former's members can be used
>>>>> + * normally without sub-struct naming, and the latter can be used to
>>>>> + * reason about the start, end, and size of the group of struct members.
>>>>> + * The named struct can also be explicitly tagged for layer reuse, as well
>>>>> + * as both having struct attributes appended.
>>>>> + */
>>>>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
>>>>> +    union { \
>>>>> +        struct { MEMBERS } ATTRS; \
>>>>> +        struct TAG { MEMBERS } ATTRS NAME; \
>>>>> +    } ATTRS
>>>>
>>>> Besides my hesitance towards having this construct, can you explain why
>>>> ATTR needs using 3 times, i.e. also on the wrapping union?
>>>
>>> The original commit didn’t have the third ATTRS, but afterwards it was introduced due to
>>> this:
>>>
>>> https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045
>>
>> Hmm. Since generally packing propagates to containing compound types, I'd
>> say this cannot go without a code comment, or at the very least a mention
>> in the description. But: Do we use this old ABI in Xen at all? IOW can we
>> get away without this 3rd instance?
> 
> Yes, I think it won’t be a problem for Xen, is it something that can be done on commit?

Don't know, maybe. First you need an ack, and I remain unconvinced that we
actually need this construct.

Jan

> Anyway in case of comments on the second patch, I’ll push the change also for this one.
> 
> Cheers,
> Luca
> 

Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux

[Stefano Stabellini]

On Tue, 30 Apr 2024, Luca Fancellu wrote:
> Import __struct_group from Linux, commit 50d7bd38c3aa
> ("stddef: Introduce struct_group() helper macro"), in order to
> allow the access through the anonymous structure to the members
> without having to write also the name, e.g:
> 
> struct foo {
>     int one;
>     struct {
>         int two;
>         int three, four;
>     } thing;
>     int five;
> };
> 
> would become:
> 
> struct foo {
>     int one;
>     __struct_group(/* None */, thing, /* None */,
>         int two;
>         int three, four;
>     );
>     int five;
> };
> 
> Allowing the users of this structure to access the .thing members by
> using .two/.three/.four on the struct foo.
> This construct will become useful in order to have some generalized
> interfaces that shares some common members.
> 
> Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 50d7bd38c3aa
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

[PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Luca Fancellu]

Commit 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory
bank structures") introduced a MISRA regression for Rule 1.1 because a
flexible array member is introduced in the middle of a struct, furthermore
this is using a GCC extension that is going to be deprecated in GCC 14 and
a warning to identify such cases will be present
(-Wflex-array-member-not-at-end) to identify such cases.

In order to fix this issue, use the macro __struct_group to create a
structure 'struct membanks_hdr' which will hold the common data among
structures using the 'struct membanks' interface.

Modify the 'struct shared_meminfo' and 'struct meminfo' to use this new
structure, effectively removing the flexible array member from the middle
of the structure and modify the code accessing the .common field to use
the macro container_of to maintain the functionality of the interface.

Given this change, container_of needs to be supplied with a type and so
the macro 'kernel_info_get_mem' inside arm/include/asm/kernel.h can't be
an option since it uses const and non-const types for struct membanks, so
introduce two static inline, one of which will keep the const qualifier.

Given the complexity of the interface, which carries a lot of benefit but
on the other hand could be prone to developer confusion if the access is
open-coded, introduce two static inline helper for the
'struct kernel_info' .shm_mem member and get rid the open-coding
shm_mem.common access.

Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures")
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/arch/arm/acpi/domain_build.c        |  2 +-
 xen/arch/arm/domain_build.c             |  6 +++---
 xen/arch/arm/include/asm/kernel.h       | 11 ++++++++++-
 xen/arch/arm/include/asm/setup.h        | 18 ++++++++++--------
 xen/arch/arm/include/asm/static-shmem.h | 12 ++++++++++++
 xen/arch/arm/static-shmem.c             | 19 +++++++++----------
 6 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
index ed895dd8f926..2ce75543d004 100644
--- a/xen/arch/arm/acpi/domain_build.c
+++ b/xen/arch/arm/acpi/domain_build.c
@@ -451,7 +451,7 @@ static int __init estimate_acpi_efi_size(struct domain *d,
     struct acpi_table_rsdp *rsdp_tbl;
     struct acpi_table_header *table;
 
-    efi_size = estimate_efi_size(kernel_info_get_mem(kinfo)->nr_banks);
+    efi_size = estimate_efi_size(kernel_info_get_mem_const(kinfo)->nr_banks);
 
     acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
     acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 0784e4c5e315..f6550809cfdf 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -805,7 +805,7 @@ int __init make_memory_node(const struct kernel_info *kinfo, int addrcells,
      * 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) )
+    if ( mem == kernel_info_get_mem_const(kinfo) )
         shm_mem_node_fill_reg_range(kinfo, reg, &nr_cells, addrcells,
                                     sizecells);
 
@@ -884,7 +884,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
 {
     const struct membanks *mem = bootinfo_get_mem();
     const struct membanks *mem_banks[] = {
-        kernel_info_get_mem(kinfo),
+        kernel_info_get_mem_const(kinfo),
         bootinfo_get_reserved_mem(),
 #ifdef CONFIG_STATIC_SHM
         bootinfo_get_shmem(),
@@ -1108,7 +1108,7 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
     uint64_t bankend;
     const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
     const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
-    const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo);
+    const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
     int res = -ENOENT;
 
     for ( i = 0; i < GUEST_RAM_BANKS; i++ )
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 16a2bfc01e27..7e6e3c82a477 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -80,7 +80,16 @@ struct kernel_info {
     };
 };
 
-#define kernel_info_get_mem(kinfo) (&(kinfo)->mem.common)
+static inline struct membanks *kernel_info_get_mem(struct kernel_info *kinfo)
+{
+    return container_of(&kinfo->mem.common, struct membanks, common);
+}
+
+static inline const struct membanks *
+kernel_info_get_mem_const(const struct kernel_info *kinfo)
+{
+    return container_of(&kinfo->mem.common, const struct membanks, common);
+}
 
 #ifdef CONFIG_STATIC_SHM
 #define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_SHMEM_BANKS,
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 28fb659fe946..61c15806a7c4 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -64,18 +64,20 @@ struct membank {
 };
 
 struct membanks {
-    unsigned int nr_banks;
-    unsigned int max_banks;
+    __struct_group(membanks_hdr, common, ,
+        unsigned int nr_banks;
+        unsigned int max_banks;
+    );
     struct membank bank[];
 };
 
 struct meminfo {
-    struct membanks common;
+    struct membanks_hdr common;
     struct membank bank[NR_MEM_BANKS];
 };
 
 struct shared_meminfo {
-    struct membanks common;
+    struct membanks_hdr common;
     struct membank bank[NR_SHMEM_BANKS];
     struct shmem_membank_extra extra[NR_SHMEM_BANKS];
 };
@@ -166,25 +168,25 @@ extern domid_t max_init_domid;
 
 static inline struct membanks *bootinfo_get_mem(void)
 {
-    return &bootinfo.mem.common;
+    return container_of(&bootinfo.mem.common, struct membanks, common);
 }
 
 static inline struct membanks *bootinfo_get_reserved_mem(void)
 {
-    return &bootinfo.reserved_mem.common;
+    return container_of(&bootinfo.reserved_mem.common, struct membanks, common);
 }
 
 #ifdef CONFIG_ACPI
 static inline struct membanks *bootinfo_get_acpi(void)
 {
-    return &bootinfo.acpi.common;
+    return container_of(&bootinfo.acpi.common, struct membanks, common);
 }
 #endif
 
 #ifdef CONFIG_STATIC_SHM
 static inline struct membanks *bootinfo_get_shmem(void)
 {
-    return &bootinfo.shmem.common;
+    return container_of(&bootinfo.shmem.common, struct membanks, common);
 }
 
 static inline struct shmem_membank_extra *bootinfo_get_shmem_extra(void)
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 3b6569e5703f..806ee41cfc37 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -45,6 +45,18 @@ int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
 void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, __be32 *reg,
                                  int *nr_cells, int addrcells, int sizecells);
 
+static inline struct membanks *
+kernel_info_get_shm_mem(struct kernel_info *kinfo)
+{
+    return container_of(&kinfo->shm_mem.common, struct membanks, common);
+}
+
+static inline const struct membanks *
+kernel_info_get_shm_mem_const(const struct kernel_info *kinfo)
+{
+    return container_of(&kinfo->shm_mem.common, const struct membanks, common);
+}
+
 #else /* !CONFIG_STATIC_SHM */
 
 /* Worst case /memory node reg element: (addrcells + sizecells) */
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 09f474ec6050..78881dd1d3f7 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -172,16 +172,16 @@ static int __init assign_shared_memory(struct domain *d,
 }
 
 static int __init
-append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start,
+append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
                           paddr_t size, const char *shm_id)
 {
-    struct membanks *shm_mem = &kinfo_shm_mem->common;
+    struct membanks *shm_mem = kernel_info_get_shm_mem(kinfo);
     struct shmem_membank_extra *shm_mem_extra;
 
     if ( shm_mem->nr_banks >= shm_mem->max_banks )
         return -ENOMEM;
 
-    shm_mem_extra = &kinfo_shm_mem->extra[shm_mem->nr_banks];
+    shm_mem_extra = &kinfo->shm_mem.extra[shm_mem->nr_banks];
 
     shm_mem->bank[shm_mem->nr_banks].start = start;
     shm_mem->bank[shm_mem->nr_banks].size = size;
@@ -289,8 +289,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
          * Record static shared memory region info for later setting
          * up shm-node in guest device tree.
          */
-        ret = append_shm_bank_to_domain(&kinfo->shm_mem, gbase, psize,
-                                        shm_id);
+        ret = append_shm_bank_to_domain(kinfo, gbase, psize, shm_id);
         if ( ret )
             return ret;
     }
@@ -301,7 +300,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
 int __init make_shm_resv_memory_node(const struct kernel_info *kinfo,
                                      int addrcells, int sizecells)
 {
-    const struct membanks *mem = &kinfo->shm_mem.common;
+    const struct membanks *mem = kernel_info_get_shm_mem_const(kinfo);
     void *fdt = kinfo->fdt;
     unsigned int i = 0;
     int res = 0;
@@ -517,7 +516,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
 int __init make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
                                  int sizecells)
 {
-    const struct membanks *mem = &kinfo->shm_mem.common;
+    const struct membanks *mem = kernel_info_get_shm_mem_const(kinfo);
     void *fdt = kinfo->fdt;
     int res = 0;
     /* Placeholder for reserved-memory\0 */
@@ -579,7 +578,7 @@ void __init init_sharedmem_pages(void)
 int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
                                     struct rangeset *rangeset)
 {
-    const struct membanks *shm_mem = &kinfo->shm_mem.common;
+    const struct membanks *shm_mem = kernel_info_get_shm_mem_const(kinfo);
     unsigned int i;
 
     /* Remove static shared memory regions */
@@ -607,7 +606,7 @@ int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
 int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
                                      struct membanks *ext_regions)
 {
-    const struct membanks *shm_mem = &kinfo->shm_mem.common;
+    const struct membanks *shm_mem = kernel_info_get_shm_mem_const(kinfo);
     struct rangeset *guest_holes;
     unsigned int i;
     paddr_t start;
@@ -673,7 +672,7 @@ 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;
+    const struct membanks *mem = kernel_info_get_shm_mem_const(kinfo);
     unsigned int i;
     __be32 *cells;
 
-- 
2.34.1

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Jan Beulich]

On 30.04.2024 13:09, Luca Fancellu wrote:
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -64,18 +64,20 @@ struct membank {
>  };
>  
>  struct membanks {
> -    unsigned int nr_banks;
> -    unsigned int max_banks;
> +    __struct_group(membanks_hdr, common, ,
> +        unsigned int nr_banks;
> +        unsigned int max_banks;
> +    );
>      struct membank bank[];
>  };

I'm afraid I can't spot why __struct_group() is needed here. Why would just
one of the two more straightforward

struct membanks {
    struct membanks_hdr {
        unsigned int nr_banks;
        unsigned int max_banks;
    );
    struct membank bank[];
};

struct membanks {
    struct membanks_hdr {
        unsigned int nr_banks;
        unsigned int max_banks;
    ) common;
    struct membank bank[];
};

not do? (Perhaps the latter, seeing that you need the field name in ...

>  struct meminfo {
> -    struct membanks common;
> +    struct membanks_hdr common;
>      struct membank bank[NR_MEM_BANKS];
>  };
>  
>  struct shared_meminfo {
> -    struct membanks common;
> +    struct membanks_hdr common;
>      struct membank bank[NR_SHMEM_BANKS];
>      struct shmem_membank_extra extra[NR_SHMEM_BANKS];
>  };
> @@ -166,25 +168,25 @@ extern domid_t max_init_domid;
>  
>  static inline struct membanks *bootinfo_get_mem(void)
>  {
> -    return &bootinfo.mem.common;
> +    return container_of(&bootinfo.mem.common, struct membanks, common);
>  }

... container_of() uses.)

Jan

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Luca Fancellu]

Hi Jan,

> On 30 Apr 2024, at 12:37, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 30.04.2024 13:09, Luca Fancellu wrote:
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -64,18 +64,20 @@ struct membank {
>> };
>> 
>> struct membanks {
>> -    unsigned int nr_banks;
>> -    unsigned int max_banks;
>> +    __struct_group(membanks_hdr, common, ,
>> +        unsigned int nr_banks;
>> +        unsigned int max_banks;
>> +    );
>>     struct membank bank[];
>> };
> 
> I'm afraid I can't spot why __struct_group() is needed here. Why would just
> one of the two more straightforward
> 
> struct membanks {
>    struct membanks_hdr {
>        unsigned int nr_banks;
>        unsigned int max_banks;
>    );
>    struct membank bank[];
> };
> 

At the first sight I thought this solution could have worked, however GCC brought me back down to earth
remembering me that flexible array members can’t be left alone in an empty structure:

/data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: error: declaration does not declare anything [-Werror]
70 | };
| ^
/data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: error: flexible array member in a struct with no named members
71 | struct membank bank[];
| ^~~~
[...]

Cheers,
Luca

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Jan Beulich]

On 01.05.2024 08:57, Luca Fancellu wrote:
> Hi Jan,
> 
>> On 30 Apr 2024, at 12:37, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>> --- a/xen/arch/arm/include/asm/setup.h
>>> +++ b/xen/arch/arm/include/asm/setup.h
>>> @@ -64,18 +64,20 @@ struct membank {
>>> };
>>>
>>> struct membanks {
>>> -    unsigned int nr_banks;
>>> -    unsigned int max_banks;
>>> +    __struct_group(membanks_hdr, common, ,
>>> +        unsigned int nr_banks;
>>> +        unsigned int max_banks;
>>> +    );
>>>     struct membank bank[];
>>> };
>>
>> I'm afraid I can't spot why __struct_group() is needed here. Why would just
>> one of the two more straightforward
>>
>> struct membanks {
>>    struct membanks_hdr {
>>        unsigned int nr_banks;
>>        unsigned int max_banks;
>>    );
>>    struct membank bank[];
>> };
>>
> 
> At the first sight I thought this solution could have worked, however GCC brought me back down to earth
> remembering me that flexible array members can’t be left alone in an empty structure:
> 
> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: error: declaration does not declare anything [-Werror]
> 70 | };
> | ^
> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: error: flexible array member in a struct with no named members
> 71 | struct membank bank[];
> | ^~~~
> [...]

Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution
to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of
borrowing __struct_group(), we could consider borrowing this as well. Or
open-code it just here, for the time being (perhaps my preference). Yet
it's not clear to me that doing so will actually be enough to make things
work for you.

Jan

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Luca Fancellu]

> On 2 May 2024, at 07:14, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 01.05.2024 08:57, Luca Fancellu wrote:
>> Hi Jan,
>> 
>>> On 30 Apr 2024, at 12:37, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>> --- a/xen/arch/arm/include/asm/setup.h
>>>> +++ b/xen/arch/arm/include/asm/setup.h
>>>> @@ -64,18 +64,20 @@ struct membank {
>>>> };
>>>> 
>>>> struct membanks {
>>>> -    unsigned int nr_banks;
>>>> -    unsigned int max_banks;
>>>> +    __struct_group(membanks_hdr, common, ,
>>>> +        unsigned int nr_banks;
>>>> +        unsigned int max_banks;
>>>> +    );
>>>>    struct membank bank[];
>>>> };
>>> 
>>> I'm afraid I can't spot why __struct_group() is needed here. Why would just
>>> one of the two more straightforward
>>> 
>>> struct membanks {
>>>   struct membanks_hdr {
>>>       unsigned int nr_banks;
>>>       unsigned int max_banks;
>>>   );
>>>   struct membank bank[];
>>> };
>>> 
>> 
>> At the first sight I thought this solution could have worked, however GCC brought me back down to earth
>> remembering me that flexible array members can’t be left alone in an empty structure:
>> 
>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: error: declaration does not declare anything [-Werror]
>> 70 | };
>> | ^
>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: error: flexible array member in a struct with no named members
>> 71 | struct membank bank[];
>> | ^~~~
>> [...]
> 
> Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution
> to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of
> borrowing __struct_group(), we could consider borrowing this as well. Or
> open-code it just here, for the time being (perhaps my preference). Yet
> it's not clear to me that doing so will actually be enough to make things
> work for you.

I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group()
was enough for my purpose, can I ask the technical reasons why it would be your
preference? Is there something in that construct that is a concern for you?

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Jan Beulich]

On 02.05.2024 08:33, Luca Fancellu wrote:
> 
> 
>> On 2 May 2024, at 07:14, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 01.05.2024 08:57, Luca Fancellu wrote:
>>> Hi Jan,
>>>
>>>> On 30 Apr 2024, at 12:37, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>>> --- a/xen/arch/arm/include/asm/setup.h
>>>>> +++ b/xen/arch/arm/include/asm/setup.h
>>>>> @@ -64,18 +64,20 @@ struct membank {
>>>>> };
>>>>>
>>>>> struct membanks {
>>>>> -    unsigned int nr_banks;
>>>>> -    unsigned int max_banks;
>>>>> +    __struct_group(membanks_hdr, common, ,
>>>>> +        unsigned int nr_banks;
>>>>> +        unsigned int max_banks;
>>>>> +    );
>>>>>    struct membank bank[];
>>>>> };
>>>>
>>>> I'm afraid I can't spot why __struct_group() is needed here. Why would just
>>>> one of the two more straightforward
>>>>
>>>> struct membanks {
>>>>   struct membanks_hdr {
>>>>       unsigned int nr_banks;
>>>>       unsigned int max_banks;
>>>>   );
>>>>   struct membank bank[];
>>>> };
>>>>
>>>
>>> At the first sight I thought this solution could have worked, however GCC brought me back down to earth
>>> remembering me that flexible array members can’t be left alone in an empty structure:
>>>
>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: error: declaration does not declare anything [-Werror]
>>> 70 | };
>>> | ^
>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: error: flexible array member in a struct with no named members
>>> 71 | struct membank bank[];
>>> | ^~~~
>>> [...]
>>
>> Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution
>> to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of
>> borrowing __struct_group(), we could consider borrowing this as well. Or
>> open-code it just here, for the time being (perhaps my preference). Yet
>> it's not clear to me that doing so will actually be enough to make things
>> work for you.
> 
> I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group()
> was enough for my purpose, can I ask the technical reasons why it would be your
> preference? Is there something in that construct that is a concern for you?

I don't like either construct very much, but of the two __DECLARE_FLEX_ARRAY()
looks slightly more "natural" for what is wanted and how it's done.
__struct_group() introducing twice the (effectively) same structure feels
pretty odd, for now at least. It's not even entirely clear to me whether there
aren't pitfalls, seeing that the C spec differentiates named and unnamed
struct fields in a few cases. For __DECLARE_FLEX_ARRAY(), otoh, I can't
presently see any reason to suspect possible corner cases.

Yet as said before - I'm not sure __DECLARE_FLEX_ARRAY() alone would be enough
for what you want to achieve.

Jan

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Luca Fancellu]

> On 2 May 2024, at 07:43, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 02.05.2024 08:33, Luca Fancellu wrote:
>> 
>> 
>>> On 2 May 2024, at 07:14, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 01.05.2024 08:57, Luca Fancellu wrote:
>>>> Hi Jan,
>>>> 
>>>>> On 30 Apr 2024, at 12:37, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> 
>>>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>>>> --- a/xen/arch/arm/include/asm/setup.h
>>>>>> +++ b/xen/arch/arm/include/asm/setup.h
>>>>>> @@ -64,18 +64,20 @@ struct membank {
>>>>>> };
>>>>>> 
>>>>>> struct membanks {
>>>>>> -    unsigned int nr_banks;
>>>>>> -    unsigned int max_banks;
>>>>>> +    __struct_group(membanks_hdr, common, ,
>>>>>> +        unsigned int nr_banks;
>>>>>> +        unsigned int max_banks;
>>>>>> +    );
>>>>>>   struct membank bank[];
>>>>>> };
>>>>> 
>>>>> I'm afraid I can't spot why __struct_group() is needed here. Why would just
>>>>> one of the two more straightforward
>>>>> 
>>>>> struct membanks {
>>>>>  struct membanks_hdr {
>>>>>      unsigned int nr_banks;
>>>>>      unsigned int max_banks;
>>>>>  );
>>>>>  struct membank bank[];
>>>>> };
>>>>> 
>>>> 
>>>> At the first sight I thought this solution could have worked, however GCC brought me back down to earth
>>>> remembering me that flexible array members can’t be left alone in an empty structure:
>>>> 
>>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: error: declaration does not declare anything [-Werror]
>>>> 70 | };
>>>> | ^
>>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: error: flexible array member in a struct with no named members
>>>> 71 | struct membank bank[];
>>>> | ^~~~
>>>> [...]
>>> 
>>> Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution
>>> to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of
>>> borrowing __struct_group(), we could consider borrowing this as well. Or
>>> open-code it just here, for the time being (perhaps my preference). Yet
>>> it's not clear to me that doing so will actually be enough to make things
>>> work for you.
>> 
>> I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group()
>> was enough for my purpose, can I ask the technical reasons why it would be your
>> preference? Is there something in that construct that is a concern for you?
> 
> I don't like either construct very much, but of the two __DECLARE_FLEX_ARRAY()
> looks slightly more "natural" for what is wanted and how it's done.
> __struct_group() introducing twice the (effectively) same structure feels
> pretty odd, for now at least. It's not even entirely clear to me whether there
> aren't pitfalls, seeing that the C spec differentiates named and unnamed
> struct fields in a few cases. For __DECLARE_FLEX_ARRAY(), otoh, I can't
> presently see any reason to suspect possible corner cases.
> 
> Yet as said before - I'm not sure __DECLARE_FLEX_ARRAY() alone would be enough
> for what you want to achieve.

Mmm yes, I think I would still have problems because container_of wants a named member,
so __DECLARE_FLEX_ARRAY() doesn’t help much alone, if I’m not missing something obvious.
If you believe however that importing __struct_group() only for this instance is not enough to justify
its presence in the codebase, I could open-code it, provided that maintainers are ok with that.

In any case it would be used soon also for other architectures using bootinfo.

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Jan Beulich]

On 02.05.2024 10:13, Luca Fancellu wrote:
> 
> 
>> On 2 May 2024, at 07:43, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 02.05.2024 08:33, Luca Fancellu wrote:
>>>
>>>
>>>> On 2 May 2024, at 07:14, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 01.05.2024 08:57, Luca Fancellu wrote:
>>>>> Hi Jan,
>>>>>
>>>>>> On 30 Apr 2024, at 12:37, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 30.04.2024 13:09, Luca Fancellu wrote:
>>>>>>> --- a/xen/arch/arm/include/asm/setup.h
>>>>>>> +++ b/xen/arch/arm/include/asm/setup.h
>>>>>>> @@ -64,18 +64,20 @@ struct membank {
>>>>>>> };
>>>>>>>
>>>>>>> struct membanks {
>>>>>>> -    unsigned int nr_banks;
>>>>>>> -    unsigned int max_banks;
>>>>>>> +    __struct_group(membanks_hdr, common, ,
>>>>>>> +        unsigned int nr_banks;
>>>>>>> +        unsigned int max_banks;
>>>>>>> +    );
>>>>>>>   struct membank bank[];
>>>>>>> };
>>>>>>
>>>>>> I'm afraid I can't spot why __struct_group() is needed here. Why would just
>>>>>> one of the two more straightforward
>>>>>>
>>>>>> struct membanks {
>>>>>>  struct membanks_hdr {
>>>>>>      unsigned int nr_banks;
>>>>>>      unsigned int max_banks;
>>>>>>  );
>>>>>>  struct membank bank[];
>>>>>> };
>>>>>>
>>>>>
>>>>> At the first sight I thought this solution could have worked, however GCC brought me back down to earth
>>>>> remembering me that flexible array members can’t be left alone in an empty structure:
>>>>>
>>>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: error: declaration does not declare anything [-Werror]
>>>>> 70 | };
>>>>> | ^
>>>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: error: flexible array member in a struct with no named members
>>>>> 71 | struct membank bank[];
>>>>> | ^~~~
>>>>> [...]
>>>>
>>>> Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution
>>>> to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of
>>>> borrowing __struct_group(), we could consider borrowing this as well. Or
>>>> open-code it just here, for the time being (perhaps my preference). Yet
>>>> it's not clear to me that doing so will actually be enough to make things
>>>> work for you.
>>>
>>> I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group()
>>> was enough for my purpose, can I ask the technical reasons why it would be your
>>> preference? Is there something in that construct that is a concern for you?
>>
>> I don't like either construct very much, but of the two __DECLARE_FLEX_ARRAY()
>> looks slightly more "natural" for what is wanted and how it's done.
>> __struct_group() introducing twice the (effectively) same structure feels
>> pretty odd, for now at least. It's not even entirely clear to me whether there
>> aren't pitfalls, seeing that the C spec differentiates named and unnamed
>> struct fields in a few cases. For __DECLARE_FLEX_ARRAY(), otoh, I can't
>> presently see any reason to suspect possible corner cases.
>>
>> Yet as said before - I'm not sure __DECLARE_FLEX_ARRAY() alone would be enough
>> for what you want to achieve.
> 
> Mmm yes, I think I would still have problems because container_of wants a named member,
> so __DECLARE_FLEX_ARRAY() doesn’t help much alone, if I’m not missing something obvious.
> If you believe however that importing __struct_group() only for this instance is not enough to justify
> its presence in the codebase, I could open-code it, provided that maintainers are ok with that.

I'm afraid I've even more strongly against open-coding. If you can get an
ack from another maintainer for the introduction of struct_group(), that
would still allow it to go in. I didn't NAK the change, I merely have
reservations.

> In any case it would be used soon also for other architectures using bootinfo.

Oh, would it?

Jan

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Luca Fancellu]

> 
>> In any case it would be used soon also for other architectures using bootinfo.
> 
> Oh, would it?

PPC people have plans on putting that interface in common:

https://patchwork.kernel.org/project/xen-devel/patch/451705505ff7f80ec66c78cc2830196fa6e4090c.1712893887.git.sanastasio@raptorengineering.com/

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Jan Beulich]

On 02.05.2024 12:12, Luca Fancellu wrote:
>>> In any case it would be used soon also for other architectures using bootinfo.
>>
>> Oh, would it?
> 
> PPC people have plans on putting that interface in common:

I'm aware, but ...

> https://patchwork.kernel.org/project/xen-devel/patch/451705505ff7f80ec66c78cc2830196fa6e4090c.1712893887.git.sanastasio@raptorengineering.com/

... I can't spot any flexible array members in this patch.

Jan

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Luca Fancellu]

> On 2 May 2024, at 11:30, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 02.05.2024 12:12, Luca Fancellu wrote:
>>>> In any case it would be used soon also for other architectures using bootinfo.
>>> 
>>> Oh, would it?
>> 
>> PPC people have plans on putting that interface in common:
> 
> I'm aware, but ...
> 
>> https://patchwork.kernel.org/project/xen-devel/patch/451705505ff7f80ec66c78cc2830196fa6e4090c.1712893887.git.sanastasio@raptorengineering.com/
> 
> ... I can't spot any flexible array members in this patch.

I guess v5 of that patch will have that, because it would be rebased on the latest state of
xen/arch/arm/include/asm/setup.h, moving that interface in xen/include/xen/bootfdt.h, in
order to use part of the code taken out from xen/arch/arm/setup.c and put in
xen/common/device-tree/bootinfo.c

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Nicola Vetrini]

On 2024-04-30 13:09, Luca Fancellu wrote:
> Commit 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory
> bank structures") introduced a MISRA regression for Rule 1.1 because a
> flexible array member is introduced in the middle of a struct, 
> furthermore
> this is using a GCC extension that is going to be deprecated in GCC 14 
> and
> a warning to identify such cases will be present
> (-Wflex-array-member-not-at-end) to identify such cases.
> 
> In order to fix this issue, use the macro __struct_group to create a
> structure 'struct membanks_hdr' which will hold the common data among
> structures using the 'struct membanks' interface.
> 
> Modify the 'struct shared_meminfo' and 'struct meminfo' to use this new
> structure, effectively removing the flexible array member from the 
> middle
> of the structure and modify the code accessing the .common field to use
> the macro container_of to maintain the functionality of the interface.
> 
> Given this change, container_of needs to be supplied with a type and so
> the macro 'kernel_info_get_mem' inside arm/include/asm/kernel.h can't 
> be
> an option since it uses const and non-const types for struct membanks, 
> so
> introduce two static inline, one of which will keep the const 
> qualifier.
> 
> Given the complexity of the interface, which carries a lot of benefit 
> but
> on the other hand could be prone to developer confusion if the access 
> is
> open-coded, introduce two static inline helper for the
> 'struct kernel_info' .shm_mem member and get rid the open-coding
> shm_mem.common access.
> 
> Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory 
> bank structures")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/arch/arm/acpi/domain_build.c        |  2 +-
>  xen/arch/arm/domain_build.c             |  6 +++---
>  xen/arch/arm/include/asm/kernel.h       | 11 ++++++++++-
>  xen/arch/arm/include/asm/setup.h        | 18 ++++++++++--------
>  xen/arch/arm/include/asm/static-shmem.h | 12 ++++++++++++
>  xen/arch/arm/static-shmem.c             | 19 +++++++++----------
>  6 files changed, 45 insertions(+), 23 deletions(-)
> 

 From a MISRA perspective the regression on R1.1 is resolved (see [1]).

[1] https://gitlab.com/xen-project/patchew/xen/-/jobs/6748211368

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Stefano Stabellini]

On Tue, 30 Apr 2024, Luca Fancellu wrote:
> Commit 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory
> bank structures") introduced a MISRA regression for Rule 1.1 because a
> flexible array member is introduced in the middle of a struct, furthermore
> this is using a GCC extension that is going to be deprecated in GCC 14 and
> a warning to identify such cases will be present
> (-Wflex-array-member-not-at-end) to identify such cases.
> 
> In order to fix this issue, use the macro __struct_group to create a
> structure 'struct membanks_hdr' which will hold the common data among
> structures using the 'struct membanks' interface.
> 
> Modify the 'struct shared_meminfo' and 'struct meminfo' to use this new
> structure, effectively removing the flexible array member from the middle
> of the structure and modify the code accessing the .common field to use
> the macro container_of to maintain the functionality of the interface.
> 
> Given this change, container_of needs to be supplied with a type and so
> the macro 'kernel_info_get_mem' inside arm/include/asm/kernel.h can't be
> an option since it uses const and non-const types for struct membanks, so
> introduce two static inline, one of which will keep the const qualifier.
> 
> Given the complexity of the interface, which carries a lot of benefit but
> on the other hand could be prone to developer confusion if the access is
> open-coded, introduce two static inline helper for the
> 'struct kernel_info' .shm_mem member and get rid the open-coding
> shm_mem.common access.
> 
> Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/acpi/domain_build.c        |  2 +-
>  xen/arch/arm/domain_build.c             |  6 +++---
>  xen/arch/arm/include/asm/kernel.h       | 11 ++++++++++-
>  xen/arch/arm/include/asm/setup.h        | 18 ++++++++++--------
>  xen/arch/arm/include/asm/static-shmem.h | 12 ++++++++++++
>  xen/arch/arm/static-shmem.c             | 19 +++++++++----------
>  6 files changed, 45 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
> index ed895dd8f926..2ce75543d004 100644
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -451,7 +451,7 @@ static int __init estimate_acpi_efi_size(struct domain *d,
>      struct acpi_table_rsdp *rsdp_tbl;
>      struct acpi_table_header *table;
>  
> -    efi_size = estimate_efi_size(kernel_info_get_mem(kinfo)->nr_banks);
> +    efi_size = estimate_efi_size(kernel_info_get_mem_const(kinfo)->nr_banks);
>  
>      acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
>      acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 0784e4c5e315..f6550809cfdf 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -805,7 +805,7 @@ int __init make_memory_node(const struct kernel_info *kinfo, int addrcells,
>       * 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) )
> +    if ( mem == kernel_info_get_mem_const(kinfo) )
>          shm_mem_node_fill_reg_range(kinfo, reg, &nr_cells, addrcells,
>                                      sizecells);
>  
> @@ -884,7 +884,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>  {
>      const struct membanks *mem = bootinfo_get_mem();
>      const struct membanks *mem_banks[] = {
> -        kernel_info_get_mem(kinfo),
> +        kernel_info_get_mem_const(kinfo),
>          bootinfo_get_reserved_mem(),
>  #ifdef CONFIG_STATIC_SHM
>          bootinfo_get_shmem(),
> @@ -1108,7 +1108,7 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>      uint64_t bankend;
>      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>      const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> -    const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo);
> +    const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
>      int res = -ENOENT;
>  
>      for ( i = 0; i < GUEST_RAM_BANKS; i++ )
> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> index 16a2bfc01e27..7e6e3c82a477 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -80,7 +80,16 @@ struct kernel_info {
>      };
>  };
>  
> -#define kernel_info_get_mem(kinfo) (&(kinfo)->mem.common)
> +static inline struct membanks *kernel_info_get_mem(struct kernel_info *kinfo)
> +{
> +    return container_of(&kinfo->mem.common, struct membanks, common);
> +}
> +
> +static inline const struct membanks *
> +kernel_info_get_mem_const(const struct kernel_info *kinfo)
> +{
> +    return container_of(&kinfo->mem.common, const struct membanks, common);
> +}
>  
>  #ifdef CONFIG_STATIC_SHM
>  #define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_SHMEM_BANKS,
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 28fb659fe946..61c15806a7c4 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -64,18 +64,20 @@ struct membank {
>  };
>  
>  struct membanks {
> -    unsigned int nr_banks;
> -    unsigned int max_banks;
> +    __struct_group(membanks_hdr, common, ,
> +        unsigned int nr_banks;
> +        unsigned int max_banks;
> +    );
>      struct membank bank[];
>  };
>  
>  struct meminfo {
> -    struct membanks common;
> +    struct membanks_hdr common;
>      struct membank bank[NR_MEM_BANKS];
>  };
>  
>  struct shared_meminfo {
> -    struct membanks common;
> +    struct membanks_hdr common;
>      struct membank bank[NR_SHMEM_BANKS];
>      struct shmem_membank_extra extra[NR_SHMEM_BANKS];
>  };
> @@ -166,25 +168,25 @@ extern domid_t max_init_domid;
>  
>  static inline struct membanks *bootinfo_get_mem(void)
>  {
> -    return &bootinfo.mem.common;
> +    return container_of(&bootinfo.mem.common, struct membanks, common);
>  }
>  
>  static inline struct membanks *bootinfo_get_reserved_mem(void)
>  {
> -    return &bootinfo.reserved_mem.common;
> +    return container_of(&bootinfo.reserved_mem.common, struct membanks, common);
>  }
>  
>  #ifdef CONFIG_ACPI
>  static inline struct membanks *bootinfo_get_acpi(void)
>  {
> -    return &bootinfo.acpi.common;
> +    return container_of(&bootinfo.acpi.common, struct membanks, common);
>  }
>  #endif
>  
>  #ifdef CONFIG_STATIC_SHM
>  static inline struct membanks *bootinfo_get_shmem(void)
>  {
> -    return &bootinfo.shmem.common;
> +    return container_of(&bootinfo.shmem.common, struct membanks, common);
>  }
>  
>  static inline struct shmem_membank_extra *bootinfo_get_shmem_extra(void)
> diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
> index 3b6569e5703f..806ee41cfc37 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -45,6 +45,18 @@ int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>  void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, __be32 *reg,
>                                   int *nr_cells, int addrcells, int sizecells);
>  
> +static inline struct membanks *
> +kernel_info_get_shm_mem(struct kernel_info *kinfo)
> +{
> +    return container_of(&kinfo->shm_mem.common, struct membanks, common);
> +}
> +
> +static inline const struct membanks *
> +kernel_info_get_shm_mem_const(const struct kernel_info *kinfo)
> +{
> +    return container_of(&kinfo->shm_mem.common, const struct membanks, common);
> +}
> +
>  #else /* !CONFIG_STATIC_SHM */
>  
>  /* Worst case /memory node reg element: (addrcells + sizecells) */
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 09f474ec6050..78881dd1d3f7 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -172,16 +172,16 @@ static int __init assign_shared_memory(struct domain *d,
>  }
>  
>  static int __init
> -append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start,
> +append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
>                            paddr_t size, const char *shm_id)
>  {
> -    struct membanks *shm_mem = &kinfo_shm_mem->common;
> +    struct membanks *shm_mem = kernel_info_get_shm_mem(kinfo);
>      struct shmem_membank_extra *shm_mem_extra;
>  
>      if ( shm_mem->nr_banks >= shm_mem->max_banks )
>          return -ENOMEM;
>  
> -    shm_mem_extra = &kinfo_shm_mem->extra[shm_mem->nr_banks];
> +    shm_mem_extra = &kinfo->shm_mem.extra[shm_mem->nr_banks];
>  
>      shm_mem->bank[shm_mem->nr_banks].start = start;
>      shm_mem->bank[shm_mem->nr_banks].size = size;
> @@ -289,8 +289,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>           * Record static shared memory region info for later setting
>           * up shm-node in guest device tree.
>           */
> -        ret = append_shm_bank_to_domain(&kinfo->shm_mem, gbase, psize,
> -                                        shm_id);
> +        ret = append_shm_bank_to_domain(kinfo, gbase, psize, shm_id);
>          if ( ret )
>              return ret;
>      }
> @@ -301,7 +300,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>  int __init make_shm_resv_memory_node(const struct kernel_info *kinfo,
>                                       int addrcells, int sizecells)
>  {
> -    const struct membanks *mem = &kinfo->shm_mem.common;
> +    const struct membanks *mem = kernel_info_get_shm_mem_const(kinfo);
>      void *fdt = kinfo->fdt;
>      unsigned int i = 0;
>      int res = 0;
> @@ -517,7 +516,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
>  int __init make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>                                   int sizecells)
>  {
> -    const struct membanks *mem = &kinfo->shm_mem.common;
> +    const struct membanks *mem = kernel_info_get_shm_mem_const(kinfo);
>      void *fdt = kinfo->fdt;
>      int res = 0;
>      /* Placeholder for reserved-memory\0 */
> @@ -579,7 +578,7 @@ void __init init_sharedmem_pages(void)
>  int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
>                                      struct rangeset *rangeset)
>  {
> -    const struct membanks *shm_mem = &kinfo->shm_mem.common;
> +    const struct membanks *shm_mem = kernel_info_get_shm_mem_const(kinfo);
>      unsigned int i;
>  
>      /* Remove static shared memory regions */
> @@ -607,7 +606,7 @@ int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
>  int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>                                       struct membanks *ext_regions)
>  {
> -    const struct membanks *shm_mem = &kinfo->shm_mem.common;
> +    const struct membanks *shm_mem = kernel_info_get_shm_mem_const(kinfo);
>      struct rangeset *guest_holes;
>      unsigned int i;
>      paddr_t start;
> @@ -673,7 +672,7 @@ 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;
> +    const struct membanks *mem = kernel_info_get_shm_mem_const(kinfo);
>      unsigned int i;
>      __be32 *cells;
>  
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end

[Luca Fancellu]

> On 2 May 2024, at 19:35, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 30 Apr 2024, Luca Fancellu wrote:
>> Commit 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory
>> bank structures") introduced a MISRA regression for Rule 1.1 because a
>> flexible array member is introduced in the middle of a struct, furthermore
>> this is using a GCC extension that is going to be deprecated in GCC 14 and
>> a warning to identify such cases will be present
>> (-Wflex-array-member-not-at-end) to identify such cases.
>> 
>> In order to fix this issue, use the macro __struct_group to create a
>> structure 'struct membanks_hdr' which will hold the common data among
>> structures using the 'struct membanks' interface.
>> 
>> Modify the 'struct shared_meminfo' and 'struct meminfo' to use this new
>> structure, effectively removing the flexible array member from the middle
>> of the structure and modify the code accessing the .common field to use
>> the macro container_of to maintain the functionality of the interface.
>> 
>> Given this change, container_of needs to be supplied with a type and so
>> the macro 'kernel_info_get_mem' inside arm/include/asm/kernel.h can't be
>> an option since it uses const and non-const types for struct membanks, so
>> introduce two static inline, one of which will keep the const qualifier.
>> 
>> Given the complexity of the interface, which carries a lot of benefit but
>> on the other hand could be prone to developer confusion if the access is
>> open-coded, introduce two static inline helper for the
>> 'struct kernel_info' .shm_mem member and get rid the open-coding
>> shm_mem.common access.
>> 
>> Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures")
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Hi Stefano,

Thanks!

Is it possible to add, eventually on commit, this tag?

Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>