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

[Dmytro Firsov]

On 19.07.22 14:04, Julien Grall wrote:
> 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.

Okay, I argee. I will reword commit message with focus on corruption.


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

Yes, grant-tables are mapped outside of actual RAM, so there is no such 
problem.


>
>
>>>
>>>> +    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.
Okay, thanks, I will add this to second version.
>
> Cheers,
>