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