[RFC,3/4] xen/arm: GICv3: address MISRA C:2012 Rule 15.2

Commit Message

Nicola Vetrini Nov. 7, 2023, 10:33 a.m. UTC

The backwards jump due to the "goto retry;" statement
can be transformed into a loop, without losing much in terms
of readability.

Signed-off-by: Stefano Stabellini <[e-mail]>
Signed-off-by: Nicola Vetrini <[e-mail]>
---
This specific patch was provided by Stefano, I just added the
commit message.
---
 xen/arch/arm/gic-v3-its.c | 81 ++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 39 deletions(-)

Comments

Julien Grall Nov. 7, 2023, 12:34 p.m. UTC | #1

Hi Nicola,

On 07/11/2023 10:33, Nicola Vetrini wrote:
> The backwards jump due to the "goto retry;" statement
> can be transformed into a loop, without losing much in terms
> of readability.
> 
> Signed-off-by: Stefano Stabellini <[e-mail]>
> Signed-off-by: Nicola Vetrini <[e-mail]>
> ---
> This specific patch was provided by Stefano, I just added the
> commit message.

If that's the case, then Stefano should be the Author (at the moment 
this is you).

> ---
>   xen/arch/arm/gic-v3-its.c | 81 ++++++++++++++++++++-------------------
>   1 file changed, 42 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 8afcd9783bc8..4ba3f869ddf2 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -341,6 +341,7 @@ static int its_map_baser(void __iomem *basereg, uint64_t regc,
>       unsigned int pagesz = 2;    /* try 64K pages first, then go down. */
>       unsigned int table_size;
>       void *buffer;
> +    bool retry = false;

retry is false so...

>   
>       attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
>       attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> @@ -351,55 +352,57 @@ static int its_map_baser(void __iomem *basereg, uint64_t regc,
>        * it back and see what sticks (page size, cacheability and shareability
>        * attributes), retrying if necessary.
>        */
> -retry:
> -    table_size = ROUNDUP(nr_items * entry_size,
> -                         BIT(BASER_PAGE_BITS(pagesz), UL));
> -    /* The BASE registers support at most 256 pages. */
> -    table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
> +    while ( retry )

... you will never end in the loop. I also think that name 'retry' is 
confusing as the first time is not retry.

It would be clearer if we use a

do {

} while (retry)

That said, I actually prefer the goto version because some of the lines 
within the loop are now over 80 characters and splitting them would make 
the code harder to read.

Below an example, with the new indentation it is just over 80 characters.

if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )

One possibly would be to move the logic within the loop in a separate 
function.

Cheers,

Patch

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 8afcd9783bc8..4ba3f869ddf2 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -341,6 +341,7 @@  static int its_map_baser(void __iomem *basereg, uint64_t regc,
     unsigned int pagesz = 2;    /* try 64K pages first, then go down. */
     unsigned int table_size;
     void *buffer;
+    bool retry = false;
 
     attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
     attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
@@ -351,55 +352,57 @@  static int its_map_baser(void __iomem *basereg, uint64_t regc,
      * it back and see what sticks (page size, cacheability and shareability
      * attributes), retrying if necessary.
      */
-retry:
-    table_size = ROUNDUP(nr_items * entry_size,
-                         BIT(BASER_PAGE_BITS(pagesz), UL));
-    /* The BASE registers support at most 256 pages. */
-    table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
+    while ( retry )
+    {
+        table_size = ROUNDUP(nr_items * entry_size,
+                BIT(BASER_PAGE_BITS(pagesz), UL));
+        /* The BASE registers support at most 256 pages. */
+        table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
 
-    buffer = _xzalloc(table_size, BIT(BASER_PAGE_BITS(pagesz), UL));
-    if ( !buffer )
-        return -ENOMEM;
+        buffer = _xzalloc(table_size, BIT(BASER_PAGE_BITS(pagesz), UL));
+        if ( !buffer )
+            return -ENOMEM;
 
-    if ( !check_baser_phys_addr(buffer, BASER_PAGE_BITS(pagesz)) )
-    {
-        xfree(buffer);
-        return -ERANGE;
-    }
+        if ( !check_baser_phys_addr(buffer, BASER_PAGE_BITS(pagesz)) )
+        {
+            xfree(buffer);
+            return -ERANGE;
+        }
 
-    reg  = attr;
-    reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
-    reg |= (table_size >> BASER_PAGE_BITS(pagesz)) - 1;
-    reg |= regc & BASER_RO_MASK;
-    reg |= GITS_VALID_BIT;
-    reg |= encode_baser_phys_addr(virt_to_maddr(buffer),
-                                  BASER_PAGE_BITS(pagesz));
+        reg  = attr;
+        reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
+        reg |= (table_size >> BASER_PAGE_BITS(pagesz)) - 1;
+        reg |= regc & BASER_RO_MASK;
+        reg |= GITS_VALID_BIT;
+        reg |= encode_baser_phys_addr(virt_to_maddr(buffer),
+                BASER_PAGE_BITS(pagesz));
 
-    writeq_relaxed(reg, basereg);
-    regc = readq_relaxed(basereg);
+        writeq_relaxed(reg, basereg);
+        regc = readq_relaxed(basereg);
 
-    /* The host didn't like our attributes, just use what it returned. */
-    if ( (regc & BASER_ATTR_MASK) != attr )
-    {
-        /* If we can't map it shareable, drop cacheability as well. */
-        if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )
+        /* The host didn't like our attributes, just use what it returned. */
+        if ( (regc & BASER_ATTR_MASK) != attr )
         {
-            regc &= ~GITS_BASER_INNER_CACHEABILITY_MASK;
-            writeq_relaxed(regc, basereg);
+            /* If we can't map it shareable, drop cacheability as well. */
+            if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )
+            {
+                regc &= ~GITS_BASER_INNER_CACHEABILITY_MASK;
+                writeq_relaxed(regc, basereg);
+            }
+            attr = regc & BASER_ATTR_MASK;
         }
-        attr = regc & BASER_ATTR_MASK;
-    }
-    if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC )
-        clean_and_invalidate_dcache_va_range(buffer, table_size);
+        if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC )
+            clean_and_invalidate_dcache_va_range(buffer, table_size);
 
-    /* If the host accepted our page size, we are done. */
-    if ( ((regc >> GITS_BASER_PAGE_SIZE_SHIFT) & 0x3UL) == pagesz )
-        return 0;
+        /* If the host accepted our page size, we are done. */
+        if ( ((regc >> GITS_BASER_PAGE_SIZE_SHIFT) & 0x3UL) == pagesz )
+            return 0;
 
-    xfree(buffer);
+        xfree(buffer);
 
-    if ( pagesz-- > 0 )
-        goto retry;
+        if ( pagesz-- > 0 )
+            retry = true;
+    }
 
     /* None of the page sizes was accepted, give up */
     return -EINVAL;