Hi,
On 04/03/2022 17:46, Marco Solieri wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
>
> 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@gmail.com>
> Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
> ---
> 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