Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region

From: Julien Grall <julien@xen.org>
To: Penny Zheng <Penny.Zheng@arm.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Wei Chen <Wei.Chen@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
Date: Mon, 9 Jan 2023 10:01:29 +0000	[thread overview]
Message-ID: <1ff6107c-bcfe-353a-04ed-b429ac65a81d@xen.org> (raw)
In-Reply-To: <AM0PR08MB4530A8EA76480621255CEFC9F7FE9@AM0PR08MB4530.eurprd08.prod.outlook.com>

Hi Penny,

On 09/01/2023 07:48, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Sunday, January 8, 2023 7:44 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory
>> region
>> On 15/11/2022 02:52, Penny Zheng wrote:
>>> This commit re-arranges the static shared memory regions into a
>>> separate array shm_meminfo. And static shared memory region now
>> would
>>> have its own structure 'shm_membank' to hold all shm-related members,
>>> like shm_id, etc, and a pointer to the normal memory bank 'membank'.
>>> This will avoid continuing to grow 'membank'.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>>    xen/arch/arm/bootfdt.c            | 40 +++++++++++++++++++------------
>>>    xen/arch/arm/domain_build.c       | 35 ++++++++++++++++-----------
>>>    xen/arch/arm/include/asm/kernel.h |  2 +-
>>>    xen/arch/arm/include/asm/setup.h  | 16 +++++++++----
>>>    4 files changed, 59 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
>>> 6014c0f852..ccf281cd37 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt,
>> int node,
>>>        const __be32 *cell;
>>>        paddr_t paddr, gaddr, size;
>>>        struct meminfo *mem = &bootinfo.reserved_mem;
>>
>> After this patch, 'mem' is barely going to be used. So I would recommend to
>> remove it or restrict the scope.
>>
> 
> Hope I understand correctly, you are saying that all static shared memory bank will be
> described as "struct shm_membank". That's right.
> However when host address is provided, we still need an instance of "struct membank"
> to refer to in "bootinfo.reserved_mem". Only in this way, it could be initialized properly in
> as static pages.
> That's why I put a "struct membank*" pointer in "struct shm_membank" to refer to the
> same object.

I wasn't talking about the field in "struct shm_membank". Instead, I was 
referring to the local variable:

struct meminfo *mem = &bootinfo.reserved_mem;

AFAICT, the only use after this patch is when you add a new bank in 
shm_mem. So you could restrict the scope of the local variable.

> If I removed instance in bootinfo.reserved_mem, a few more path needs to be updated, like
> Init_staticmem_pages, dt_unreserved_regions, etc
>   
>> This will make easier to confirm that most of the use of 'mem' have been
>> replaced with 'shm_mem' and reduce the risk of confusion between the two
>> (the name are quite similar).
>>
>> [...]
>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index bd30d3798c..c0fd13f6ed 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -757,20 +757,20 @@ static int __init
>> acquire_nr_borrower_domain(struct domain *d,
>>>    {
>>>        unsigned int bank;
>>>
>>> -    /* Iterate reserved memory to find requested shm bank. */
>>> -    for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
>>> +    /* Iterate static shared memory to find requested shm bank. */
>>> +    for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
>>>        {
>>> -        paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
>>> -        paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
>>> +        paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank-
>>> start;
>>> +        paddr_t bank_size =
>>> + bootinfo.shm_mem.bank[bank].membank->size;
>>
>> I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..."  to be
>> removed. But it looks like there was none. I guess that was a mistake in the
>> existing code?
>>
> 
> Oh, you're right, the type shall also be checked.

Just to clarify, with this patch you don't need to check the type. I was 
pointing out a latent error in the existing code.

> 
>>>
>>>            if ( (pbase == bank_start) && (psize == bank_size) )
>>>                break;
>>>        }
>>>
>>> -    if ( bank == bootinfo.reserved_mem.nr_banks )
>>> +    if ( bank == bootinfo.shm_mem.nr_banks )
>>>            return -ENOENT;
>>>
>>> -    *nr_borrowers =
>> bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
>>> +    *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
>>>
>>>        return 0;
>>>    }
>>> @@ -907,11 +907,18 @@ static int __init
>> append_shm_bank_to_domain(struct kernel_info *kinfo,
>>>                                                paddr_t start, paddr_t size,
>>>                                                const char *shm_id)
>>>    {
>>> +    struct membank *membank;
>>> +
>>>        if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
>>>            return -ENOMEM;
>>>
>>> -    kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
>>> -    kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
>>> +    membank = xmalloc_bytes(sizeof(struct membank));
>>
>> You allocate memory but never free it. However, I think it would be better to
>> avoid the dynamic allocation. So I would consider to not use the structure
>> shm_membank and instead create a specific one for the domain construction.
>>
> 
> True, a local variable "struct meminfo shm_banks" could be introduced only
> for domain construction in function construct_domU

Hmmm... I didn't suggest to introduce a local variable. I would still 
much prefer if we keep using 'kinfo' because we keep all the domain 
building information in one place.

So ``struct meninfo`` should want to be defined in ``kinfo``.

Cheers,

-- 
Julien Grall