Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Michal,

> On 20 May 2024, at 12:16, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 15/05/2024 16:26, Luca Fancellu wrote:
>> 
>> 
>> This commit implements the logic to have the static shared memory banks
>> from the Xen heap instead of having the host physical address passed from
>> the user.
>> 
>> When the host physical address is not supplied, the physical memory is
>> taken from the Xen heap using allocate_domheap_memory, the allocation
>> needs to occur at the first handled DT node and the allocated banks
>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>> global variable of type 'struct meminfo' that will hold the banks
>> allocated from the heap, its field .shmem_extra will be used to point
>> to the bootinfo shared memory banks .shmem_extra space, so that there
>> is not further allocation of memory and every bank in shm_heap_banks
>> can be safely identified by the shm_id to reconstruct its traceability
>> and if it was allocated or not.
> NIT for the future: it's better to split 10 lines long sentence into multiple 
> ones.
> Otherwise it reads difficult.

I’ll do,

>> 
>> xen/arch/arm/static-shmem.c | 186 ++++++++++++++++++++++++++++++------
>> 1 file changed, 155 insertions(+), 31 deletions(-)
>> 
>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>> index ddaacbc77740..9c3a83042d8b 100644
>> --- a/xen/arch/arm/static-shmem.c
>> +++ b/xen/arch/arm/static-shmem.c
>> @@ -9,6 +9,22 @@
>> #include <asm/static-memory.h>
>> #include <asm/static-shmem.h>
>> 
>> +typedef struct {
>> +    struct domain *d;
>> +    paddr_t gbase;
>> +    const char *role_str;
> You could swap role_str and gbase to avoid a 4B hole on arm32

Sure I will,

> 
>> +    struct shmem_membank_extra *bank_extra_info;
>> +} alloc_heap_pages_cb_extra;
>> +
>> +static struct meminfo __initdata shm_heap_banks = {
>> +    .common.max_banks = NR_MEM_BANKS
> Do we expect that many banks?

Not really, but I was trying to don’t introduce another type, do you think it’s 
better instead to
introduce a new type only here, with a lower amount of banks?

Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ 
member.

>> 
>> static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>> +                                       bool bank_from_heap,
>>                                        const struct membank *shm_bank)
>> {
>>     mfn_t smfn;
>> @@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain 
>> *d, paddr_t gbase,
>>     psize = shm_bank->size;
>>     nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>> 
>> -    printk("%pd: allocate static shared memory BANK 
>> %#"PRIpaddr"-%#"PRIpaddr".\n",
>> -           d, pbase, pbase + psize);
>> -
>> -    smfn = acquire_shared_memory_bank(d, pbase, psize);
>> +    smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
>>     if ( mfn_eq(smfn, INVALID_MFN) )
>>         return -EINVAL;
>> 
>> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, 
>> paddr_t start,
>> 
>> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>                                          const char *role_str,
>> +                                         bool bank_from_heap,
>>                                          const struct membank *shm_bank)
>> {
>>     bool owner_dom_io = true;
>> @@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain 
>> *d, paddr_t gbase,
>>     pbase = shm_bank->start;
>>     psize = shm_bank->size;
>> 
>> +    printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 
>> 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
>> +           d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
> This looks more like a debug print since I don't expect user to want to see a 
> machine address.

printk(XENLOG_DEBUG ? 


>> 
>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>                        const struct dt_device_node *node)
>> {
>> @@ -265,37 +329,97 @@ int __init process_shm(struct domain *d, struct 
>> kernel_info *kinfo,
>>         pbase = boot_shm_bank->start;
>>         psize = boot_shm_bank->size;
>> 
>> -        if ( INVALID_PADDR == pbase )
>> -        {
>> -            printk("%pd: host physical address must be chosen by users at 
>> the moment", d);
>> -            return -EINVAL;
>> -        }
>> +        /* "role" property is optional */
>> +        dt_property_read_string(shm_node, "role", &role_str);
> This function returns a value but you seem to ignore it

Sure, I’ll handle that

>> 
>> -        ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
>> -        if ( ret )
>> -            return ret;
>> +            if ( !alloc_bank )
>> +            {
>> +                alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str,
>> +                    boot_shm_bank->shmem_extra };
>> +
>> +                /* shm_id identified bank is not yet allocated */
>> +                if ( !allocate_domheap_memory(NULL, psize, 
>> save_map_heap_pages,
>> +                                              &cb_arg) )
>> +                {
>> +                    printk(XENLOG_ERR
>> +                           "Failed to allocate (%"PRIpaddr"MB) pages as 
>> static shared memory from heap\n",
> Why limiting to MB?

I think I used it from domain_build.c, do you think it’s better to limit it on 
KB instead?

>> 
>> +                for ( ; alloc_bank < end_bank; alloc_bank++ )
>> +                {
>> +                    if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
>> +                                 MAX_SHM_ID_LENGTH) != 0 )
> shm_id has been already validated above, hence no need for a safe version of 
> strcmp
> 

I always try to use the safe version, even when redundant, I feel that if 
someone is copying part of the code,
at least it would copy a safe version. Anyway I will change it if it’s not 
desirable.

Cheers,
Luca