Re: [PATCH v4 6/6] xen: retrieve reserved pages on populate_physmap

[Julien Grall <julien@xxxxxxx>]

On 17/05/2022 07:24, Penny Zheng wrote:
> Hi Julien
Hi Penny,

> > > +    if ( unlikely(!page) )
> > > +        return INVALID_MFN;
> > > +
> > > +    smfn = page_to_mfn(page);
> > > +
> > > +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
> > I am OK if we call acquire_domstatic_pages() for now. But long term, I think we
> > should consider to optimize it because we know the page is valid and belong
> > to the guest. So there are a lot of pointless work (checking mfn_valid(),
> > scrubbing in the free part, cleaning the cache...).
> I'm willing to fix it here since this fix is not blocking any other patch 
> serie~~
> I'm considering that maybe we could add a new memflag MEMF_xxx, (oh,
> Naming something is really "killing" me), then all these pointless work, 
> checking
> mfn_valid, flushing TLB and cache, we could exclude them by checking
> memflags & MEMF_xxxx.
> Wdyt?
I don't think we need a new flag because the decision is internal to the 
page allocator. Instead, acquire_staticmem_pages() could be split in two 
parts. Something like (function names are random):

static bool foo(struct page_info *pg,
                unsigned long nr,
                unsigned long memflags)
{
        spin_lock(&heap_lock);

        for ( i = 0; i < nr; i++ )
                ...

        spin_unlock(&heap_lock);

        if ( need_tlbflush )
          filtered...

        return true;

out_err:
        for ( ... )
          ...
        return false;
}

static struct page_info *bar(mfn_t smfn,
                             unsigned long mfn,
                             unsigned int memflags)
{
        ASSERT(nr_mfns);
        for ( i = 0; i < nr_mfns; i++ )
            if ( !mfn_valid(mfn_add(smfn, i)) )
                return NULL;

        pg = mfn_to_page(mfn);
        if ( !foo(...) )
          return NULL;

        for ( i = 0; i < nr_mfns; i++ )
                flush_page_to_ram(...);
}


acquire_reserved_page() would then only call foo() and assign_pages().

Cheers,

--
Julien Grall