Re: [PATCH 04/14] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn

From: Julien Grall <julien.grall@arm.com>
To: Jan Beulich <JBeulich@suse.com>, Tim Deegan <tim@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 04/14] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn
Date: Fri, 10 May 2019 14:07:40 +0100	[thread overview]
Message-ID: <3afc26ea-3cad-fb63-78f3-10084bd4f417@arm.com> (raw)
In-Reply-To: <5CD56B61020000780022D7FB@prv1-mh.provo.novell.com>

Hi Jan,

On 10/05/2019 13:15, Jan Beulich wrote:
>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote:
>> mfn_to_gfn and mfn_to_gmfn are doing exactly the same except the former
>> is using mfn_t.
> 
> ... and gfn_t (return type) as of patch 3.
> 
>> Furthermore, the naming of the former is more consistent with the
>> current naming scheme (GFN/MFN). So use replace mfn_to_gmfn with
>> mfn_to_gfn in x86 code.
> 
> Nit: Either "use" or "replace with", but not both.
> 
>> @@ -713,19 +713,20 @@ int arch_domain_soft_reset(struct domain *d)
>>       ASSERT( owner == d );
>>   
>>       mfn = page_to_mfn(page);
>> -    gfn = mfn_to_gmfn(d, mfn_x(mfn));
>> +    gfn = mfn_to_gfn(d, mfn);
>>   
>>       /*
>>        * gfn == INVALID_GFN indicates that the shared_info page was never mapped
>>        * to the domain's address space and there is nothing to replace.
>>        */
>> -    if ( gfn == gfn_x(INVALID_GFN) )
>> +    if ( gfn_eq(gfn, INVALID_GFN) )
>>           goto exit_put_page;
>>   
>> -    if ( !mfn_eq(get_gfn_query(d, gfn, &p2mt), mfn) )
>> +    if ( !mfn_eq(get_gfn_query(d, gfn_x(gfn), &p2mt), mfn) )
>>       {
>> -        printk(XENLOG_G_ERR "Failed to get Dom%d's shared_info GFN (%lx)\n",
>> -               d->domain_id, gfn);
>> +        printk(XENLOG_G_ERR
>> +               "Failed to get %pd's shared_info GFN (%"PRI_gfn")\n",
> 
> I'd recommend to drop the parentheses from the format string at the
> same time.
> 
>> @@ -733,31 +734,34 @@ int arch_domain_soft_reset(struct domain *d)
>>       new_page = alloc_domheap_page(d, 0);
>>       if ( !new_page )
>>       {
>> -        printk(XENLOG_G_ERR "Failed to alloc a page to replace"
>> -               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
>> +        printk(XENLOG_G_ERR
>> +               "Failed to alloc a page to replace %pd's shared_info frame %"PRI_gfn"\n",
> 
> s/frame/GFN/ to better match the earlier one? Same in the further log
> messages here then.
> 
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2632,19 +2632,20 @@ int free_page_type(struct page_info *page, unsigned long type,
>>   {
>>   #ifdef CONFIG_PV
>>       struct domain *owner = page_get_owner(page);
>> -    unsigned long gmfn;
>>       int rc;
>>   
>>       if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
>>       {
>> +        gfn_t gfn;
>> +
>>           /* A page table is dirtied when its type count becomes zero. */
>>           paging_mark_dirty(owner, page_to_mfn(page));
>>   
>>           ASSERT(!shadow_mode_refcounts(owner));
>>   
>> -        gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
>> -        if ( VALID_M2P(gmfn) )
>> -            shadow_remove_all_shadows(owner, _mfn(gmfn));
>> +        gfn = mfn_to_gfn(owner, page_to_mfn(page));
>> +        if ( VALID_M2P(gfn_x(gfn)) )
>> +            shadow_remove_all_shadows(owner, _mfn(gfn_x(gfn)));
>>       }
> 
> This is a highly suspicious change imo (albeit the code was bogus
> already before): It certainly isn't GFN here even if we were to assume
> translated mode could be in use. One other caller of
> the function, sh_page_fault() passes a variable named gmfn as well,
> but typed mfn_t (and this gmfn gets set from get_gfn(), i.e. is _not_
> a GFN). The 3rd one, shadow_prepare_page_type_change(), clearly
> passes an MFN.
> 
> I think the best course of action here is to split out the change,
> just to explain why removing the mfn_to_gmfn() here altogether
> is appropriate nowadays: PV guests can't be in translated mode
> anymore, and hence mfn_to_gmfn() doesn't do any translation. At
> that point the VALID_M2P() check can go away as well, so you'll be
> able to simply do
> 
>          shadow_remove_all_shadows(owner, page_to_mfn(page));
> 
> perhaps with another !shadow_mode_translate() assertion added
> next to the one that's already there. Tim, thoughts?
> 
> With this split out and irrespective of whether you decide to follow
> the format string suggestions further up

I don't have enough experience with x86 to provide the patch you suggest.

I am happy to rebase on top of any patch you provide. Alternatively I can drop 
this and keep mfn_to_gmfn on x86 but replaces the one in common code with 
mfn_to_gfn.

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

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel