[Xen-devel] [PATCH 04/14] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn

Julien Grall posted 14 patches 15 weeks ago

[Xen-devel] [PATCH 04/14] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn

Posted by Julien Grall 15 weeks ago
mfn_to_gfn and mfn_to_gmfn are doing exactly the same except the former
is using mfn_t.

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.

Take the opportunity to convert some of the callers to use typesafe GFN and
format the message correctly.

No functional changes.

Signed-off-by: Julien Grall <julien.grall@arm.com>

--
    Changes in v2:
        - mfn_to_gfn now returns a gfn_t
        - Use %pd and PRI_gfn when possible in the message
        - Don't split format string to help grep/ack.
---
 xen/arch/x86/domain.c               | 34 +++++++++++++++++++---------------
 xen/arch/x86/mm.c                   |  9 +++++----
 xen/arch/x86/pv/emul-priv-op.c      |  4 ++--
 xen/drivers/passthrough/x86/iommu.c | 16 +++++++++-------
 4 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9eaa978ce5..8d29dfeecc 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -679,7 +679,7 @@ int arch_domain_soft_reset(struct domain *d)
     int ret = 0;
     struct domain *owner;
     mfn_t mfn;
-    unsigned long gfn;
+    gfn_t gfn;
     p2m_type_t p2mt;
     unsigned int i;
 
@@ -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",
+               d, gfn_x(gfn));
         ret = -EINVAL;
         goto exit_put_gfn;
     }
@@ -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",
+               d, gfn_x(gfn));
         ret = -ENOMEM;
         goto exit_put_gfn;
     }
 
-    ret = guest_physmap_remove_page(d, _gfn(gfn), mfn, PAGE_ORDER_4K);
+    ret = guest_physmap_remove_page(d, gfn, mfn, PAGE_ORDER_4K);
     if ( ret )
     {
-        printk(XENLOG_G_ERR "Failed to remove Dom%d's shared_info frame %lx\n",
-               d->domain_id, gfn);
+        printk(XENLOG_G_ERR
+               "Failed to remove %pd's shared_info frame %"PRI_gfn"\n",
+               d, gfn_x(gfn));
         free_domheap_page(new_page);
         goto exit_put_gfn;
     }
 
-    ret = guest_physmap_add_page(d, _gfn(gfn), page_to_mfn(new_page),
+    ret = guest_physmap_add_page(d, gfn, page_to_mfn(new_page),
                                  PAGE_ORDER_4K);
     if ( ret )
     {
-        printk(XENLOG_G_ERR "Failed to add a page to replace"
-               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
+        printk(XENLOG_G_ERR
+               "Failed to add a page to replace %pd's shared_info frame %"PRI_gfn"\n",
+               d, gfn_x(gfn));
         free_domheap_page(new_page);
     }
  exit_put_gfn:
-    put_gfn(d, gfn);
+    put_gfn(d, gfn_x(gfn));
  exit_put_page:
     put_page(page);
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 45fadbab61..9878453eb0 100644
--- 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)));
     }
 
     if ( !(type & PGT_partial) )
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index af74f50dc8..e976ff9898 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -712,7 +712,7 @@ static int read_cr(unsigned int reg, unsigned long *val,
         if ( !is_pv_32bit_domain(currd) )
         {
             mfn = pagetable_get_mfn(curr->arch.guest_table);
-            *val = xen_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn)));
+            *val = xen_pfn_to_cr3(gfn_x(mfn_to_gfn(currd, mfn)));
         }
         else
         {
@@ -721,7 +721,7 @@ static int read_cr(unsigned int reg, unsigned long *val,
 
             mfn = l4e_get_mfn(*pl4e);
             unmap_domain_page(pl4e);
-            *val = compat_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn)));
+            *val = compat_pfn_to_cr3(gfn_x(mfn_to_gfn(currd, mfn)));
         }
         /* PTs should not be shared */
         BUG_ON(page_get_owner(mfn_to_page(mfn)) == dom_cow);
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 034ac903dd..7a756ef19e 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -92,15 +92,17 @@ int arch_iommu_populate_page_table(struct domain *d)
         if ( is_hvm_domain(d) ||
             (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
         {
-            unsigned long mfn = mfn_x(page_to_mfn(page));
-            unsigned long gfn = mfn_to_gmfn(d, mfn);
+            mfn_t mfn = page_to_mfn(page);
+            gfn_t gfn = mfn_to_gfn(d, mfn);
             unsigned int flush_flags = 0;
 
-            if ( gfn != gfn_x(INVALID_GFN) )
+            if ( !gfn_eq(gfn, INVALID_GFN) )
             {
-                ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
-                BUG_ON(SHARED_M2P(gfn));
-                rc = iommu_map(d, _dfn(gfn), _mfn(mfn), PAGE_ORDER_4K,
+                dfn_t dfn = _dfn(gfn_x(gfn));
+
+                ASSERT(!(gfn_x(gfn) >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
+                BUG_ON(SHARED_M2P(gfn_x(gfn)));
+                rc = iommu_map(d, dfn, mfn, PAGE_ORDER_4K,
                                IOMMUF_readable | IOMMUF_writable,
                                &flush_flags);
 
@@ -118,7 +120,7 @@ int arch_iommu_populate_page_table(struct domain *d)
                      ((page->u.inuse.type_info & PGT_type_mask) !=
                       PGT_writable_page) )
                 {
-                    rc = iommu_unmap(d, _dfn(gfn), PAGE_ORDER_4K, &flush_flags);
+                    rc = iommu_unmap(d, dfn, PAGE_ORDER_4K, &flush_flags);
                     /* If the type changed yet again, simply force a retry. */
                     if ( !rc && ((page->u.inuse.type_info & PGT_type_mask) ==
                                  PGT_writable_page) )
-- 
2.11.0


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

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

Posted by Jan Beulich 15 weeks ago
>>> 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
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

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

Posted by Julien Grall 15 weeks ago
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