Re: [PATCH v10 01/13] x86/mm: rewrite virt_to_xen_l*e

From: Jan Beulich <jbeulich@suse.com>
To: Hongyan Xia <hx242@xen.org>
Cc: jgrall@amazon.com, "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v10 01/13] x86/mm: rewrite virt_to_xen_l*e
Date: Thu, 22 Apr 2021 13:54:30 +0200	[thread overview]
Message-ID: <b667c7ab-b12d-d46a-9318-925c4a19faaa@suse.com> (raw)
In-Reply-To: <0ccd3883b0ec2e05d600bb593f8f0afd058adc43.1619014052.git.hongyxia@amazon.com>

On 21.04.2021 16:15, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> Rewrite those functions to use the new APIs. Modify its callers to unmap
> the pointer returned. Since alloc_xen_pagetable_new() is almost never
> useful unless accompanied by page clearing and a mapping, introduce a
> helper alloc_map_clear_xen_pt() for this sequence.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> @@ -4941,33 +4961,33 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
>      if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
>      {
>          bool locking = system_state > SYS_STATE_boot;
> -        l3_pgentry_t *l3t = alloc_xen_pagetable();
> +        mfn_t l3mfn;
> +        l3_pgentry_t *l3t = alloc_mapped_pagetable(&l3mfn);
>  
>          if ( !l3t )
>              return NULL;
> -        clear_page(l3t);
> +        UNMAP_DOMAIN_PAGE(l3t);

... this immediate unmapping (and then re-mapping below) will imo
want re-doing down the road as well. Even if it's not a severe
performance hit, it's simply odd, at least to me.

Jan