[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
- To: Michal Orzel <michal.orzel@xxxxxxx>
- From: Luca Fancellu <Luca.Fancellu@xxxxxxx>
- Date: Mon, 20 May 2024 13:11:06 +0000
- Accept-language: en-GB, en-US
- Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=arm.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
- Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=lp1DqpcJayaK20Ky8dRfa8nfdAjnU/mEShlYoSbGCJ4=; b=lkOvmo8jUkhZbQWkUDS3ArVfSpufDSrGrbxHiH/s479cqiEbvh8pQLduerR9QWTmqUbRcEE0AZUpN286kOSsmbapBaQWSQjMvl1bP/rLIDN1QAVXm0smz9ZxbN7R7Zi82yUCrWyvh0LgTIK0Q7cYJ1silbPH1wBaPAOulseIisGSMiTXlJu0B7332U9VgdWGuuSF8BXCaT+2c2Pm0KkSZkfwX4gRR9WLA5eYctkWM8U9BGVElr+/ix2Nf9n1szctM36W1V4k+n3Tu6rVDDCcuCPpg2hIroPH7w2csFmb68wWWVbuh/HQh3++No0BqTY7p1OrWrptDR3n/jX6Ql2QRQ==
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=lp1DqpcJayaK20Ky8dRfa8nfdAjnU/mEShlYoSbGCJ4=; b=UrWbugpAiJOkgdHyKK15d4Ol/UYOCs10r+fOk/R5qgqRlnyco1CwzDCi00HH4UN+gsRwxBGNDZHEM1o/Jy9yicxUNTRTRMt2CFup7SmIAutKszJ1GcXnewGXfSLKli2CKHK3z7dFjyC6ZXiUhBuHPnz7rMFjTOsvUoYtn8E//r90FBeJkv+nrqHoTSkH1+pgOfPd/Zj9/7CwyDCS1LFluVPT8d36Taih4fwLHiq8f7rzinTavRYuOP4o4bJL77mG5edAJlsOxNwpy8mMsWz6EfqUUY970iZJOoNUfYq8UnnsB2T/fcKCz7+evrDZhtC2nDjTxz23ixLSEFkF4vElwA==
- Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=Mm/O9+H6YxgMI/CpF5dCTgtObRIrF81eGOzZ/Ld5VqK0xcktspXCAgo1A5+UzAxwNdzfAysOnLkjjl3cOG1ulDbDdizjRGBX6FZz5zLVdEzCIg7KK5OkCzW6f/kkjzNYWDPdze2sp/wQ257N2OnVqQaHtzU6XLTqNia4x3tKbEM3s0gSyv/T9uEcdDSBP0xnNSub8x4ZoNh6PPLeBS2oW3qMdHVRabI22kaiekcbNXkgWt9/AJuugUP/2ZqR6CcWKM4/EmG6GWg2+ZKTJaqjU7/49Lt3oXdNcs082qFzU8XZ89f2G10Po0HVMfsOu0HfS7V2ha3cRe3EU0pp9G03Tw==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gZB9ioESLKFMRHxSQTcFKgJ0jNB7rZfXHEQINCrxYQ1hcH/lfr7dRsY8pOOJOB0F9NZemNEWr7axN7AJnXyHcX+56ibYP6N6M5pIaHEf5Fwt9IuGlykcFbmexwFv8rNxzae4VQ0Q9xmqCOKQvDf4KiDeRiv6LzuBBked5/+QI7ah6oasfQQX4DEOYhsem4DAojPI+VcxrdIM6qKxSIs/L5T5r8EpQ9PeiG86W4wK60DPzZ0OUZ2IAXJVQQ/sktaebviXaAcmVboOFLZMNY+jIql4A/T56nJ0zbRKmmBnsqAh/794Fm9+E9aPJ93MVxDy4d9w449xk16FRkwV2U3ofg==
- Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
- Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
- Delivery-date: Mon, 20 May 2024 13:11:23 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
- Nodisclaimer: true
- Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
- Thread-index: AQHaptP4l1WkDlPwC0KOp+ZDl+nOHbGgALAAgAAYgICAAASzAIAAAsEA
- Thread-topic: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
>>
>>>
>>>> + 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
|