1. Patchew
  2. Xen
  3. View series

[PATCH] fully replace mfn_to_gmfn()

Jan Beulich posted 1 patch 2 years, 10 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/389f5988-4700-da3e-e628-1513e87eb56a@suse.com
[PATCH] fully replace mfn_to_gmfn()
Posted by Jan Beulich 2 years, 10 months ago 
Convert the two remaining uses as well as Arm's stub to the properly
named and type-safe mfn_to_gfn(), dropping x86's definition (where we
already have mfn_to_gfn()).

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

--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -111,7 +111,8 @@ void getdomaininfo(struct domain *d, str
     info->outstanding_pages = d->outstanding_pages;
     info->shr_pages         = atomic_read(&d->shr_pages);
     info->paged_pages       = atomic_read(&d->paged_pages);
-    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
+    info->shared_info_frame =
+        gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
     BUG_ON(SHARED_M2P(info->shared_info_frame));
 
     info->cpupool = cpupool_get_id(d);
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -714,13 +714,13 @@ static long memory_exchange(XEN_GUEST_HA
          */
         while ( (page = page_list_remove_head(&in_chunk_list)) )
         {
-            unsigned long gfn;
+            gfn_t gfn;
 
             mfn = page_to_mfn(page);
-            gfn = mfn_to_gmfn(d, mfn_x(mfn));
+            gfn = mfn_to_gfn(d, mfn);
             /* Pages were unshared above */
-            BUG_ON(SHARED_M2P(gfn));
-            if ( guest_physmap_remove_page(d, _gfn(gfn), mfn, 0) )
+            BUG_ON(SHARED_M2P(gfn_x(gfn)));
+            if ( guest_physmap_remove_page(d, gfn, mfn, 0) )
                 domain_crash(d);
             free_domheap_page(page);
         }
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -328,8 +328,7 @@ struct page_info *get_page_from_gva(stru
 
 /* Xen always owns P2M on ARM */
 #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
-#define mfn_to_gmfn(_d, mfn)  (mfn)
-
+#define mfn_to_gfn(d, mfn) _gfn(mfn_x(mfn))
 
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -527,11 +527,6 @@ extern struct rangeset *mmio_ro_ranges;
 
 #define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
 
-#define mfn_to_gmfn(_d, mfn)                            \
-    ( (paging_mode_translate(_d))                       \
-      ? get_gpfn_from_mfn(mfn)                          \
-      : (mfn) )
-
 #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20))
 #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20))
Re: [PATCH] fully replace mfn_to_gmfn()
Posted by Julien Grall 2 years, 10 months ago 
Hi Jan,

On 28/06/2021 12:52, Jan Beulich wrote:
> Convert the two remaining uses as well as Arm's stub to the properly
> named and type-safe mfn_to_gfn(), dropping x86's definition (where we
> already have mfn_to_gfn()).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -111,7 +111,8 @@ void getdomaininfo(struct domain *d, str
>       info->outstanding_pages = d->outstanding_pages;
>       info->shr_pages         = atomic_read(&d->shr_pages);
>       info->paged_pages       = atomic_read(&d->paged_pages);
> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> +    info->shared_info_frame =
> +        gfn_x(mfn_to_gfn(d, _mfn(virt_to_mfn(d->shared_info))));
>       BUG_ON(SHARED_M2P(info->shared_info_frame));
>   
>       info->cpupool = cpupool_get_id(d);
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -714,13 +714,13 @@ static long memory_exchange(XEN_GUEST_HA
>            */
>           while ( (page = page_list_remove_head(&in_chunk_list)) )
>           {
> -            unsigned long gfn;
> +            gfn_t gfn;
>   
>               mfn = page_to_mfn(page);
> -            gfn = mfn_to_gmfn(d, mfn_x(mfn));
> +            gfn = mfn_to_gfn(d, mfn);
>               /* Pages were unshared above */
> -            BUG_ON(SHARED_M2P(gfn));
> -            if ( guest_physmap_remove_page(d, _gfn(gfn), mfn, 0) )
> +            BUG_ON(SHARED_M2P(gfn_x(gfn)));
> +            if ( guest_physmap_remove_page(d, gfn, mfn, 0) )
>                   domain_crash(d);
>               free_domheap_page(page);
>           }
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -328,8 +328,7 @@ struct page_info *get_page_from_gva(stru
>   
>   /* Xen always owns P2M on ARM */
>   #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
> -#define mfn_to_gmfn(_d, mfn)  (mfn)
> -
> +#define mfn_to_gfn(d, mfn) _gfn(mfn_x(mfn))

I still have a series pending to drop mfn_to_g{,m}fn() on Arm. But it 
has been stuck for quite a while on an agreement on for the toolstack 
interface (see [1]).

Anyway, this function is not worse than before. So:

Acked-by: Julien Grall <julien@xen.org>

Cheers,

[1] 
https://lore.kernel.org/xen-devel/32d4f762-a61d-bfdd-c4a8-38e5edef1aa8@xen.org/

-- 
Julien Grall
Re: [PATCH] fully replace mfn_to_gmfn()
Posted by Andrew Cooper 2 years, 10 months ago 
On 28/06/2021 12:52, Jan Beulich wrote:
> Convert the two remaining uses as well as Arm's stub to the properly
> named and type-safe mfn_to_gfn(), dropping x86's definition (where we
> already have mfn_to_gfn()).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but ...

> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -328,8 +328,7 @@ struct page_info *get_page_from_gva(stru
>  
>  /* Xen always owns P2M on ARM */
>  #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
> -#define mfn_to_gmfn(_d, mfn)  (mfn)
> -
> +#define mfn_to_gfn(d, mfn) _gfn(mfn_x(mfn))

... surely this wants to be ((void)(d), _gfn(mfn_x(mfn))), even if it's
just a latent bug right now?

~Andrew
Re: [PATCH] fully replace mfn_to_gmfn()
Posted by Jan Beulich 2 years, 10 months ago 
On 28.06.2021 17:42, Andrew Cooper wrote:
> On 28/06/2021 12:52, Jan Beulich wrote:
>> Convert the two remaining uses as well as Arm's stub to the properly
>> named and type-safe mfn_to_gfn(), dropping x86's definition (where we
>> already have mfn_to_gfn()).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but ...

Thanks.

>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -328,8 +328,7 @@ struct page_info *get_page_from_gva(stru
>>  
>>  /* Xen always owns P2M on ARM */
>>  #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>> -
>> +#define mfn_to_gfn(d, mfn) _gfn(mfn_x(mfn))
> 
> ... surely this wants to be ((void)(d), _gfn(mfn_x(mfn))), even if it's
> just a latent bug right now?

Well, Julien said he plans to get rid of this anyway. I'll do here
whatever the Arm maintainers say is wanted. Julien, Stefano?

Jan
Re: [PATCH] fully replace mfn_to_gmfn()
Posted by Julien Grall 2 years, 10 months ago 
Hi Jan,

On 28/06/2021 17:15, Jan Beulich wrote:
> On 28.06.2021 17:42, Andrew Cooper wrote:
>> On 28/06/2021 12:52, Jan Beulich wrote:
>>> Convert the two remaining uses as well as Arm's stub to the properly
>>> named and type-safe mfn_to_gfn(), dropping x86's definition (where we
>>> already have mfn_to_gfn()).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but ...
> 
> Thanks.
> 
>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -328,8 +328,7 @@ struct page_info *get_page_from_gva(stru
>>>   
>>>   /* Xen always owns P2M on ARM */
>>>   #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>>> -
>>> +#define mfn_to_gfn(d, mfn) _gfn(mfn_x(mfn))
>>
>> ... surely this wants to be ((void)(d), _gfn(mfn_x(mfn))), even if it's
>> just a latent bug right now?
> 
> Well, Julien said he plans to get rid of this anyway. I'll do here
> whatever the Arm maintainers say is wanted. Julien, Stefano?

I am fine with the change suggested by Andrew.

Cheers,

-- 
Julien Grall

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.