Re: [PATCH V6 1/2] xen/gnttab: Store frame GFN in struct page_info on Arm

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 24/06/2022 07:45, Jan Beulich wrote:
> On 23.06.2022 19:50, Julien Grall wrote:
> > On 11/05/2022 19:47, Oleksandr Tyshchenko wrote:
> > > @@ -1505,7 +1507,23 @@ int xenmem_add_to_physmap_one(
> > >        }
> > >    
> > >        /* Map at new location. */
> > > -    rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
> > > +    if ( !p2m_is_ram(t) || !is_xen_heap_mfn(mfn) )
> > > +        rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
> > I would expand the comment above to explain why you need a different
> > path for xenheap mapped as RAM. AFAICT, this is because we need to call
> > page_set_xenheap_gfn().
> >
> > > +    else
> > > +    {
> > > +        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > +
> > > +        p2m_write_lock(p2m);
> > > +        if ( gfn_eq(page_get_xenheap_gfn(mfn_to_page(mfn)), INVALID_GFN) )
> > Sorry to only notice it now. This check will also change the behavior
> > for XENMAPSPACE_shared_info. Now, we are only allowed to map the shared
> > info once.
> >
> > I believe this is fine because AFAICT x86 already prevents it. But this
> > is probably something that ought to be explained in the already long
> > commit message.
> If by "prevent" you mean x86 unmaps the page from its earlier GFN, then
> yes. But this means that Arm would better follow that model instead of
> returning -EBUSY in this case. Just think of kexec-ing or a boot loader
> wanting to map shared info or grant table: There wouldn't necessarily
> be an explicit unmap.
I spent some time to think about this. There is a potential big issue 
with implicit unmapping from its earlier GFN. Imagine the boot loader 
decided to map the page in place of a RAM.
If the boot loader didn't unmap the page then when the OS map again, we 
would have a hole in the RAM. The OS may not know that and it may end up 
to use a page as RAM (and crash).
The problem is the same for kexec and AFAIU that's why we need to use 
soft-reset when kexec-ing.
So overall, I think we should prevent implicit unmap. So it would help 
to enforce that the bootloader (or any other components) clean-up behind 
themselves (i.e. unmap the page and populate if necessary).
Cheers,

--
Julien Grall