Re: [Xen-devel] [RFC PATCH 68/84] page_alloc: actually do the mapping and unmapping on xenheap.

[<hongyax@xxxxxxxxxx>]

On 26/09/2019 11:39, Julien Grall wrote:
> Hi,
>
> NIT Title: Please remove full stop.
>
> On 9/26/19 10:46 AM, hongyax@xxxxxxxxxx wrote:
> > From: Hongyan Xia <hongyax@xxxxxxxxxx>
> Please provide a description of what/why you are doing this in the commit 
> message.
>
> Also, IIRC, x86 always have !CONFIG_SEPARATE_XENHEAP. So can you explain why 
> the path with separate xenheap is also modified?
> > Signed-off-by: Hongyan Xia <hongyax@xxxxxxxxxx>
> > ---
> >   xen/common/page_alloc.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> > index 7cb1bd368b..4ec6299ba8 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2143,6 +2143,7 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
> >   void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
> >   {
> >       struct page_info *pg;
> > +    void *ret;
> >       ASSERT(!in_irq());
> > @@ -2151,7 +2152,10 @@ void *alloc_xenheap_pages(unsigned int order, unsigned 
> > int memflags)
> >       if ( unlikely(pg == NULL) )
> >           return NULL;
> > -    memguard_unguard_range(page_to_virt(pg), 1 << (order + PAGE_SHIFT));
> > +    ret = page_to_virt(pg);
> > +    memguard_unguard_range(ret, 1 << (order + PAGE_SHIFT));
> > +    map_pages_to_xen((unsigned long)ret, page_to_mfn(pg),
> > +                     1UL << order, PAGE_HYPERVISOR);
> As mentioned earlier on for Arm, xenheap will always be mapped. So unless you 
> have plan to tackle the Arm side as well, we should make sure that the behavior 
> is not changed for Arm.
I can add an #ifdef for x86. Although I think if the Arm code is correct, this 
should still not break things. It breaks if a xenheap access is made even 
before allocation or after free, which I think is a bug.
> It feels to me we want to introduce a new Kconfig that is selected by x86 to 
> tell whether the direct map is mapped. I would then implement maybe in xen/mm.h 
> two stub (one for when the config is selected, the other when it is not).
I have the same question. Do we want to move forward with no direct map in x86 
or do we want to have a compile-time config? If the performance is decent, I 
would prefer the former since this could be a big compile-time switch which 
leaves two branches of code to be maintained in the future.
Hongyan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel