[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.