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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 20/05/2024 14:44, Luca Fancellu wrote:
> 
> 
> 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?
I'd be ok with meminfo provided you add a reasoning behind this being meminfo 
and not shared_meminfo.

> 
> Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ 
> member.
Would it result in a smaller footprint overall?

> 
>>>
>>> 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 ?
Why would you want user to know the machine physical address? It's a heap 
address.

> 
> 
>>>
>>> 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?
User can request static shmem region of as little as 1 page.

> 
>>>
>>> +                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
> 
> 

~Michal