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

[Dmytro Firsov]

On 19.07.22 12:50, Julien Grall wrote:
> 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?
>
No, U-boot memory allocator uses real RAM pages and one of them will be 
used for shared_info. Previously U-boot did not unmap it when jumped to 
Linux. So, during Linux run, one of the pages that is marked as RAM in 
device-tree will contain shared_info values. I don't know how it worked 
previously, but it definitely will cause weird issue when Linux will try 
to use it as regular RAM page.


>> 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://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20220716145658.4175730-2-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!2OWttkgakR7s6GUn6e60EweRQ44H-TyI-8wWzhX0vWzR33BJE_z-x_AZ0S5VBBnXwwQ73-eIQ-sNcGLf$ 
>> [lore[.]kernel[.]org]
>>
>> 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].
>
Yes, definitely, but with new patches this problem becomes critical and 
it will block Linux boot. General problem is explained in previous 
section. This patch is needed to U-boot even without new patches to Xen, 
but problem became visible only after them.


>> 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...)?

Yes, but only shared_info is mapped with XENMEM_add_to_physmap, so other 
drivers are not affected.


>
>>
>> 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?

I am using it on Armv8 and U-boot have hardcoded PAGE_SHIFT==12, and 
PAGE_SIZE==(1<<PAGE_SHIFT)==4K

So, 16K/64K is not possible in current implementation for Armv8.


>
>> +    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).

I did not see any padding in these structs definition in U-boot, so all 
fields are initialized. But I can add zeroing with memset in next 
version to be sure in this if structures will change.


>
>> +    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.

Agree, will be fixed in next version.


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

Agree, will be fixed in next version.


>
>> +     * 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()

Agree, will be fixed in next version.


>
>> +
>> +    /* 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://urldefense.com/v3/__https://github.com/xen-troops/u-boot/commit/f759b151116af204a5ab02ace82c09300cd6233a*__;Iw!!GF_29dbcQIUBPA!2OWttkgakR7s6GUn6e60EweRQ44H-TyI-8wWzhX0vWzR33BJE_z-x_AZ0S5VBBnXwwQ73-eIQ_0Hh8nA$ 
> [github[.]com]
>