All of lore.kernel.org
 help / color / mirror / Atom feed
From: Julien Grall <julien@xen.org>
To: Penny Zheng <Penny.Zheng@arm.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>
Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Wei Chen <Wei.Chen@arm.com>, nd <nd@arm.com>
Subject: Re: [PATCH 10/10] xen/arm: introduce allocate_static_memory
Date: Wed, 19 May 2021 21:10:01 +0100	[thread overview]
Message-ID: <72a374ca-4d75-70b4-3ee9-ad1dbdefa2d6@xen.org> (raw)
In-Reply-To: <VE1PR08MB5215B4D187DFE8AE20DF2B95F72B9@VE1PR08MB5215.eurprd08.prod.outlook.com>



On 19/05/2021 08:27, Penny Zheng wrote:
> Hi Julien
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Tuesday, May 18, 2021 8:06 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
>> sstabellini@kernel.org
>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
>> <Wei.Chen@arm.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH 10/10] xen/arm: introduce allocate_static_memory
>>
>> Hi Penny,
>>
>> On 18/05/2021 06:21, Penny Zheng wrote:
>>> This commit introduces allocate_static_memory to allocate static
>>> memory as guest RAM for domain on Static Allocation.
>>>
>>> It uses alloc_domstatic_pages to allocate pre-defined static memory
>>> banks for this domain, and uses guest_physmap_add_page to set up P2M
>>> table, guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>>    xen/arch/arm/domain_build.c | 157
>> +++++++++++++++++++++++++++++++++++-
>>>    1 file changed, 155 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 30b55588b7..9f662313ad 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct
>> domain *d,
>>>        return true;
>>>    }
>>>
>>> +/*
>>> + * #ram_index and #ram_index refer to the index and starting address
>>> +of guest
>>> + * memory kank stored in kinfo->mem.
>>> + * Static memory at #smfn of #tot_size shall be mapped #sgfn, and
>>> + * #sgfn will be next guest address to map when returning.
>>> + */
>>> +static bool __init allocate_static_bank_memory(struct domain *d,
>>> +                                               struct kernel_info *kinfo,
>>> +                                               int ram_index,
>>
>> Please use unsigned.
>>
>>> +                                               paddr_t ram_addr,
>>> +                                               gfn_t* sgfn,
>>
>> I am confused, what is the difference between ram_addr and sgfn?
>>
> 
> We need to constructing kinfo->mem(guest RAM banks) here, and
> we are indexing in static_mem(physical ram banks). Multiple physical ram
> banks consist of one guest ram bank(like, GUEST_RAM0).
> 
> ram_addr  here will either be GUEST_RAM0_BASE, or GUEST_RAM1_BASE,
> for now. I kinds struggled in how to name it. And maybe it shall not be a
> parameter here.
> 
> Maybe I should switch.. case.. on the ram_index, if its 0, its GUEST_RAM0_BASE,
> And if its 1, its GUEST_RAM1_BASE.

You only need to set kinfo->mem.bank[ram_index].start once. This is when 
you know the bank is first used.

AFAICT, this function will map the memory for a range start at ``sgfn``. 
It doesn't feel this belongs to the function.

The same remark is valid for kinfo->mem.nr_banks.

>>> +                                               mfn_t smfn,
>>> +                                               paddr_t tot_size) {
>>> +    int res;
>>> +    struct membank *bank;
>>> +    paddr_t _size = tot_size;
>>> +
>>> +    bank = &kinfo->mem.bank[ram_index];
>>> +    bank->start = ram_addr;
>>> +    bank->size = bank->size + tot_size;
>>> +
>>> +    while ( tot_size > 0 )
>>> +    {
>>> +        unsigned int order = get_allocation_size(tot_size);
>>> +
>>> +        res = guest_physmap_add_page(d, *sgfn, smfn, order);
>>> +        if ( res )
>>> +        {
>>> +            dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
>>> +            return false;
>>> +        }
>>> +
>>> +        *sgfn = gfn_add(*sgfn, 1UL << order);
>>> +        smfn = mfn_add(smfn, 1UL << order);
>>> +        tot_size -= (1ULL << (PAGE_SHIFT + order));
>>> +    }
>>> +
>>> +    kinfo->mem.nr_banks = ram_index + 1;
>>> +    kinfo->unassigned_mem -= _size;
>>> +
>>> +    return true;
>>> +}
>>> +
>>>    static void __init allocate_memory(struct domain *d, struct kernel_info
>> *kinfo)
>>>    {
>>>        unsigned int i;
>>> @@ -480,6 +524,116 @@ fail:
>>>              (unsigned long)kinfo->unassigned_mem >> 10);
>>>    }
>>>
>>> +/* Allocate memory from static memory as RAM for one specific domain
>>> +d. */ static void __init allocate_static_memory(struct domain *d,
>>> +                                            struct kernel_info
>>> +*kinfo) {
>>> +    int nr_banks, _banks = 0;
>>
>> AFAICT, _banks is the index in the array. I think it would be clearer if it is
>> caller 'bank' or 'idx'.
>>
> 
> Sure, I’ll use the 'bank' here.
> 
>>> +    size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE;
>>> +    paddr_t bank_start, bank_size;
>>> +    gfn_t sgfn;
>>> +    mfn_t smfn;
>>> +
>>> +    kinfo->mem.nr_banks = 0;
>>> +    sgfn = gaddr_to_gfn(GUEST_RAM0_BASE);
>>> +    nr_banks = d->arch.static_mem.nr_banks;
>>> +    ASSERT(nr_banks >= 0);
>>> +
>>> +    if ( kinfo->unassigned_mem <= 0 )
>>> +        goto fail;
>>> +
>>> +    while ( _banks < nr_banks )
>>> +    {
>>> +        bank_start = d->arch.static_mem.bank[_banks].start;
>>> +        smfn = maddr_to_mfn(bank_start);
>>> +        bank_size = d->arch.static_mem.bank[_banks].size;
>>
>> The variable name are slightly confusing because it doesn't tell whether this
>> is physical are guest RAM. You might want to consider to prefix them with p
>> (resp. g) for physical (resp. guest) RAM.
> 
> Sure, I'll rename to make it more clearly.
> 
>>
>>> +
>>> +        if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start,
>> 0) )
>>> +        {
>>> +            printk(XENLOG_ERR
>>> +                    "%pd: cannot allocate static memory"
>>> +                    "(0x%"PRIx64" - 0x%"PRIx64")",
>>
>> bank_start and bank_size are both paddr_t. So this should be PRIpaddr.
> 
> Sure, I'll change
> 
>>
>>> +                    d, bank_start, bank_start + bank_size);
>>> +            goto fail;
>>> +        }
>>> +
>>> +        /*
>>> +         * By default, it shall be mapped to the fixed guest RAM address
>>> +         * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
>>> +         * Starting from RAM0(GUEST_RAM0_BASE).
>>> +         */
>>
>> Ok. So you are first trying to exhaust the guest bank 0 and then moved to
>> bank 1. This wasn't entirely clear from the design document.
>>
>> I am fine with that, but in this case, the developper should not need to know
>> that (in fact this is not part of the ABI).
>>
>> Regarding this code, I am a bit concerned about the scalability if we introduce
>> a second bank.
>>
>> Can we have an array of the possible guest banks and increment the index
>> when exhausting the current bank?
>>
> 
> Correct me if I understand wrongly,
> 
> What you suggest here is that we make an array of guest banks, right now, including
> GUEST_RAM0 and GUEST_RAM1. And if later, adding more guest banks, it will not
> bring scalability problem here, right?

Yes. This should also reduce the current complexity of the code.

Cheers,

-- 
Julien Grall


  reply	other threads:[~2021-05-19 20:10 UTC|newest]

Thread overview: 82+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-18  5:21 [PATCH 00/10] Domain on Static Allocation Penny Zheng
2021-05-18  5:21 ` [PATCH 01/10] xen/arm: introduce domain " Penny Zheng
2021-05-18  8:58   ` Julien Grall
2021-05-19  2:22     ` Penny Zheng
2021-05-19 18:27       ` Julien Grall
2021-05-20  6:07         ` Penny Zheng
2021-05-20  8:50           ` Julien Grall
2021-06-02 10:09             ` Penny Zheng
2021-06-03  9:09               ` Julien Grall
2021-06-03 21:32                 ` Stefano Stabellini
2021-06-03 22:07                   ` Julien Grall
2021-06-03 23:55                     ` Stefano Stabellini
2021-06-04  4:00                       ` Penny Zheng
2021-06-05  2:00                         ` Stefano Stabellini
2021-06-07 18:09                       ` Julien Grall
2021-06-09  9:56                         ` Bertrand Marquis
2021-06-09 10:47                           ` Julien Grall
2021-06-15  6:08                             ` Penny Zheng
2021-06-17 11:22                               ` Julien Grall
2021-05-18  5:21 ` [PATCH 02/10] xen/arm: handle static memory in dt_unreserved_regions Penny Zheng
2021-05-18  9:04   ` Julien Grall
2021-05-18  5:21 ` [PATCH 03/10] xen/arm: introduce PGC_reserved Penny Zheng
2021-05-18  9:45   ` Julien Grall
2021-05-19  3:16     ` Penny Zheng
2021-05-19  9:49       ` Jan Beulich
2021-05-19 19:49         ` Julien Grall
2021-05-20  7:05           ` Jan Beulich
2021-05-19 19:46       ` Julien Grall
2021-05-20  6:19         ` Penny Zheng
2021-05-20  8:40           ` Penny Zheng
2021-05-20  8:59             ` Julien Grall
2021-05-20  9:27               ` Jan Beulich
2021-05-20  9:45                 ` Julien Grall
2021-05-18  5:21 ` [PATCH 04/10] xen/arm: static memory initialization Penny Zheng
2021-05-18  7:15   ` Jan Beulich
2021-05-18  9:51     ` Penny Zheng
2021-05-18 10:43       ` Jan Beulich
2021-05-20  9:04         ` Penny Zheng
2021-05-20  9:32           ` Jan Beulich
2021-05-18 10:00   ` Julien Grall
2021-05-18 10:01     ` Julien Grall
2021-05-19  5:02     ` Penny Zheng
2021-05-18  5:21 ` [PATCH 05/10] xen/arm: introduce alloc_staticmem_pages Penny Zheng
2021-05-18  7:24   ` Jan Beulich
2021-05-18  9:30     ` Penny Zheng
2021-05-18 10:09     ` Julien Grall
2021-05-18 10:15   ` Julien Grall
2021-05-19  5:23     ` Penny Zheng
2021-05-24 10:10       ` Penny Zheng
2021-05-24 10:24         ` Julien Grall
2021-05-18  5:21 ` [PATCH 06/10] xen: replace order with nr_pfns in assign_pages for better compatibility Penny Zheng
2021-05-18  7:27   ` Jan Beulich
2021-05-18  9:11     ` Penny Zheng
2021-05-18 10:20   ` Julien Grall
2021-05-19  5:35     ` Penny Zheng
2021-05-18  5:21 ` [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages Penny Zheng
2021-05-18  7:34   ` Jan Beulich
2021-05-18  8:57     ` Penny Zheng
2021-05-18 11:23       ` Jan Beulich
2021-05-21  6:41         ` Penny Zheng
2021-05-21  7:09           ` Jan Beulich
2021-06-03  2:44             ` Penny Zheng
2021-05-18 12:13       ` Julien Grall
2021-05-19  7:52         ` Penny Zheng
2021-05-19 20:01           ` Julien Grall
2021-05-18 10:30   ` Julien Grall
2021-05-19  6:03     ` Penny Zheng
2021-05-18  5:21 ` [PATCH 08/10] xen/arm: introduce reserved_page_list Penny Zheng
2021-05-18  7:39   ` Jan Beulich
2021-05-18  8:38     ` Penny Zheng
2021-05-18 11:24       ` Jan Beulich
2021-05-19  6:46         ` Penny Zheng
2021-05-18 11:02   ` Julien Grall
2021-05-19  6:43     ` Penny Zheng
2021-05-18  5:21 ` [PATCH 09/10] xen/arm: parse `xen,static-mem` info during domain construction Penny Zheng
2021-05-18 12:09   ` Julien Grall
2021-05-19  7:58     ` Penny Zheng
2021-05-18  5:21 ` [PATCH 10/10] xen/arm: introduce allocate_static_memory Penny Zheng
2021-05-18 12:05   ` Julien Grall
2021-05-19  7:27     ` Penny Zheng
2021-05-19 20:10       ` Julien Grall [this message]
2021-05-20  6:29         ` Penny Zheng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=72a374ca-4d75-70b4-3ee9-ad1dbdefa2d6@xen.org \
    --to=julien@xen.org \
    --cc=Bertrand.Marquis@arm.com \
    --cc=Penny.Zheng@arm.com \
    --cc=Wei.Chen@arm.com \
    --cc=nd@arm.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.