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