RE: [PATCH 10/10] xen/arm: introduce allocate_static_memory
Hi Julien
> -Original Message-
> From: Julien Grall
> Sent: Thursday, May 20, 2021 4:10 AM
> To: Penny Zheng ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Cc: Bertrand Marquis ; Wei Chen
> ; nd
> Subject: Re: [PATCH 10/10] xen/arm: introduce allocate_static_memory
>
>
>
> On 19/05/2021 08:27, Penny Zheng wrote:
> > Hi Julien
> >
> >> -Original Message-
> >> From: Julien Grall
> >> Sent: Tuesday, May 18, 2021 8:06 PM
> >> To: Penny Zheng ;
> >> xen-devel@lists.xenproject.org; sstabell...@kernel.org
> >> Cc: Bertrand Marquis ; Wei Chen
> >> ; nd
> >> 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
> >>> ---
> >>>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.
>
Ok. I finally totally understand what you suggest here.
I'll try to let the action related to setting kinfo->mem.bank[ram_index].start/
kinfo->mem.bank[ram_index].size/ kinfo->mem. nr_banks out of this function,
and only keep the simple functionality of mapping the memory for a range start
at ``sgfn``.
> >>> + mfn_t smfn,
> >>> + paddr_t tot_size) {
> >>> +int res;
> >>> +struct membank *bank;
> >>> +paddr_t _size = tot_size;
> >>> +
> >>> +bank = >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, *
Re: [PATCH 10/10] xen/arm: introduce allocate_static_memory
On 19/05/2021 08:27, Penny Zheng wrote:
Hi Julien
-Original Message-
From: Julien Grall
Sent: Tuesday, May 18, 2021 8:06 PM
To: Penny Zheng ; xen-devel@lists.xenproject.org;
sstabell...@kernel.org
Cc: Bertrand Marquis ; Wei Chen
; nd
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
---
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 = >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, ban
RE: [PATCH 10/10] xen/arm: introduce allocate_static_memory
Hi Julien
> -Original Message-
> From: Julien Grall
> Sent: Tuesday, May 18, 2021 8:06 PM
> To: Penny Zheng ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Cc: Bertrand Marquis ; Wei Chen
> ; nd
> 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
> > ---
> > 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.
> > + mfn_t smfn,
> > + paddr_t tot_size) {
> > +int res;
> > +struct membank *bank;
> > +paddr_t _size = tot_size;
> > +
> > +bank = >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_ba
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
---
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?
+ mfn_t smfn,
+ paddr_t tot_size)
+{
+int res;
+struct membank *bank;
+paddr_t _size = tot_size;
+
+bank = >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'.
+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.
+
+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.
+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?
Cheers,
--
Julien Grall
[PATCH 10/10] xen/arm: introduce allocate_static_memory
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
---
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,
+ paddr_t ram_addr,
+ gfn_t* sgfn,
+ mfn_t smfn,
+ paddr_t tot_size)
+{
+int res;
+struct membank *bank;
+paddr_t _size = tot_size;
+
+bank = >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;
+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;
+
+if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start, 0)
)
+{
+printk(XENLOG_ERR
+"%pd: cannot allocate static memory"
+"(0x%"PRIx64" - 0x%"PRIx64")",
+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).
+ */
+if ( ram0_size )
+{
+/* RAM0 at most holds GUEST_RAM0_SIZE. */
+if ( ram0_size >= bank_size )
+{
+if ( !allocate_static_bank_memory(d, kinfo,
+ 0, GUEST_RAM0_BASE,
+ , smfn, bank_size) )
+goto fail;
+
+ram0_size = ram0_size - bank_size;
+_banks++;
+continue;
+}
+else /* bank_size > ram0_size */
+{
+if ( !allocate_static_bank_memory(d, kinfo,
+ 0, GUEST_RAM0_BASE,
+ , smfn, ram0_size) )
+goto fail;
+
+/* The whole RAM0 is consumed. */
+ram0_size -= ram0_size;
+/* This bank hasn't been totally mapped, seeking to RAM1. */
+bank_size = bank_size - ram0_size;
+smfn = mfn_add(smfn, ram0_size >> PAGE_SHIFT);
+sgfn = gaddr_to_gfn(GUEST_RAM1_BASE);
+}
+}
+
+if ( ram1_size >= bank_size )
+{
+if (
