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

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

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?

I'm guessing it has to do with ARM having granules, but the looping logic seems
wonky in the non 4K case anyway seeing how the va jumps are based on PAGE_SIZE.

> +     }
>  
>       ptr = xmalloc_bytes(sz);
>       ASSERT(!ptr || is_xmalloc_memory(ptr));
> @@ -246,5 +250,11 @@ void __init acpi_os_free_memory(void *ptr)
>       if (is_xmalloc_memory(ptr))
>               xfree(ptr);
>       else if (ptr && system_state == SYS_STATE_early_boot)
> -             init_boot_pages(__pa(ptr), __pa(ptr) + PAGE_SIZE);
> +     {
> +             paddr_t addr = mfn_to_maddr(vmap_to_mfn(ptr));
> +             unsigned int nr = vmap_size(ptr);
> +
> +             vunmap(ptr);
> +             init_boot_pages(addr, addr + nr * PAGE_SIZE);
> +     }
>  }
> diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
> index 24c85de490..0c16baa85f 100644
> --- a/xen/include/xen/vmap.h
> +++ b/xen/include/xen/vmap.h
> @@ -15,6 +15,7 @@ void vm_init_type(enum vmap_region type, void *start, void 
> *end);
>  void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
>               unsigned int align, unsigned int flags, enum vmap_region type);
>  void *vmap(const mfn_t *mfn, unsigned int nr);
> +void *vmap_contig(mfn_t mfn, unsigned int nr);
>  void vunmap(const void *va);
>  
>  void *vmalloc(size_t size);