Re: [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 15/05/2024 16:26, Luca Fancellu wrote:
> 
> 
> The current static shared memory code is using bootinfo banks when it
> needs to find the number of borrowers, so every time assign_shared_memory
> is called, the bank is searched in the bootinfo.shmem structure.
> 
> There is nothing wrong with it, however the bank can be used also to
> retrieve the start address and size and also to pass less argument to
s/argument/arguments

> assign_shared_memory. When retrieving the information from the bootinfo
> bank, it's also possible to move the checks on alignment to
> process_shm_node in the early stages.
> 
> So create a new function find_shm_bank_by_id() which takes a
> 'struct shared_meminfo' structure and the shared memory ID, to look for a
> bank with a matching ID, take the physical host address and size from the
> bank, pass the bank to assign_shared_memory() removing the now unnecessary
> arguments and finally remove the acquire_nr_borrower_domain() function
> since now the information can be extracted from the passed bank.
> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
> case of errors (unlikely), as said above, move the checks on alignment
> to process_shm_node.
> 
> Drawback of this change is that now the bootinfo are used also when the
> bank doesn't need to be allocated, however it will be convinient later
s/convinient/convenient

> to use it as an argument for assign_shared_memory when dealing with
> the use case where the Host physical address is not supplied by the user.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v2 changes:
>  - fix typo commit msg, renamed find_shm() to find_shm_bank_by_id(),
>    swap region size check different from zero and size alignment, remove
>    not necessary BUGON(). (Michal)
> ---
>  xen/arch/arm/static-shmem.c | 101 +++++++++++++++++++-----------------
>  1 file changed, 54 insertions(+), 47 deletions(-)
> 
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 78881dd1d3f7..0afc86c43f85 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -19,29 +19,22 @@ static void __init __maybe_unused build_assertions(void)
>                   offsetof(struct shared_meminfo, bank)));
>  }
> 
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> -                                             paddr_t pbase, paddr_t psize,
> -                                             unsigned long *nr_borrowers)
> +static const struct membank __init *
> +find_shm_bank_by_id(const struct membanks *shmem, const char *shm_id)
>  {
> -    const struct membanks *shmem = bootinfo_get_shmem();
>      unsigned int bank;
> 
> -    /* Iterate reserved memory to find requested shm bank. */
>      for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
>      {
> -        paddr_t bank_start = shmem->bank[bank].start;
> -        paddr_t bank_size = shmem->bank[bank].size;
> -
> -        if ( (pbase == bank_start) && (psize == bank_size) )
> +        if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
Does it really need to be strncmp? You validated it a few times already.

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal