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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

Well overall yes, meminfo now is 255 banks, shared_meminfo is 64 in total, even 
if we use 32 of them and
32 are wasted.

Otherwise, as I said, I could do something like this in this module:

static struct shared_heap_meminfo {
    struct membanks_hdr common;
    struct membank bank[NR_SHMEM_BANKS];
} __initdata shm_heap_banks = {
    .common.max_banks = NR_SHMEM_BANKS
};

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

Oh ok your comment was about removing it, ok I don’t have strong objections to 
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.

Ok I’ll change to KB

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

Cheers,
Luca