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