All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drivers: xen: unmap Enlighten page before jumping to Linux
@ 2022-07-19  8:52 Dmytro Firsov
  2022-07-19  9:50 ` Julien Grall
  0 siblings, 1 reply; 5+ messages in thread
From: Dmytro Firsov @ 2022-07-19  8:52 UTC (permalink / raw)
  To: u-boot; +Cc: olekstysh, vicooodin, julien, Dmytro Firsov

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. 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://lore.kernel.org/xen-devel/20220716145658.4175730-2-olekstysh@gmail.com/

So, without unmapping in U-boot, Linux will fail to start. 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.

Signed-off-by: Dmytro Firsov <dmytro_firsov@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);
+	struct xen_remove_from_physmap xrfp;
+	struct xen_memory_reservation reservation;
+	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();
+
+	/*
+	 * After remove from physmap there will be a hole in address space on
+	 * 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();
+
+	/* 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();
 }
-- 
2.25.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers: xen: unmap Enlighten page before jumping to Linux
  2022-07-19  8:52 [PATCH] drivers: xen: unmap Enlighten page before jumping to Linux Dmytro Firsov
@ 2022-07-19  9:50 ` Julien Grall
  2022-07-19 10:45   ` Dmytro Firsov
  0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2022-07-19  9:50 UTC (permalink / raw)
  To: Dmytro Firsov, u-boot; +Cc: olekstysh, vicooodin

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?

> 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://lore.kernel.org/xen-devel/20220716145658.4175730-2-olekstysh@gmail.com/
> 
> 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].

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

> 
> Signed-off-by: Dmytro Firsov <dmytro_firsov@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?

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

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

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

Typo: s/remove/removing/

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

> +
> +	/* 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://github.com/xen-troops/u-boot/commit/f759b151116af204a5ab02ace82c09300cd6233a#

-- 
Julien Grall

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers: xen: unmap Enlighten page before jumping to Linux
  2022-07-19  9:50 ` Julien Grall
@ 2022-07-19 10:45   ` Dmytro Firsov
  2022-07-19 11:04     ` Julien Grall
  0 siblings, 1 reply; 5+ messages in thread
From: Dmytro Firsov @ 2022-07-19 10:45 UTC (permalink / raw)
  To: Julien Grall, u-boot; +Cc: olekstysh, vicooodin


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers: xen: unmap Enlighten page before jumping to Linux
  2022-07-19 10:45   ` Dmytro Firsov
@ 2022-07-19 11:04     ` Julien Grall
  2022-07-19 13:03       ` Dmytro Firsov
  0 siblings, 1 reply; 5+ messages in thread
From: Julien Grall @ 2022-07-19 11:04 UTC (permalink / raw)
  To: Dmytro Firsov, u-boot; +Cc: olekstysh, vicooodin

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers: xen: unmap Enlighten page before jumping to Linux
  2022-07-19 11:04     ` Julien Grall
@ 2022-07-19 13:03       ` Dmytro Firsov
  0 siblings, 0 replies; 5+ messages in thread
From: Dmytro Firsov @ 2022-07-19 13:03 UTC (permalink / raw)
  To: Julien Grall, u-boot; +Cc: olekstysh, vicooodin


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-07-19 15:31 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19  8:52 [PATCH] drivers: xen: unmap Enlighten page before jumping to Linux Dmytro Firsov
2022-07-19  9:50 ` Julien Grall
2022-07-19 10:45   ` Dmytro Firsov
2022-07-19 11:04     ` Julien Grall
2022-07-19 13:03       ` Dmytro Firsov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.