[PATCH] drivers: xen: unmap Enlighten page before jumping to Linux

[Julien Grall]

Hi Dmytro,

On 19/07/2022 09:52, Dmytro Firsov wrote:
> This commit fixes issue with usage of Xen hypervisor shared info page.
> Previously U-boot did not unmap it at the end of OS boot process. It
> leads to repeated mapping during Enlighten initialization in Linux.
> Xen did not prevent guest from this, so it works but causes weird
> wierd issues - one memory page, that was returned by memalign in U-boot
> for Enlighten mapping was not returned to allocator and contained
> shared info values during Linux run.

I can't quite parse this. Do you mean the page will be marked as 
reserved for Linux and therefore it will never reach Linux's page allocator?

> Also Linux mapped it to another
> place in memory again. >
> Now code, that restricts repeated mapping of Enlighten page, is about
> to be merged to Xen:
> https://lore.kernel.org/xen-devel/20220716145658.4175730-2-olekstysh@gmail.com/
> 
> So, without unmapping in U-boot, Linux will fail to start.

Hmmm... From a discussion with Oleksandr, I was under the impression 
that this patch would also be necessary without the Xen patch merged. 
This was in order to prevent a corruption of the shared page [1].

> As discussed
> on the xen-devel mailing list, to fix this issue the code should:
>     1) Unmap the page
>     2) Populate the area with memory using XENMEM_populate_physmap
> 
> This patch adds page unmapping via XENMEM_remove_from_physmap, fills
> hole in address space where page was mapped via XENMEM_populate_physmap
> and return this address to memory allocator for freeing.

Is U-boot mapping other shared page from Xen (e.g. grant-table...)?

> 
> Signed-off-by: Dmytro Firsov <dmytro_firsov at epam.com>
> ---
>   drivers/xen/hypervisor.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/xen/hypervisor.c b/drivers/xen/hypervisor.c
> index 2560894832..9ac377b618 100644
> --- a/drivers/xen/hypervisor.c
> +++ b/drivers/xen/hypervisor.c
> @@ -144,6 +144,36 @@ struct shared_info *map_shared_info(void *p)
>   	return HYPERVISOR_shared_info;
>   }
>   
> +void unmap_shared_info(void)
> +{
> +	xen_pfn_t shared_info_pfn = virt_to_pfn(HYPERVISOR_shared_info);

Somewhat unrelated to this patch. Can U-boot be compiled with 16K/64K 
page granularity?

> +	struct xen_remove_from_physmap xrfp;
> +	struct xen_memory_reservation reservation;

AFAICT, there some paddings in the two structure. So I would suggest to 
zero the structure (including paddings).

> +	xen_ulong_t nr_exts = 1;
> +
> +	xrfp.domid = DOMID_SELF;
> +	xrfp.gpfn = shared_info_pfn;
> +	if (HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrfp) != 0)
> +		BUG();

If I am not mistaken, U-boot provide a panic() helper. So I would 
suggest to use it as this would be useful to print with the error returned.

> +
> +	/*
> +	 * After remove from physmap there will be a hole in address space on

Typo: s/remove/removing/

> +	 * HYPERVISOR_shared_info address, so to free memory allocated with
> +	 * memalign and prevent exceptions during access to this page we need to
> +	 * fill this 4KB hole with XENMEM_populate_physmap before jumping to Linux.
> +	 */
> +	reservation.domid = DOMID_SELF;
> +	reservation.extent_order = 0;
> +	reservation.address_bits = 0;
> +	set_xen_guest_handle(reservation.extent_start, &shared_info_pfn);
> +	reservation.nr_extents = nr_exts;
> +	if (HYPERVISOR_memory_op(XENMEM_populate_physmap, &reservation) != nr_exts)
> +		BUG();

Same here regarding using panic() rather than BUG()

> +
> +	/* Now we can return this to memory allocator */
> +	free(HYPERVISOR_shared_info);
> +}
> +
>   void do_hypervisor_callback(struct pt_regs *regs)
>   {
>   	unsigned long l1, l2, l1i, l2i;
> @@ -251,4 +281,5 @@ void xen_fini(void)
>   	fini_gnttab();
>   	fini_xenbus();
>   	fini_events();
> +	unmap_shared_info();
>   }

[1] 
https://github.com/xen-troops/u-boot/commit/f759b151116af204a5ab02ace82c09300cd6233a#

-- 
Julien Grall