Re: [PATCH 02/16] acpi: vmap pages in acpi_os_alloc_memory

From: Hongyan Xia <hx242@xen.org>
To: Julien Grall <julien.grall.oss@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 02/16] acpi: vmap pages in acpi_os_alloc_memory
Date: Mon, 04 May 2020 09:27:39 +0100	[thread overview]
Message-ID: <c3edd42c66ded0c1f129e1de2a4b8f54cef4c136.camel@xen.org> (raw)
In-Reply-To: <CAJ=z9a0S0rOYbJVPGK6mTKN0OgJtiTU7YN-APLF4Dvr4CaKfJg@mail.gmail.com>

On Fri, 2020-05-01 at 22:35 +0100, Julien Grall wrote:
> Hi,
> 
> On Thu, 30 Apr 2020 at 21:44, Hongyan Xia <hx242@xen.org> wrote:
> > 
> > From: Hongyan Xia <hongyxia@amazon.com>
> > 
> > Also, introduce a wrapper around vmap that maps a contiguous range
> > for
> > boot allocations.
> > 
> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> > ---
> >  xen/drivers/acpi/osl.c | 9 ++++++++-
> >  xen/include/xen/vmap.h | 5 +++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> > index 4c8bb7839e..d0762dad4e 100644
> > --- a/xen/drivers/acpi/osl.c
> > +++ b/xen/drivers/acpi/osl.c
> > @@ -219,7 +219,11 @@ void *__init acpi_os_alloc_memory(size_t sz)
> >         void *ptr;
> > 
> >         if (system_state == SYS_STATE_early_boot)
> > -               return
> > mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1)));
> > +       {
> > +               mfn_t mfn = alloc_boot_pages(PFN_UP(sz), 1);
> > +
> > +               return vmap_boot_pages(mfn, PFN_UP(sz));
> > +       }
> > 
> >         ptr = xmalloc_bytes(sz);
> >         ASSERT(!ptr || is_xmalloc_memory(ptr));
> > @@ -244,5 +248,8 @@ void __init acpi_os_free_memory(void *ptr)
> >         if (is_xmalloc_memory(ptr))
> >                 xfree(ptr);
> >         else if (ptr && system_state == SYS_STATE_early_boot)
> > +       {
> > +               vunmap(ptr);
> >                 init_boot_pages(__pa(ptr), __pa(ptr) + PAGE_SIZE);
> 
> __pa(ptr) can only work on the direct map. Even worth, on Arm it will
> fault because there is no mapping.
> I think you will want to use vmap_to_mfn() before calling vunmap().

Thanks for spotting this. This is definitely wrong. Will revise.

Hongyan