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-olekst...@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_fir...@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] >