RE: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Jan
> -Original Message-
> From: Jan Beulich
> Sent: Friday, May 21, 2021 3:09 PM
> To: Penny Zheng
> Cc: Bertrand Marquis ; Wei Chen
> ; nd ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org; jul...@xen.org
> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
>
> On 21.05.2021 08:41, Penny Zheng wrote:
> > Hi Jan
> >
> >> -Original Message-
> >> From: Jan Beulich
> >> Sent: Tuesday, May 18, 2021 7:23 PM
> >> To: Penny Zheng
> >> Cc: Bertrand Marquis ; Wei Chen
> >> ; nd ; xen-devel@lists.xenproject.org;
> >> sstabell...@kernel.org; jul...@xen.org
> >> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
> >>
> >> On 18.05.2021 10:57, Penny Zheng wrote:
> >>>> From: Jan Beulich
> >>>> Sent: Tuesday, May 18, 2021 3:35 PM
> >>>>
> >>>> On 18.05.2021 07:21, Penny Zheng wrote:
> >>>>> --- a/xen/common/page_alloc.c
> >>>>> +++ b/xen/common/page_alloc.c
> >>>>> @@ -2447,6 +2447,9 @@ int assign_pages(
> >>>>> {
> >>>>> ASSERT(page_get_owner([i]) == NULL);
> >>>>> page_set_owner([i], d);
> >>>>> +/* use page_set_reserved_owner to set its reserved domain
> >>>>> owner.
> >>>> */
> >>>>> +if ( (pg[i].count_info & PGC_reserved) )
> >>>>> +page_set_reserved_owner([i], d);
> >>>>
> >>>> Now this is puzzling: What's the point of setting two owner fields
> >>>> to the same value? I also don't recall you having introduced
> >>>> page_set_reserved_owner() for x86, so how is this going to build there?
> >>>>
> >>>
> >>> Thanks for pointing out that it will fail on x86.
> >>> As for the same value, sure, I shall change it to domid_t domid to
> >>> record its
> >> reserved owner.
> >>> Only domid is enough for differentiate.
> >>> And even when domain get rebooted, struct domain may be destroyed,
> >>> but domid will stays The same.
> >>
> >> Will it? Are you intending to put in place restrictions that make it
> >> impossible for the ID to get re-used by another domain?
> >>
> >>> Major user cases for domain on static allocation are referring to
> >>> the whole system are static, No runtime creation.
> >>
> >> Right, but that's not currently enforced afaics. If you would enforce
> >> it, it may simplify a number of things.
> >>
> >>>>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
> >>>>> return pg;
> >>>>> }
> >>>>>
> >>>>> +/*
> >>>>> + * Allocate nr_pfns contiguous pages, starting at #start, of
> >>>>> +static memory,
> >>>>> + * then assign them to one specific domain #d.
> >>>>> + * It is the equivalent of alloc_domheap_pages for static memory.
> >>>>> + */
> >>>>> +struct page_info *alloc_domstatic_pages(
> >>>>> +struct domain *d, unsigned long nr_pfns, paddr_t start,
> >>>>> +unsigned int memflags)
> >>>>> +{
> >>>>> +struct page_info *pg = NULL;
> >>>>> +unsigned long dma_size;
> >>>>> +
> >>>>> +ASSERT(!in_irq());
> >>>>> +
> >>>>> +if ( memflags & MEMF_no_owner )
> >>>>> +memflags |= MEMF_no_refcount;
> >>>>> +
> >>>>> +if ( !dma_bitsize )
> >>>>> +memflags &= ~MEMF_no_dma;
> >>>>> +else
> >>>>> +{
> >>>>> +dma_size = 1ul << bits_to_zone(dma_bitsize);
> >>>>> +/* Starting address shall meet the DMA limitation. */
> >>>>> +if ( dma_size && start < dma_size )
> >>>>> +return NULL;
> >>>>
> >>>> It is the entire range (i.e. in particular the last byte) which
> >>>> needs to meet such a restriction. I'm not convinced though that DMA
> >>>> width restrictions and static allocation are sensible to coexist.
> >>>>
> >>>
> >>> FWIT, if starting address meets the limitation, the last byte,
> >>> greater than starting address shall meet it too.
> >>
> >> I'm afraid I don't know what you're meaning to tell me here.
> >>
> >
> > Referring to alloc_domheap_pages, if `dma_bitsize` is none-zero value,
> > it will use alloc_heap_pages to allocate pages from [dma_zone + 1,
> > zone_hi], `dma_zone + 1` pointing to address larger than 2^(dma_zone + 1).
> > So I was setting address limitation for the starting address.
>
> But does this zone concept apply to static pages at all?
>
Oh, so sorry. I finally got what you were asking here. Hmm, I was using the
logic in
bits_to_zone to do the address bits translation. But I got, it will bring
confusion. I'll
fix it. Thx.
> Jan
Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
On 21.05.2021 08:41, Penny Zheng wrote:
> Hi Jan
>
>> -Original Message-
>> From: Jan Beulich
>> Sent: Tuesday, May 18, 2021 7:23 PM
>> To: Penny Zheng
>> Cc: Bertrand Marquis ; Wei Chen
>> ; nd ; xen-devel@lists.xenproject.org;
>> sstabell...@kernel.org; jul...@xen.org
>> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
>>
>> On 18.05.2021 10:57, Penny Zheng wrote:
>>>> From: Jan Beulich
>>>> Sent: Tuesday, May 18, 2021 3:35 PM
>>>>
>>>> On 18.05.2021 07:21, Penny Zheng wrote:
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -2447,6 +2447,9 @@ int assign_pages(
>>>>> {
>>>>> ASSERT(page_get_owner([i]) == NULL);
>>>>> page_set_owner([i], d);
>>>>> +/* use page_set_reserved_owner to set its reserved domain owner.
>>>> */
>>>>> +if ( (pg[i].count_info & PGC_reserved) )
>>>>> +page_set_reserved_owner([i], d);
>>>>
>>>> Now this is puzzling: What's the point of setting two owner fields to
>>>> the same value? I also don't recall you having introduced
>>>> page_set_reserved_owner() for x86, so how is this going to build there?
>>>>
>>>
>>> Thanks for pointing out that it will fail on x86.
>>> As for the same value, sure, I shall change it to domid_t domid to record
>>> its
>> reserved owner.
>>> Only domid is enough for differentiate.
>>> And even when domain get rebooted, struct domain may be destroyed, but
>>> domid will stays The same.
>>
>> Will it? Are you intending to put in place restrictions that make it
>> impossible
>> for the ID to get re-used by another domain?
>>
>>> Major user cases for domain on static allocation are referring to the
>>> whole system are static, No runtime creation.
>>
>> Right, but that's not currently enforced afaics. If you would enforce it, it
>> may
>> simplify a number of things.
>>
>>>>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
>>>>> return pg;
>>>>> }
>>>>>
>>>>> +/*
>>>>> + * Allocate nr_pfns contiguous pages, starting at #start, of static
>>>>> +memory,
>>>>> + * then assign them to one specific domain #d.
>>>>> + * It is the equivalent of alloc_domheap_pages for static memory.
>>>>> + */
>>>>> +struct page_info *alloc_domstatic_pages(
>>>>> +struct domain *d, unsigned long nr_pfns, paddr_t start,
>>>>> +unsigned int memflags)
>>>>> +{
>>>>> +struct page_info *pg = NULL;
>>>>> +unsigned long dma_size;
>>>>> +
>>>>> +ASSERT(!in_irq());
>>>>> +
>>>>> +if ( memflags & MEMF_no_owner )
>>>>> +memflags |= MEMF_no_refcount;
>>>>> +
>>>>> +if ( !dma_bitsize )
>>>>> +memflags &= ~MEMF_no_dma;
>>>>> +else
>>>>> +{
>>>>> +dma_size = 1ul << bits_to_zone(dma_bitsize);
>>>>> +/* Starting address shall meet the DMA limitation. */
>>>>> +if ( dma_size && start < dma_size )
>>>>> +return NULL;
>>>>
>>>> It is the entire range (i.e. in particular the last byte) which needs
>>>> to meet such a restriction. I'm not convinced though that DMA width
>>>> restrictions and static allocation are sensible to coexist.
>>>>
>>>
>>> FWIT, if starting address meets the limitation, the last byte, greater
>>> than starting address shall meet it too.
>>
>> I'm afraid I don't know what you're meaning to tell me here.
>>
>
> Referring to alloc_domheap_pages, if `dma_bitsize` is none-zero value,
> it will use alloc_heap_pages to allocate pages from [dma_zone + 1,
> zone_hi], `dma_zone + 1` pointing to address larger than 2^(dma_zone + 1).
> So I was setting address limitation for the starting address.
But does this zone concept apply to static pages at all?
Jan
RE: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Jan
> -Original Message-
> From: Jan Beulich
> Sent: Tuesday, May 18, 2021 7:23 PM
> To: Penny Zheng
> Cc: Bertrand Marquis ; Wei Chen
> ; nd ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org; jul...@xen.org
> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
>
> On 18.05.2021 10:57, Penny Zheng wrote:
> >> From: Jan Beulich
> >> Sent: Tuesday, May 18, 2021 3:35 PM
> >>
> >> On 18.05.2021 07:21, Penny Zheng wrote:
> >>> --- a/xen/common/page_alloc.c
> >>> +++ b/xen/common/page_alloc.c
> >>> @@ -2447,6 +2447,9 @@ int assign_pages(
> >>> {
> >>> ASSERT(page_get_owner([i]) == NULL);
> >>> page_set_owner([i], d);
> >>> +/* use page_set_reserved_owner to set its reserved domain owner.
> >> */
> >>> +if ( (pg[i].count_info & PGC_reserved) )
> >>> +page_set_reserved_owner([i], d);
> >>
> >> Now this is puzzling: What's the point of setting two owner fields to
> >> the same value? I also don't recall you having introduced
> >> page_set_reserved_owner() for x86, so how is this going to build there?
> >>
> >
> > Thanks for pointing out that it will fail on x86.
> > As for the same value, sure, I shall change it to domid_t domid to record
> > its
> reserved owner.
> > Only domid is enough for differentiate.
> > And even when domain get rebooted, struct domain may be destroyed, but
> > domid will stays The same.
>
> Will it? Are you intending to put in place restrictions that make it
> impossible
> for the ID to get re-used by another domain?
>
> > Major user cases for domain on static allocation are referring to the
> > whole system are static, No runtime creation.
>
> Right, but that's not currently enforced afaics. If you would enforce it, it
> may
> simplify a number of things.
>
> >>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
> >>> return pg;
> >>> }
> >>>
> >>> +/*
> >>> + * Allocate nr_pfns contiguous pages, starting at #start, of static
> >>> +memory,
> >>> + * then assign them to one specific domain #d.
> >>> + * It is the equivalent of alloc_domheap_pages for static memory.
> >>> + */
> >>> +struct page_info *alloc_domstatic_pages(
> >>> +struct domain *d, unsigned long nr_pfns, paddr_t start,
> >>> +unsigned int memflags)
> >>> +{
> >>> +struct page_info *pg = NULL;
> >>> +unsigned long dma_size;
> >>> +
> >>> +ASSERT(!in_irq());
> >>> +
> >>> +if ( memflags & MEMF_no_owner )
> >>> +memflags |= MEMF_no_refcount;
> >>> +
> >>> +if ( !dma_bitsize )
> >>> +memflags &= ~MEMF_no_dma;
> >>> +else
> >>> +{
> >>> +dma_size = 1ul << bits_to_zone(dma_bitsize);
> >>> +/* Starting address shall meet the DMA limitation. */
> >>> +if ( dma_size && start < dma_size )
> >>> +return NULL;
> >>
> >> It is the entire range (i.e. in particular the last byte) which needs
> >> to meet such a restriction. I'm not convinced though that DMA width
> >> restrictions and static allocation are sensible to coexist.
> >>
> >
> > FWIT, if starting address meets the limitation, the last byte, greater
> > than starting address shall meet it too.
>
> I'm afraid I don't know what you're meaning to tell me here.
>
Referring to alloc_domheap_pages, if `dma_bitsize` is none-zero value,
it will use alloc_heap_pages to allocate pages from [dma_zone + 1,
zone_hi], `dma_zone + 1` pointing to address larger than 2^(dma_zone + 1).
So I was setting address limitation for the starting address.
> Jan
Cheers
Penny
Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
On 19/05/2021 08:52, Penny Zheng wrote:
Hi Julien
Hi Penny,
-Original Message-
From: Julien Grall
Sent: Tuesday, May 18, 2021 8:13 PM
To: Penny Zheng ; Jan Beulich
Cc: Bertrand Marquis ; Wei Chen
; nd ; xen-devel@lists.xenproject.org;
sstabell...@kernel.org
Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Penny,
On 18/05/2021 09:57, Penny Zheng wrote:
-Original Message-
From: Jan Beulich
Sent: Tuesday, May 18, 2021 3:35 PM
To: Penny Zheng
Cc: Bertrand Marquis ; Wei Chen
; nd ; xen-
de...@lists.xenproject.org;
sstabell...@kernel.org; jul...@xen.org
Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
On 18.05.2021 07:21, Penny Zheng wrote:
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2447,6 +2447,9 @@ int assign_pages(
{
ASSERT(page_get_owner([i]) == NULL);
page_set_owner([i], d);
+/* use page_set_reserved_owner to set its reserved domain owner.
*/
+if ( (pg[i].count_info & PGC_reserved) )
+page_set_reserved_owner([i], d);
Now this is puzzling: What's the point of setting two owner fields to
the same value? I also don't recall you having introduced
page_set_reserved_owner() for x86, so how is this going to build there?
Thanks for pointing out that it will fail on x86.
As for the same value, sure, I shall change it to domid_t domid to record its
reserved owner.
Only domid is enough for differentiate.
And even when domain get rebooted, struct domain may be destroyed, but
domid will stays The same.
Major user cases for domain on static allocation are referring to the
whole system are static, No runtime creation.
One may want to have static memory yet doesn't care about the domid. So I
am not in favor to restrict about the domid unless there is no other way.
The user case you bring up here is that static memory pool?
No. The use case I am talking about is an user who wants to give a
specific memory region to the guest but doesn't care about which domid
was allocated to the guest.
Right now, the user cases are mostly restricted to static system.
If we bring runtime allocation, `xl` here, it will add a lot more complexity.
But if the system has static behavior, the domid is also static.
I read this as the admin would have to specify the domain ID in the
Device-Tree. Is that what you meant?
If so, then I don't see why we should mandate that. I would mind less if
by static you mean the domid will be allocated by Xen and then not
changed accross reboot.
On rebooting domain from static memory pool, it brings up more discussion, like
do we intend to give the memory back to static memory pool when rebooting,
if so, ram could be allocated from different place compared with the previous
one.
You should have all the information in the Device-Tree to re-assign the
correct regions. So why would it be allocated from a different place?
Cheers,
--
Julien Grall
RE: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Julien
> -Original Message-
> From: Julien Grall
> Sent: Tuesday, May 18, 2021 8:13 PM
> To: Penny Zheng ; Jan Beulich
> Cc: Bertrand Marquis ; Wei Chen
> ; nd ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
>
> Hi Penny,
>
> On 18/05/2021 09:57, Penny Zheng wrote:
> >> -Original Message-
> >> From: Jan Beulich
> >> Sent: Tuesday, May 18, 2021 3:35 PM
> >> To: Penny Zheng
> >> Cc: Bertrand Marquis ; Wei Chen
> >> ; nd ; xen-
> de...@lists.xenproject.org;
> >> sstabell...@kernel.org; jul...@xen.org
> >> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
> >>
> >> On 18.05.2021 07:21, Penny Zheng wrote:
> >>> --- a/xen/common/page_alloc.c
> >>> +++ b/xen/common/page_alloc.c
> >>> @@ -2447,6 +2447,9 @@ int assign_pages(
> >>> {
> >>> ASSERT(page_get_owner([i]) == NULL);
> >>> page_set_owner([i], d);
> >>> +/* use page_set_reserved_owner to set its reserved domain owner.
> >> */
> >>> +if ( (pg[i].count_info & PGC_reserved) )
> >>> +page_set_reserved_owner([i], d);
> >>
> >> Now this is puzzling: What's the point of setting two owner fields to
> >> the same value? I also don't recall you having introduced
> >> page_set_reserved_owner() for x86, so how is this going to build there?
> >>
> >
> > Thanks for pointing out that it will fail on x86.
> > As for the same value, sure, I shall change it to domid_t domid to record
> > its
> reserved owner.
> > Only domid is enough for differentiate.
> > And even when domain get rebooted, struct domain may be destroyed, but
> > domid will stays The same.
> > Major user cases for domain on static allocation are referring to the
> > whole system are static, No runtime creation.
>
> One may want to have static memory yet doesn't care about the domid. So I
> am not in favor to restrict about the domid unless there is no other way.
>
The user case you bring up here is that static memory pool?
Right now, the user cases are mostly restricted to static system.
If we bring runtime allocation, `xl` here, it will add a lot more complexity.
But if the system has static behavior, the domid is also static.
On rebooting domain from static memory pool, it brings up more discussion, like
do we intend to give the memory back to static memory pool when rebooting,
if so, ram could be allocated from different place compared with the previous
one.
And it kinds destroys system static behavior.
or not giving back, just store it in global variable `struct page_info
*[DOMID]` like the
others.
> Cheers,
>
> --
> Julien Grall
Cheers
Penny Zheng
RE: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Julien
> -Original Message-
> From: Julien Grall
> Sent: Tuesday, May 18, 2021 6:30 PM
> To: Penny Zheng ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org
> Cc: Bertrand Marquis ; Wei Chen
> ; nd
> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
>
> Hi Penny,
>
> Title: s/intruduce/introduce/
>
Thx~
> On 18/05/2021 06:21, Penny Zheng wrote:
> > alloc_domstatic_pages is the equivalent of alloc_domheap_pages for
> > static mmeory, and it is to allocate nr_pfns pages of static memory
> > and assign them to one specific domain.
> >
> > It uses alloc_staticmen_pages to get nr_pages pages of static memory,
> > then on success, it will use assign_pages to assign those pages to one
> > specific domain, including using page_set_reserved_owner to set its
> > reserved domain owner.
> >
> > Signed-off-by: Penny Zheng
> > ---
> > xen/common/page_alloc.c | 53
> +
> > xen/include/xen/mm.h| 4
> > 2 files changed, 57 insertions(+)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 0eb9f22a00..f1f1296a61 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2447,6 +2447,9 @@ int assign_pages(
> > {
> > ASSERT(page_get_owner([i]) == NULL);
> > page_set_owner([i], d);
> > +/* use page_set_reserved_owner to set its reserved domain owner.
> */
> > +if ( (pg[i].count_info & PGC_reserved) )
> > +page_set_reserved_owner([i], d);
>
> I have skimmed through the rest of the series and couldn't find anyone
> calling page_get_reserved_owner(). The value is also going to be the exact
> same as page_set_owner().
>
> So why do we need it?
>
In my first intent, This two helper page_get_reserved_owner/
page_set_reserved_owner
and the new field `reserved` in page_info are all for rebooting domain on
static allocation.
I was considering that, when implementing rebooting domain on static
allocation, memory
will be relinquished and right now, all freed back to heap, which is not
suitable for static memory here.
` relinquish_memory(d, >page_list) --> put_page --> free_domheap_page`
For pages in PGC_reserved, now, I am considering that, other than giving it
back to heap, maybe creating
a new global `struct page_info*[DOMID]` value to hold.
So it is better to have a new field in struct page_info, as follows, to hold
such info.
/* Page is reserved. */
struct {
/*
* Reserved Owner of this page,
* if this page is reserved to a specific domain.
*/
domid_t reserved_owner;
} reserved;
But this patch Serie is not going to include this feature, and I will delete
related helpers and values.
> > smp_wmb(); /* Domain pointer must be visible before updating
> refcnt. */
> > pg[i].count_info =
> > (pg[i].count_info & PGC_extra) | PGC_allocated | 1;
>
> This will clobber PGC_reserved.
>
related changes have been included into the commit of
"0008-xen-arm-introduce-reserved_page_list.patch".
> > @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
> > return pg;
> > }
> >
> > +/*
> > + * Allocate nr_pfns contiguous pages, starting at #start, of static
> > +memory,
>
> s/nr_pfns/nr_mfns/
>
Sure.
> > + * then assign them to one specific domain #d.
> > + * It is the equivalent of alloc_domheap_pages for static memory.
> > + */
> > +struct page_info *alloc_domstatic_pages(
> > +struct domain *d, unsigned long nr_pfns, paddr_t start,
>
> s/nr_pfns/nf_mfns/. Also, I would the third parameter to be an mfn_t.
>
Sure.
> > +unsigned int memflags)
> > +{
> > +struct page_info *pg = NULL;
> > +unsigned long dma_size;
> > +
> > +ASSERT(!in_irq());
> > +
> > +if ( memflags & MEMF_no_owner )
> > +memflags |= MEMF_no_refcount;
> > +
> > +if ( !dma_bitsize )
> > +memflags &= ~MEMF_no_dma;
> > +else
> > +{
> > +dma_size = 1ul << bits_to_zone(dma_bitsize);
> > +/* Starting address shall meet the DMA limitation. */
> > +if ( dma_size && start < dma_size )
> > +return NULL;
> > +}
> > +
> > +pg = alloc_staticmem_pages(nr_pfns, start, memflags);
> > +if ( !pg )
> > +return NULL;
> > +
> > +if ( d && !(memflags & MEMF_no_owner) )
> > +{
> > +if ( memflags & MEMF_no_re
Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Penny,
On 18/05/2021 09:57, Penny Zheng wrote:
-Original Message-
From: Jan Beulich
Sent: Tuesday, May 18, 2021 3:35 PM
To: Penny Zheng
Cc: Bertrand Marquis ; Wei Chen
; nd ; xen-devel@lists.xenproject.org;
sstabell...@kernel.org; jul...@xen.org
Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
On 18.05.2021 07:21, Penny Zheng wrote:
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2447,6 +2447,9 @@ int assign_pages(
{
ASSERT(page_get_owner([i]) == NULL);
page_set_owner([i], d);
+/* use page_set_reserved_owner to set its reserved domain owner.
*/
+if ( (pg[i].count_info & PGC_reserved) )
+page_set_reserved_owner([i], d);
Now this is puzzling: What's the point of setting two owner fields to the same
value? I also don't recall you having introduced
page_set_reserved_owner() for x86, so how is this going to build there?
Thanks for pointing out that it will fail on x86.
As for the same value, sure, I shall change it to domid_t domid to record its
reserved owner.
Only domid is enough for differentiate.
And even when domain get rebooted, struct domain may be destroyed, but domid
will stays
The same.
Major user cases for domain on static allocation are referring to the whole
system are static,
No runtime creation.
One may want to have static memory yet doesn't care about the domid. So
I am not in favor to restrict about the domid unless there is no other way.
Cheers,
--
Julien Grall
Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
On 18.05.2021 10:57, Penny Zheng wrote:
>> From: Jan Beulich
>> Sent: Tuesday, May 18, 2021 3:35 PM
>>
>> On 18.05.2021 07:21, Penny Zheng wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2447,6 +2447,9 @@ int assign_pages(
>>> {
>>> ASSERT(page_get_owner([i]) == NULL);
>>> page_set_owner([i], d);
>>> +/* use page_set_reserved_owner to set its reserved domain owner.
>> */
>>> +if ( (pg[i].count_info & PGC_reserved) )
>>> +page_set_reserved_owner([i], d);
>>
>> Now this is puzzling: What's the point of setting two owner fields to the
>> same
>> value? I also don't recall you having introduced
>> page_set_reserved_owner() for x86, so how is this going to build there?
>>
>
> Thanks for pointing out that it will fail on x86.
> As for the same value, sure, I shall change it to domid_t domid to record its
> reserved owner.
> Only domid is enough for differentiate.
> And even when domain get rebooted, struct domain may be destroyed, but domid
> will stays
> The same.
Will it? Are you intending to put in place restrictions that make it
impossible for the ID to get re-used by another domain?
> Major user cases for domain on static allocation are referring to the whole
> system are static,
> No runtime creation.
Right, but that's not currently enforced afaics. If you would
enforce it, it may simplify a number of things.
>>> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
>>> return pg;
>>> }
>>>
>>> +/*
>>> + * Allocate nr_pfns contiguous pages, starting at #start, of static
>>> +memory,
>>> + * then assign them to one specific domain #d.
>>> + * It is the equivalent of alloc_domheap_pages for static memory.
>>> + */
>>> +struct page_info *alloc_domstatic_pages(
>>> +struct domain *d, unsigned long nr_pfns, paddr_t start,
>>> +unsigned int memflags)
>>> +{
>>> +struct page_info *pg = NULL;
>>> +unsigned long dma_size;
>>> +
>>> +ASSERT(!in_irq());
>>> +
>>> +if ( memflags & MEMF_no_owner )
>>> +memflags |= MEMF_no_refcount;
>>> +
>>> +if ( !dma_bitsize )
>>> +memflags &= ~MEMF_no_dma;
>>> +else
>>> +{
>>> +dma_size = 1ul << bits_to_zone(dma_bitsize);
>>> +/* Starting address shall meet the DMA limitation. */
>>> +if ( dma_size && start < dma_size )
>>> +return NULL;
>>
>> It is the entire range (i.e. in particular the last byte) which needs to
>> meet such
>> a restriction. I'm not convinced though that DMA width restrictions and
>> static
>> allocation are sensible to coexist.
>>
>
> FWIT, if starting address meets the limitation, the last byte, greater than
> starting
> address shall meet it too.
I'm afraid I don't know what you're meaning to tell me here.
Jan
Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Penny,
Title: s/intruduce/introduce/
On 18/05/2021 06:21, Penny Zheng wrote:
alloc_domstatic_pages is the equivalent of alloc_domheap_pages for
static mmeory, and it is to allocate nr_pfns pages of static memory
and assign them to one specific domain.
It uses alloc_staticmen_pages to get nr_pages pages of static memory,
then on success, it will use assign_pages to assign those pages to
one specific domain, including using page_set_reserved_owner to set its
reserved domain owner.
Signed-off-by: Penny Zheng
---
xen/common/page_alloc.c | 53 +
xen/include/xen/mm.h| 4
2 files changed, 57 insertions(+)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 0eb9f22a00..f1f1296a61 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2447,6 +2447,9 @@ int assign_pages(
{
ASSERT(page_get_owner([i]) == NULL);
page_set_owner([i], d);
+/* use page_set_reserved_owner to set its reserved domain owner. */
+if ( (pg[i].count_info & PGC_reserved) )
+page_set_reserved_owner([i], d);
I have skimmed through the rest of the series and couldn't find anyone
calling page_get_reserved_owner(). The value is also going to be the
exact same as page_set_owner().
So why do we need it?
smp_wmb(); /* Domain pointer must be visible before updating refcnt.
*/
pg[i].count_info =
(pg[i].count_info & PGC_extra) | PGC_allocated | 1;
This will clobber PGC_reserved.
@@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
return pg;
}
+/*
+ * Allocate nr_pfns contiguous pages, starting at #start, of static memory,
s/nr_pfns/nr_mfns/
+ * then assign them to one specific domain #d.
+ * It is the equivalent of alloc_domheap_pages for static memory.
+ */
+struct page_info *alloc_domstatic_pages(
+struct domain *d, unsigned long nr_pfns, paddr_t start,
s/nr_pfns/nf_mfns/. Also, I would the third parameter to be an mfn_t.
+unsigned int memflags)
+{
+struct page_info *pg = NULL;
+unsigned long dma_size;
+
+ASSERT(!in_irq());
+
+if ( memflags & MEMF_no_owner )
+memflags |= MEMF_no_refcount;
+
+if ( !dma_bitsize )
+memflags &= ~MEMF_no_dma;
+else
+{
+dma_size = 1ul << bits_to_zone(dma_bitsize);
+/* Starting address shall meet the DMA limitation. */
+if ( dma_size && start < dma_size )
+return NULL;
+}
+
+pg = alloc_staticmem_pages(nr_pfns, start, memflags);
+if ( !pg )
+return NULL;
+
+if ( d && !(memflags & MEMF_no_owner) )
+{
+if ( memflags & MEMF_no_refcount )
+{
+unsigned long i;
+
+for ( i = 0; i < nr_pfns; i++ )
+pg[i].count_info = PGC_extra;
+}
+if ( assign_pages(d, pg, nr_pfns, memflags) )
+{
+free_staticmem_pages(pg, nr_pfns, memflags & MEMF_no_scrub);
+return NULL;
+}
+}
+
+return pg;
+}
+
void free_domheap_pages(struct page_info *pg, unsigned int order)
{
struct domain *d = page_get_owner(pg);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index dcf9daaa46..e45987f0ed 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -111,6 +111,10 @@ unsigned long __must_check domain_adjust_tot_pages(struct
domain *d,
int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
void get_outstanding_claims(uint64_t *free_pages, uint64_t
*outstanding_pages);
+/* Static Memory */
+struct page_info *alloc_domstatic_pages(struct domain *d,
+unsigned long nr_pfns, paddr_t start, unsigned int memflags);
+
/* Domain suballocator. These functions are *not* interrupt-safe.*/
void init_domheap_pages(paddr_t ps, paddr_t pe);
struct page_info *alloc_domheap_pages(
Cheers,
--
Julien Grall
RE: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
Hi Jan
> -Original Message-
> From: Jan Beulich
> Sent: Tuesday, May 18, 2021 3:35 PM
> To: Penny Zheng
> Cc: Bertrand Marquis ; Wei Chen
> ; nd ; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org; jul...@xen.org
> Subject: Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
>
> On 18.05.2021 07:21, Penny Zheng wrote:
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2447,6 +2447,9 @@ int assign_pages(
> > {
> > ASSERT(page_get_owner([i]) == NULL);
> > page_set_owner([i], d);
> > +/* use page_set_reserved_owner to set its reserved domain owner.
> */
> > +if ( (pg[i].count_info & PGC_reserved) )
> > +page_set_reserved_owner([i], d);
>
> Now this is puzzling: What's the point of setting two owner fields to the same
> value? I also don't recall you having introduced
> page_set_reserved_owner() for x86, so how is this going to build there?
>
Thanks for pointing out that it will fail on x86.
As for the same value, sure, I shall change it to domid_t domid to record its
reserved owner.
Only domid is enough for differentiate.
And even when domain get rebooted, struct domain may be destroyed, but domid
will stays
The same.
Major user cases for domain on static allocation are referring to the whole
system are static,
No runtime creation.
> > @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
> > return pg;
> > }
> >
> > +/*
> > + * Allocate nr_pfns contiguous pages, starting at #start, of static
> > +memory,
> > + * then assign them to one specific domain #d.
> > + * It is the equivalent of alloc_domheap_pages for static memory.
> > + */
> > +struct page_info *alloc_domstatic_pages(
> > +struct domain *d, unsigned long nr_pfns, paddr_t start,
> > +unsigned int memflags)
> > +{
> > +struct page_info *pg = NULL;
> > +unsigned long dma_size;
> > +
> > +ASSERT(!in_irq());
> > +
> > +if ( memflags & MEMF_no_owner )
> > +memflags |= MEMF_no_refcount;
> > +
> > +if ( !dma_bitsize )
> > +memflags &= ~MEMF_no_dma;
> > +else
> > +{
> > +dma_size = 1ul << bits_to_zone(dma_bitsize);
> > +/* Starting address shall meet the DMA limitation. */
> > +if ( dma_size && start < dma_size )
> > +return NULL;
>
> It is the entire range (i.e. in particular the last byte) which needs to meet
> such
> a restriction. I'm not convinced though that DMA width restrictions and static
> allocation are sensible to coexist.
>
FWIT, if starting address meets the limitation, the last byte, greater than
starting
address shall meet it too.
> > +}
> > +
> > +pg = alloc_staticmem_pages(nr_pfns, start, memflags);
> > +if ( !pg )
> > +return NULL;
> > +
> > +if ( d && !(memflags & MEMF_no_owner) )
> > +{
> > +if ( memflags & MEMF_no_refcount )
> > +{
> > +unsigned long i;
> > +
> > +for ( i = 0; i < nr_pfns; i++ )
> > +pg[i].count_info = PGC_extra;
> > +}
>
> Is this as well as the MEMF_no_owner case actually meaningful for statically
> allocated pages?
>
Thanks for pointing out. Truly, we do not need to take it considered.
> Jan
Re: [PATCH 07/10] xen/arm: intruduce alloc_domstatic_pages
On 18.05.2021 07:21, Penny Zheng wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2447,6 +2447,9 @@ int assign_pages(
> {
> ASSERT(page_get_owner([i]) == NULL);
> page_set_owner([i], d);
> +/* use page_set_reserved_owner to set its reserved domain owner. */
> +if ( (pg[i].count_info & PGC_reserved) )
> +page_set_reserved_owner([i], d);
Now this is puzzling: What's the point of setting two owner fields
to the same value? I also don't recall you having introduced
page_set_reserved_owner() for x86, so how is this going to build
there?
> @@ -2509,6 +2512,56 @@ struct page_info *alloc_domheap_pages(
> return pg;
> }
>
> +/*
> + * Allocate nr_pfns contiguous pages, starting at #start, of static memory,
> + * then assign them to one specific domain #d.
> + * It is the equivalent of alloc_domheap_pages for static memory.
> + */
> +struct page_info *alloc_domstatic_pages(
> +struct domain *d, unsigned long nr_pfns, paddr_t start,
> +unsigned int memflags)
> +{
> +struct page_info *pg = NULL;
> +unsigned long dma_size;
> +
> +ASSERT(!in_irq());
> +
> +if ( memflags & MEMF_no_owner )
> +memflags |= MEMF_no_refcount;
> +
> +if ( !dma_bitsize )
> +memflags &= ~MEMF_no_dma;
> +else
> +{
> +dma_size = 1ul << bits_to_zone(dma_bitsize);
> +/* Starting address shall meet the DMA limitation. */
> +if ( dma_size && start < dma_size )
> +return NULL;
It is the entire range (i.e. in particular the last byte) which needs
to meet such a restriction. I'm not convinced though that DMA width
restrictions and static allocation are sensible to coexist.
> +}
> +
> +pg = alloc_staticmem_pages(nr_pfns, start, memflags);
> +if ( !pg )
> +return NULL;
> +
> +if ( d && !(memflags & MEMF_no_owner) )
> +{
> +if ( memflags & MEMF_no_refcount )
> +{
> +unsigned long i;
> +
> +for ( i = 0; i < nr_pfns; i++ )
> +pg[i].count_info = PGC_extra;
> +}
Is this as well as the MEMF_no_owner case actually meaningful for
statically allocated pages?
Jan
