Re: [Xen-devel] [PATCH 4/4] tools/dombuilder: Don't allocate dom->p2m_host[] for translated domains

[Julien Grall <julien@xxxxxxx>]

Hi,

On 03/01/2020 10:44, Julien Grall wrote:
> Hi Andrew,
>
> On 02/01/2020 17:49, Andrew Cooper wrote:
> > On 23/12/2019 18:23, Julien Grall wrote:
> > > Hi,
> > >
> > > On 17/12/2019 21:15, Andrew Cooper wrote:
> > > > xc_dom_p2m() and dom->p2m_host[] implement a linear transform for
> > > > translated
> > > > domains, but waste a substantial chunk of RAM doing so.
> > > >
> > > > ARM literally never reads dom->p2m_host[] (because of the
> > > > xc_dom_translated()
> > > > short circuit in xc_dom_p2m()).  Drop it all.
> > > >
> > > > x86 HVM does use dom->p2m_host[] for
> > > > xc_domain_populate_physmap_exact() calls
> > > > when populating 4k pages.  Reuse the same tactic from 2M/1G ranges
> > > > and use an
> > > > on-stack array instead.  Drop the memory allocation.
> > > >
> > > > x86 PV guests do use dom->p2m_host[] as a non-identity transform.
> > > > Rename the
> > > > field to pv_p2m to make it clear it is PV-only.
> > > Nice cleanup! This will probably make slightly faster guest boot :).
> > >
> > > > @@ -359,7 +356,6 @@ static int populate_guest_memory(struct
> > > > xc_dom_image *dom,
> > > >    static int meminit(struct xc_dom_image *dom)
> > > >    {
> > > >        int i, rc;
> > > > -    xen_pfn_t pfn;
> > > >        uint64_t modbase;
> > > >          uint64_t ramsize = (uint64_t)dom->total_pages << 
> > > > XC_PAGE_SHIFT;
> > > > @@ -423,11 +419,6 @@ static int meminit(struct xc_dom_image *dom)
> > > >        assert(ramsize == 0); /* Too much RAM is rejected above */
> > > >          dom->p2m_size = p2m_size;
> > > Do we need to keep p2m_size?
> > Yes, I think so.
> >
> > There are some common checks which would fail if it becomes 0, and
> > importantly, it is used to locate safe gfns for temporary physmap 
> > mappings.
> Do you mean the scratch page? If so, on Arm we use a fix address in the 
> memory map rather than p2m_size.
> I have looked at the usage of the p2m_size in the common code and I 
> didn't spot any path that can be used by Arm and using p2m_size.
> Also, I don't think p2m_size is calculated correctly on Arm. It only 
> englobe the RAM and doesn't take into account the rest of the mappings 
> (e.g MMIO...). So I am not sure how this could be used in common code.
Thinking a bit more of this. As the p2m_size was IHMO already buggy, it 
should not make any difference after this series. Therefore removing 
p2m_size can be probably done on a follow-up patch.
So I am happy with the patch as-is:

Acked-by: Julien Grall <julien@xxxxxxx>

Cheers,

--
Julien Grall

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