Re: [PATCH v2 (resend) 04/27] acpi: vmap pages in acpi_os_alloc_memory

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Wed Jun 26, 2024 at 4:17 PM BST, Jan Beulich wrote:
> On 26.06.2024 15:54, Alejandro Vallejo wrote:
> > I'm late to the party but there's something bothering me a little.
> > 
> > On Tue Jan 16, 2024 at 7:25 PM GMT, Elias El Yandouzi wrote:
> >> diff --git a/xen/common/vmap.c b/xen/common/vmap.c
> >> index 171271fae3..966a7e763f 100644
> >> --- a/xen/common/vmap.c
> >> +++ b/xen/common/vmap.c
> >> @@ -245,6 +245,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr)
> >>      return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> >>  }
> >>  
> >> +void *vmap_contig(mfn_t mfn, unsigned int nr)
> >> +{
> >> +    return __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> >> +}
> >> +
> >>  unsigned int vmap_size(const void *va)
> >>  {
> >>      unsigned int pages = vm_size(va, VMAP_DEFAULT);
> > 
> > How is vmap_contig() different from regular vmap()?
> > 
> > vmap() calls map_pages_to_xen() `nr` times, while vmap_contig() calls it 
> > just
> > once. I'd expect both cases to work fine as they are. What am I missing? 
> > What
> > would make...
> > 
> >> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> >> index 389505f786..ab80d6b2a9 100644
> >> --- a/xen/drivers/acpi/osl.c
> >> +++ b/xen/drivers/acpi/osl.c
> >> @@ -221,7 +221,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_contig(mfn, PFN_UP(sz));
> > ... this statement not operate identically with regular vmap()? Or
> > probably more interestingly, what would preclude existing calls to vmap() 
> > not
> > operate under vmap_contig() instead?
>
> Note how vmap()'s first parameter is "const mfn_t *mfn". This needs to point
> to an array of "nr" MFNs. In order to use plain vmap() here, you'd first need
> to set up a suitably large array, populate if with increasing MFN values, and
> then make the call. Possible, but more complicated.
>
> Jan

I knew I must've been missing something. That pesky pointer... No wonder the
loop looked wonky. It was doing something completely different from what I
expected it to.

That clarifies it. Thanks a bunch, Jan.

Cheers,
Alejandro