* [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory
@ 2020-02-27 10:27 Hongyan Xia
2020-02-27 11:44 ` Wei Liu
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Hongyan Xia @ 2020-02-27 10:27 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Ian Jackson, George Dunlap, Hongyan Xia,
Jan Beulich, Roger Pau Monné
From: Wei Liu <wei.liu2@citrix.com>
The function will map and unmap pages on demand.
Since we now map and unmap Xen PTE pages, we would like to track the
lifetime of mappings so that 1) we do not dereference memory through a
variable after it is unmapped, 2) we do not unmap more than once.
Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the
variable after unmapping, and ignore NULL.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
---
Changed in v2:
- let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid
adding the check in unmap_domain_page.
- reword the commit message.
---
xen/arch/x86/mm.c | 14 ++++++++------
xen/include/xen/domain_page.h | 7 +++++++
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 70b87c4830..9fcdcde5b7 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -356,19 +356,21 @@ void __init arch_init_memory(void)
ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS);
if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) )
{
- l3_pgentry_t *l3tab = alloc_xen_pagetable();
+ mfn_t l3mfn = alloc_xen_pagetable_new();
- if ( l3tab )
+ if ( !mfn_eq(l3mfn, INVALID_MFN) )
{
- const l3_pgentry_t *l3idle =
- l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]);
+ l3_pgentry_t *l3idle = map_l3t_from_l4e(
+ idle_pg_table[l4_table_offset(split_va)]);
+ l3_pgentry_t *l3tab = map_domain_page(l3mfn);
for ( i = 0; i < l3_table_offset(split_va); ++i )
l3tab[i] = l3idle[i];
for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
l3tab[i] = l3e_empty();
- split_l4e = l4e_from_mfn(virt_to_mfn(l3tab),
- __PAGE_HYPERVISOR_RW);
+ split_l4e = l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW);
+ UNMAP_DOMAIN_PAGE(l3idle);
+ UNMAP_DOMAIN_PAGE(l3tab);
}
else
++root_pgt_pv_xen_slots;
diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
index 32669a3339..bfc3bf6aeb 100644
--- a/xen/include/xen/domain_page.h
+++ b/xen/include/xen/domain_page.h
@@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {};
#endif /* !CONFIG_DOMAIN_PAGE */
+#define UNMAP_DOMAIN_PAGE(p) do { \
+ if ( p ) { \
+ unmap_domain_page(p); \
+ (p) = NULL; \
+ } \
+} while ( false )
+
#endif /* __XEN_DOMAIN_PAGE_H__ */
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory
2020-02-27 10:27 [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory Hongyan Xia
@ 2020-02-27 11:44 ` Wei Liu
2020-02-27 11:51 ` Julien Grall
2020-03-03 9:21 ` Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: Wei Liu @ 2020-02-27 11:44 UTC (permalink / raw)
To: Hongyan Xia
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich,
xen-devel, Roger Pau Monné
On Thu, Feb 27, 2020 at 10:27:39AM +0000, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
>
> The function will map and unmap pages on demand.
>
> Since we now map and unmap Xen PTE pages, we would like to track the
> lifetime of mappings so that 1) we do not dereference memory through a
> variable after it is unmapped, 2) we do not unmap more than once.
> Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the
> variable after unmapping, and ignore NULL.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
>
> ---
> Changed in v2:
> - let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid
> adding the check in unmap_domain_page.
> - reword the commit message.
> ---
> xen/arch/x86/mm.c | 14 ++++++++------
> xen/include/xen/domain_page.h | 7 +++++++
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 70b87c4830..9fcdcde5b7 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -356,19 +356,21 @@ void __init arch_init_memory(void)
> ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS);
> if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) )
> {
> - l3_pgentry_t *l3tab = alloc_xen_pagetable();
> + mfn_t l3mfn = alloc_xen_pagetable_new();
>
> - if ( l3tab )
> + if ( !mfn_eq(l3mfn, INVALID_MFN) )
> {
> - const l3_pgentry_t *l3idle =
> - l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]);
> + l3_pgentry_t *l3idle = map_l3t_from_l4e(
> + idle_pg_table[l4_table_offset(split_va)]);
> + l3_pgentry_t *l3tab = map_domain_page(l3mfn);
>
> for ( i = 0; i < l3_table_offset(split_va); ++i )
> l3tab[i] = l3idle[i];
> for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
> l3tab[i] = l3e_empty();
> - split_l4e = l4e_from_mfn(virt_to_mfn(l3tab),
> - __PAGE_HYPERVISOR_RW);
> + split_l4e = l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW);
> + UNMAP_DOMAIN_PAGE(l3idle);
> + UNMAP_DOMAIN_PAGE(l3tab);
> }
> else
> ++root_pgt_pv_xen_slots;
> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
> index 32669a3339..bfc3bf6aeb 100644
> --- a/xen/include/xen/domain_page.h
> +++ b/xen/include/xen/domain_page.h
> @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {};
>
> #endif /* !CONFIG_DOMAIN_PAGE */
>
> +#define UNMAP_DOMAIN_PAGE(p) do { \
> + if ( p ) { \
Coding style mandates { should be placed in the next line.
Other than this, the code looks correct to me.
Wei.
> + unmap_domain_page(p); \
> + (p) = NULL; \
> + } \
> +} while ( false )
> +
> #endif /* __XEN_DOMAIN_PAGE_H__ */
> --
> 2.17.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory
2020-02-27 10:27 [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory Hongyan Xia
2020-02-27 11:44 ` Wei Liu
@ 2020-02-27 11:51 ` Julien Grall
2020-02-27 11:59 ` Xia, Hongyan
2020-03-03 9:21 ` Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2020-02-27 11:51 UTC (permalink / raw)
To: Hongyan Xia, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich,
Roger Pau Monné
Hi Hongyan,
On 27/02/2020 10:27, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
>
> The function will map and unmap pages on demand.
>
> Since we now map and unmap Xen PTE pages, we would like to track the
> lifetime of mappings so that 1) we do not dereference memory through a
> variable after it is unmapped, 2) we do not unmap more than once.
> Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the
> variable after unmapping, and ignore NULL.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
>
> ---
> Changed in v2:
> - let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid
> adding the check in unmap_domain_page.
> - reword the commit message.
> ---
> xen/arch/x86/mm.c | 14 ++++++++------
> xen/include/xen/domain_page.h | 7 +++++++
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 70b87c4830..9fcdcde5b7 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -356,19 +356,21 @@ void __init arch_init_memory(void)
> ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS);
> if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) )
> {
> - l3_pgentry_t *l3tab = alloc_xen_pagetable();
> + mfn_t l3mfn = alloc_xen_pagetable_new();
>
> - if ( l3tab )
> + if ( !mfn_eq(l3mfn, INVALID_MFN) )
> {
> - const l3_pgentry_t *l3idle =
> - l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]);
> + l3_pgentry_t *l3idle = map_l3t_from_l4e(
> + idle_pg_table[l4_table_offset(split_va)]);
> + l3_pgentry_t *l3tab = map_domain_page(l3mfn);
>
> for ( i = 0; i < l3_table_offset(split_va); ++i )
> l3tab[i] = l3idle[i];
> for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
> l3tab[i] = l3e_empty();
> - split_l4e = l4e_from_mfn(virt_to_mfn(l3tab),
> - __PAGE_HYPERVISOR_RW);
> + split_l4e = l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR_RW);
> + UNMAP_DOMAIN_PAGE(l3idle);
> + UNMAP_DOMAIN_PAGE(l3tab);
> }
> else
> ++root_pgt_pv_xen_slots;
> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
> index 32669a3339..bfc3bf6aeb 100644
> --- a/xen/include/xen/domain_page.h
> +++ b/xen/include/xen/domain_page.h
> @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {};
>
> #endif /* !CONFIG_DOMAIN_PAGE */
>
> +#define UNMAP_DOMAIN_PAGE(p) do { \
> + if ( p ) { \
> + unmap_domain_page(p); \
> + (p) = NULL; \
> + } \
> +} while ( false )
Do we need to keep the do {} while ()?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory
2020-02-27 11:51 ` Julien Grall
@ 2020-02-27 11:59 ` Xia, Hongyan
2020-02-27 12:03 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Xia, Hongyan @ 2020-02-27 11:59 UTC (permalink / raw)
To: julien, xen-devel
Cc: sstabellini, wl, konrad.wilk, andrew.cooper3, ian.jackson,
george.dunlap, jbeulich, roger.pau
On Thu, 2020-02-27 at 11:51 +0000, Julien Grall wrote:
> Hi Hongyan,
>
> On 27/02/2020 10:27, Hongyan Xia wrote:
> > ...
> > diff --git a/xen/include/xen/domain_page.h
> > b/xen/include/xen/domain_page.h
> > index 32669a3339..bfc3bf6aeb 100644
> > --- a/xen/include/xen/domain_page.h
> > +++ b/xen/include/xen/domain_page.h
> > @@ -72,4 +72,11 @@ static inline void
> > unmap_domain_page_global(const void *va) {};
> >
> > #endif /* !CONFIG_DOMAIN_PAGE */
> >
> > +#define UNMAP_DOMAIN_PAGE(p) do { \
> > + if ( p ) { \
> > + unmap_domain_page(p); \
> > + (p) = NULL; \
> > + } \
> > +} while ( false )
>
> Do we need to keep the do {} while ()?
I think we do. For example:
if ( cond )
UNMAP_DOMAIN_PAGE(p);
else
blah_blah_blah();
If we remove the do-while, the else clause will be paired with the if
in UNMAP_DOMAIN_PAGE();
Hongyan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory
2020-02-27 11:59 ` Xia, Hongyan
@ 2020-02-27 12:03 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2020-02-27 12:03 UTC (permalink / raw)
To: Xia, Hongyan, xen-devel
Cc: sstabellini, wl, konrad.wilk, andrew.cooper3, ian.jackson,
george.dunlap, jbeulich, roger.pau
Hi Hongyan,
On 27/02/2020 11:59, Xia, Hongyan wrote:
> On Thu, 2020-02-27 at 11:51 +0000, Julien Grall wrote:
>> Hi Hongyan,
>>
>> On 27/02/2020 10:27, Hongyan Xia wrote:
>>> ...
>>> diff --git a/xen/include/xen/domain_page.h
>>> b/xen/include/xen/domain_page.h
>>> index 32669a3339..bfc3bf6aeb 100644
>>> --- a/xen/include/xen/domain_page.h
>>> +++ b/xen/include/xen/domain_page.h
>>> @@ -72,4 +72,11 @@ static inline void
>>> unmap_domain_page_global(const void *va) {};
>>>
>>> #endif /* !CONFIG_DOMAIN_PAGE */
>>>
>>> +#define UNMAP_DOMAIN_PAGE(p) do { \
>>> + if ( p ) { \
>>> + unmap_domain_page(p); \
>>> + (p) = NULL; \
>>> + } \
>>> +} while ( false )
>>
>> Do we need to keep the do {} while ()?
>
> I think we do. For example:
>
> if ( cond )
> UNMAP_DOMAIN_PAGE(p);
> else
> blah_blah_blah();
>
> If we remove the do-while, the else clause will be paired with the if
> in UNMAP_DOMAIN_PAGE();
GCC will actually throw a compiler error:
test.c: In function ‘f’:
test.c:13:5: error: ‘else’ without a previous ‘if’
else
^~~~
Anyway, yes we do need to keep do while {} to catch any use without the
semicolon. Sorry for the noise.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory
2020-02-27 10:27 [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory Hongyan Xia
2020-02-27 11:44 ` Wei Liu
2020-02-27 11:51 ` Julien Grall
@ 2020-03-03 9:21 ` Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2020-03-03 9:21 UTC (permalink / raw)
To: Hongyan Xia
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, Ian Jackson, George Dunlap, xen-devel,
Roger Pau Monné
On 27.02.2020 11:27, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
>
> The function will map and unmap pages on demand.
>
> Since we now map and unmap Xen PTE pages, we would like to track the
> lifetime of mappings so that 1) we do not dereference memory through a
> variable after it is unmapped, 2) we do not unmap more than once.
> Therefore, we introduce the UNMAP_DOMAIN_PAGE macro to nullify the
> variable after unmapping, and ignore NULL.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
>
> ---
> Changed in v2:
> - let UNMAP_DOMAIN_PAGE itself check whether the input is NULL to avoid
> adding the check in unmap_domain_page.
> - reword the commit message.
> ---
> xen/arch/x86/mm.c | 14 ++++++++------
> xen/include/xen/domain_page.h | 7 +++++++
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 70b87c4830..9fcdcde5b7 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -356,19 +356,21 @@ void __init arch_init_memory(void)
> ASSERT(root_pgt_pv_xen_slots < ROOT_PAGETABLE_PV_XEN_SLOTS);
> if ( l4_table_offset(split_va) == l4_table_offset(split_va - 1) )
> {
> - l3_pgentry_t *l3tab = alloc_xen_pagetable();
> + mfn_t l3mfn = alloc_xen_pagetable_new();
>
> - if ( l3tab )
> + if ( !mfn_eq(l3mfn, INVALID_MFN) )
> {
> - const l3_pgentry_t *l3idle =
> - l4e_to_l3e(idle_pg_table[l4_table_offset(split_va)]);
> + l3_pgentry_t *l3idle = map_l3t_from_l4e(
> + idle_pg_table[l4_table_offset(split_va)]);
Somehow you've lost the const. I think both this and ...
> --- a/xen/include/xen/domain_page.h
> +++ b/xen/include/xen/domain_page.h
> @@ -72,4 +72,11 @@ static inline void unmap_domain_page_global(const void *va) {};
>
> #endif /* !CONFIG_DOMAIN_PAGE */
>
> +#define UNMAP_DOMAIN_PAGE(p) do { \
> + if ( p ) { \
... the brace placement here can be dealt with while committing.
With both of them in place
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-03 9:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 10:27 [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory Hongyan Xia
2020-02-27 11:44 ` Wei Liu
2020-02-27 11:51 ` Julien Grall
2020-02-27 11:59 ` Xia, Hongyan
2020-02-27 12:03 ` Julien Grall
2020-03-03 9:21 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).