[PATCH] x86/mm: free_page_type() is PV-only

[PATCH] x86/mm: free_page_type() is PV-only

[Jan Beulich]

While it already has a CONFIG_PV wrapped around its entire body, it is
still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
Avoid morphing this code into even more suspicious shape and remove the
effectively dead code - translated mode has been made impossible for PV
quite some time ago.

Adjust and extend the assertions at the same time: The original
ASSERT(!shadow_mode_refcounts(owner)) really means
ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
which isn't what we want here.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag
 {
 #ifdef CONFIG_PV
     struct domain *owner = page_get_owner(page);
-    unsigned long gmfn;
     int rc;
 
     if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
@@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
         /* A page table is dirtied when its type count becomes zero. */
         paging_mark_dirty(owner, page_to_mfn(page));
 
-        ASSERT(!shadow_mode_refcounts(owner));
+        ASSERT(shadow_mode_enabled(owner));
+        ASSERT(!paging_mode_refcounts(owner));
+        ASSERT(!paging_mode_translate(owner));
 
-        gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
-        if ( VALID_M2P(gmfn) )
-            shadow_remove_all_shadows(owner, _mfn(gmfn));
+        shadow_remove_all_shadows(owner, page_to_mfn(page));
     }
 
     if ( !(type & PGT_partial) )





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

[Xen-devel] [PATCH] x86/mm: free_page_type() is PV-only

[Jan Beulich]

While it already has a CONFIG_PV wrapped around its entire body, it is
still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
Avoid morphing this code into even more suspicious shape and remove the
effectively dead code - translated mode has been made impossible for PV
quite some time ago.

Adjust and extend the assertions at the same time: The original
ASSERT(!shadow_mode_refcounts(owner)) really means
ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
which isn't what we want here.

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

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag
 {
 #ifdef CONFIG_PV
     struct domain *owner = page_get_owner(page);
-    unsigned long gmfn;
     int rc;
 
     if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
@@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
         /* A page table is dirtied when its type count becomes zero. */
         paging_mark_dirty(owner, page_to_mfn(page));
 
-        ASSERT(!shadow_mode_refcounts(owner));
+        ASSERT(shadow_mode_enabled(owner));
+        ASSERT(!paging_mode_refcounts(owner));
+        ASSERT(!paging_mode_translate(owner));
 
-        gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
-        if ( VALID_M2P(gmfn) )
-            shadow_remove_all_shadows(owner, _mfn(gmfn));
+        shadow_remove_all_shadows(owner, page_to_mfn(page));
     }
 
     if ( !(type & PGT_partial) )





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

Re: [PATCH] x86/mm: free_page_type() is PV-only

[Julien Grall]

Hi Jan,

Thank you for sending the patch! I will rebase my M2P series on top of this.

Cheers,

On 10/05/2019 15:12, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
> 
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag
>   {
>   #ifdef CONFIG_PV
>       struct domain *owner = page_get_owner(page);
> -    unsigned long gmfn;
>       int rc;
>   
>       if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
> @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
>           /* A page table is dirtied when its type count becomes zero. */
>           paging_mark_dirty(owner, page_to_mfn(page));
>   
> -        ASSERT(!shadow_mode_refcounts(owner));
> +        ASSERT(shadow_mode_enabled(owner));
> +        ASSERT(!paging_mode_refcounts(owner));
> +        ASSERT(!paging_mode_translate(owner));
>   
> -        gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
> -        if ( VALID_M2P(gmfn) )
> -            shadow_remove_all_shadows(owner, _mfn(gmfn));
> +        shadow_remove_all_shadows(owner, page_to_mfn(page));
>       }
>   
>       if ( !(type & PGT_partial) )
> 
> 
> 
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] x86/mm: free_page_type() is PV-only

[Julien Grall]

Hi Jan,

Thank you for sending the patch! I will rebase my M2P series on top of this.

Cheers,

On 10/05/2019 15:12, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
> 
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag
>   {
>   #ifdef CONFIG_PV
>       struct domain *owner = page_get_owner(page);
> -    unsigned long gmfn;
>       int rc;
>   
>       if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
> @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
>           /* A page table is dirtied when its type count becomes zero. */
>           paging_mark_dirty(owner, page_to_mfn(page));
>   
> -        ASSERT(!shadow_mode_refcounts(owner));
> +        ASSERT(shadow_mode_enabled(owner));
> +        ASSERT(!paging_mode_refcounts(owner));
> +        ASSERT(!paging_mode_translate(owner));
>   
> -        gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
> -        if ( VALID_M2P(gmfn) )
> -            shadow_remove_all_shadows(owner, _mfn(gmfn));
> +        shadow_remove_all_shadows(owner, page_to_mfn(page));
>       }
>   
>       if ( !(type & PGT_partial) )
> 
> 
> 
> 

-- 
Julien Grall

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

Re: [PATCH] x86/mm: free_page_type() is PV-only

[Andrew Cooper]

On 10/05/2019 15:12, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
>
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [Xen-devel] [PATCH] x86/mm: free_page_type() is PV-only

[Andrew Cooper]

On 10/05/2019 15:12, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
>
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

Re: [PATCH] x86/mm: free_page_type() is PV-only

[Wei Liu]

On Fri, May 10, 2019 at 08:12:51AM -0600, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
> 
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [Xen-devel] [PATCH] x86/mm: free_page_type() is PV-only

[Wei Liu]

On Fri, May 10, 2019 at 08:12:51AM -0600, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
> 
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH] x86/mm: free_page_type() is PV-only

[George Dunlap]

On 5/10/19 3:12 PM, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
> 
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag
>  {
>  #ifdef CONFIG_PV
>      struct domain *owner = page_get_owner(page);
> -    unsigned long gmfn;
>      int rc;
>  
>      if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
> @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
>          /* A page table is dirtied when its type count becomes zero. */
>          paging_mark_dirty(owner, page_to_mfn(page));
>  
> -        ASSERT(!shadow_mode_refcounts(owner));
> +        ASSERT(shadow_mode_enabled(owner));
> +        ASSERT(!paging_mode_refcounts(owner));
> +        ASSERT(!paging_mode_translate(owner));

In the context of my patch to CODING_STYLE about the use of ASSERTs,
thinking about ASSERT vs BUG_ON vs something else here.  I guess in this
case:

1. PV guests can't be in translate mode
2. If that ever changed, we'd probably trip over the ASSERT() while
debugging

So I guess ASSERT() is probably fine.

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

So does that mean, though, that SHARED_M2P_ENTRY & friends are entirely
vestigal now, and can be removed?

 -George

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

Re: [Xen-devel] [PATCH] x86/mm: free_page_type() is PV-only

[George Dunlap]

On 5/10/19 3:12 PM, Jan Beulich wrote:
> While it already has a CONFIG_PV wrapped around its entire body, it is
> still uselessly invoking mfn_to_gmfn(), which is about to be replaced.
> Avoid morphing this code into even more suspicious shape and remove the
> effectively dead code - translated mode has been made impossible for PV
> quite some time ago.
> 
> Adjust and extend the assertions at the same time: The original
> ASSERT(!shadow_mode_refcounts(owner)) really means
> ASSERT(!shadow_mode_enabled(owner) || !paging_mode_refcounts(owner)),
> which isn't what we want here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2632,7 +2632,6 @@ int free_page_type(struct page_info *pag
>  {
>  #ifdef CONFIG_PV
>      struct domain *owner = page_get_owner(page);
> -    unsigned long gmfn;
>      int rc;
>  
>      if ( likely(owner != NULL) && unlikely(paging_mode_enabled(owner)) )
> @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
>          /* A page table is dirtied when its type count becomes zero. */
>          paging_mark_dirty(owner, page_to_mfn(page));
>  
> -        ASSERT(!shadow_mode_refcounts(owner));
> +        ASSERT(shadow_mode_enabled(owner));
> +        ASSERT(!paging_mode_refcounts(owner));
> +        ASSERT(!paging_mode_translate(owner));

In the context of my patch to CODING_STYLE about the use of ASSERTs,
thinking about ASSERT vs BUG_ON vs something else here.  I guess in this
case:

1. PV guests can't be in translate mode
2. If that ever changed, we'd probably trip over the ASSERT() while
debugging

So I guess ASSERT() is probably fine.

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

So does that mean, though, that SHARED_M2P_ENTRY & friends are entirely
vestigal now, and can be removed?

 -George

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

Re: [PATCH] x86/mm: free_page_type() is PV-only

[Jan Beulich]

>>> On 13.05.19 at 12:33, <george.dunlap@citrix.com> wrote:
> On 5/10/19 3:12 PM, Jan Beulich wrote:
>> @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
>>          /* A page table is dirtied when its type count becomes zero. */
>>          paging_mark_dirty(owner, page_to_mfn(page));
>>  
>> -        ASSERT(!shadow_mode_refcounts(owner));
>> +        ASSERT(shadow_mode_enabled(owner));
>> +        ASSERT(!paging_mode_refcounts(owner));
>> +        ASSERT(!paging_mode_translate(owner));
> 
> In the context of my patch to CODING_STYLE about the use of ASSERTs,
> thinking about ASSERT vs BUG_ON vs something else here.  I guess in this
> case:
> 
> 1. PV guests can't be in translate mode
> 2. If that ever changed, we'd probably trip over the ASSERT() while
> debugging
> 
> So I guess ASSERT() is probably fine.

Right, in other (more likely to be [wrongly] exposed to actual
execution) cases I'd probably not have used plain ASSERT()
here.

> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks.

> So does that mean, though, that SHARED_M2P_ENTRY & friends are entirely
> vestigal now, and can be removed?

No, it's pointless to use here only because there's no M2P
translation done here in the first place, due to the code
being PV only. In code paths reachable for HVM these
ought to still be necessary.

Jan



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

Re: [Xen-devel] [PATCH] x86/mm: free_page_type() is PV-only

[Jan Beulich]

>>> On 13.05.19 at 12:33, <george.dunlap@citrix.com> wrote:
> On 5/10/19 3:12 PM, Jan Beulich wrote:
>> @@ -2640,11 +2639,11 @@ int free_page_type(struct page_info *pag
>>          /* A page table is dirtied when its type count becomes zero. */
>>          paging_mark_dirty(owner, page_to_mfn(page));
>>  
>> -        ASSERT(!shadow_mode_refcounts(owner));
>> +        ASSERT(shadow_mode_enabled(owner));
>> +        ASSERT(!paging_mode_refcounts(owner));
>> +        ASSERT(!paging_mode_translate(owner));
> 
> In the context of my patch to CODING_STYLE about the use of ASSERTs,
> thinking about ASSERT vs BUG_ON vs something else here.  I guess in this
> case:
> 
> 1. PV guests can't be in translate mode
> 2. If that ever changed, we'd probably trip over the ASSERT() while
> debugging
> 
> So I guess ASSERT() is probably fine.

Right, in other (more likely to be [wrongly] exposed to actual
execution) cases I'd probably not have used plain ASSERT()
here.

> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks.

> So does that mean, though, that SHARED_M2P_ENTRY & friends are entirely
> vestigal now, and can be removed?

No, it's pointless to use here only because there's no M2P
translation done here in the first place, due to the code
being PV only. In code paths reachable for HVM these
ought to still be necessary.

Jan



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