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

[Julien Grall]

Hi,

On 19/07/2022 11:45, Dmytro Firsov wrote:
> 
> 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.

Ok. I would suggest to reword the commit message.

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

I would argue that it was more critical before because you would end up 
with some random corruption of the shared page. At least, Oleksandr 
patch bring up some certainty because now Linux can't boot.

> 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.
See above, I think the commit message should focus on the corruption 
rather than Xen forbidding double map. So it is clear that this patch is 
not to paper over a new issue in Xen (which would technically be a 
regression) but fixing a *real* problem on any Xen version.

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

Hmmmm... A grep in u-boot source shows that XENMEM_add_to_physmap is 
used to map grant-table frame. However, the driver seems to use the 
unallocated address space provided by Xen. So you are fine there.

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

There are no explicit padding but there are some implicit one. If you 
use pahole, you will see:

struct xen_remove_from_physmap {
         domid_t                    domid;                /*     0     2 */

         /* XXX 6 bytes hole, try to pack */

         xen_pfn_t                  gpfn;                 /*     8     8 */

         /* size: 16, cachelines: 1, members: 2 */
         /* sum members: 10, holes: 1, sum holes: 6 */
         /* last cacheline: 16 bytes */
};


struct xen_memory_reservation {
         __guest_handle_64_xen_pfn_t extent_start;        /*     0     8 */
         xen_ulong_t                nr_extents;           /*     8     8 */
         unsigned int               extent_order;         /*    16     4 */
         unsigned int               mem_flags;            /*    20     4 */
         domid_t                    domid;                /*    24     2 */

         /* Force padding: */
         domid_t                    :16;
         domid_t                    :16;
         domid_t                    :16;

         /* size: 32, cachelines: 1, members: 5 */
         /* padding: 6 */
         /* last cacheline: 32 bytes */
};

> But I can add zeroing with memset in next
> version to be sure in this if structures will change.

AFAIK, = {} should also do the job.

Cheers,

-- 
Julien Grall