Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory

From: Jan Beulich <jbeulich@suse.com>
To: Hongyan Xia <hongyxia@amazon.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2] x86/mm: switch to new APIs in arch_init_memory
Date: Tue, 3 Mar 2020 10:21:35 +0100	[thread overview]
Message-ID: <bec9dda1-c5bb-9495-67e8-a76dec641706@suse.com> (raw)
In-Reply-To: <e8ba0fb1451ef89c36b21a2063590baed2432031.1582799255.git.hongyxia@amazon.com>

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