Re: [PATCH 23/36] xen/arch: coloring: manually calculate Xen physical addresses

[Julien Grall <julien@xxxxxxx>]

Hi,

On 04/03/2022 17:46, Marco Solieri wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> During Xen coloring procedure, we need to manually calculate consecutive
> physical addresses that conform to the color selection. Add an helper
> function that does this operation. The latter will return the next
> address that conforms to Xen color selection.
>
> The next_colored function is architecture dependent and the provided
> implementation is for ARMv8.
>
> Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/coloring.c             | 43 +++++++++++++++++++++++++++++
>   xen/arch/arm/include/asm/coloring.h | 14 ++++++++++
>   2 files changed, 57 insertions(+)
>
> diff --git a/xen/arch/arm/coloring.c b/xen/arch/arm/coloring.c
> index 761414fcd7..aae3c77a7b 100644
> --- a/xen/arch/arm/coloring.c
> +++ b/xen/arch/arm/coloring.c
> @@ -222,6 +222,49 @@ uint32_t get_max_colors(void)
>       return max_col_num;
>   }
>   
> +paddr_t next_xen_colored(paddr_t phys)
> +{
> +    unsigned int i;
> +    unsigned int col_next_number = 0;
> +    unsigned int col_cur_number = (phys & addr_col_mask) >> PAGE_SHIFT;
> +    int overrun = 0;
This only looks to be used as an unsigned value. So please use 'unsigned 
int'.
> +    paddr_t ret;
> +
> +    /*
> +     * Check if address color conforms to Xen selection. If it does, return
> +     * the address as is.
> +     */
> +    for( i = 0; i < xen_col_num; i++)
Coding style: missing space after 'for' and before ')'.

> +        if ( col_cur_number == xen_col_list[i] )
> +            return phys;
> +
> +    /* Find next col */
> +    for( i = xen_col_num -1 ; i >= 0; i--)
i is unsigned. So the 'i >= 0' will always be true as it will wrap to 
2^32 - 1. What did you intend to check?
Coding style: missing space after 'for', '-' and before ')'.

> +    {
> +        if ( col_cur_number > xen_col_list[i])
Coding style: missing space before ')'.

> +        {
> +            /* Need to start to first element and add a way_size */
> +            if ( i == (xen_col_num - 1) )
> +            {
> +                col_next_number = xen_col_list[0];
> +                overrun = 1;
> +            }
> +            else
> +            {
> +                col_next_number = xen_col_list[i+1];
Coding style: Missing space before and after '+'.

> +                overrun = 0;
> +            }
> +            break;
> +        }
> +    }
> +
> +    /* Align phys to way_size */
> +    ret = phys - (PAGE_SIZE * col_cur_number);
I am not sure to understand how the comment is matching with the code. 
From the comment, I would expect the expression to contain 'way_size'.
> +    /* Add the offset based on color selection*/
Coding style: missing space before '*/'.

> +    ret += (PAGE_SIZE * (col_next_number)) + (way_size*overrun);
Coding style: Missing space before and after '*'.

> +    return ret;
> +}
> +
>   bool __init coloring_init(void)
>   {
>       int i, rc;
> diff --git a/xen/arch/arm/include/asm/coloring.h 
> b/xen/arch/arm/include/asm/coloring.h
> index 22e67dc9d8..8c4525677c 100644
> --- a/xen/arch/arm/include/asm/coloring.h
> +++ b/xen/arch/arm/include/asm/coloring.h
> @@ -28,6 +28,20 @@
>   
>   bool __init coloring_init(void);
>   
> +/*
> + * Return physical page address that conforms to the colors selection
> + * given in col_selection_mask after @param phys.
> + *
> + * @param phys         Physical address start.
> + * @param addr_col_mask        Mask specifying the bits available for coloring.
> + * @param col_selection_mask   Mask asserting the color bits to be used,
> + * must not be 0.
The function belows have only one parameter. Yet, you are description 3 
parameters here.
> + *
> + * @return The lowest physical page address being greater or equal than
> + * 'phys' and belonging to Xen color selection
> + */
> +paddr_t next_xen_colored(paddr_t phys);
> +
>   /*
>    * Check colors of a given domain.
>    * Return true if check passed, false otherwise.
Cheers,

--
Julien Grall