Re: [PATCH 2/4] x86/P2M: un-indent write_p2m_entry()

[Jan Beulich]

On 24.04.2024 11:16, Roger Pau Monné wrote:
> On Tue, Apr 23, 2024 at 04:32:14PM +0200, Jan Beulich wrote:
>> Drop the inner scope that was left from earlier if/else removal. Take
>> the opportunity and make the paging_unlock() invocation common to
>> success and error paths, though.
> 
> TBH I'm not sure I prefer the fact to continue function execution
> after an error is found, I specially dislike that you have to add a
> !rc check to the nestedhvm conditional block, and because anything
> that we further add to the function would also need a !rc check.
> 
>>
>> Signed-off-by: Jan Beulich 
> 
> Acked-by: Roger Pau Monné 

Thanks.

> Albeit I do prefer the extra call to paging_unlock() and early return
> from the function in case of error.

Which puts me in the middle of your preference and the one George voiced
in the context of what is now cc950c49ae6a ("x86/PoD: tie together P2M
update and increment of entry count"). Doing the extra adjustment was
merely in the hope of meeting his desires ...

Jan

Re: [PATCH 2/4] x86/P2M: un-indent write_p2m_entry()

[Roger Pau Monné]

On Tue, Apr 23, 2024 at 04:32:14PM +0200, Jan Beulich wrote:
> Drop the inner scope that was left from earlier if/else removal. Take
> the opportunity and make the paging_unlock() invocation common to
> success and error paths, though.

TBH I'm not sure I prefer the fact to continue function execution
after an error is found, I specially dislike that you have to add a
!rc check to the nestedhvm conditional block, and because anything
that we further add to the function would also need a !rc check.

> 
> Signed-off-by: Jan Beulich 

Acked-by: Roger Pau Monné 

Albeit I do prefer the extra call to paging_unlock() and early return
from the function in case of error.

Thanks, Roger.

[PATCH 2/4] x86/P2M: un-indent write_p2m_entry()

[Jan Beulich]

Drop the inner scope that was left from earlier if/else removal. Take
the opportunity and make the paging_unlock() invocation common to
success and error paths, though.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -110,49 +110,43 @@ static int write_p2m_entry(struct p2m_do
unsigned int level)
 {
 struct domain *d = p2m->domain;
-
+unsigned int oflags;
+mfn_t omfn;
+int rc;
+
+paging_lock(d);
+
+if ( p2m->write_p2m_entry_pre && gfn != gfn_x(INVALID_GFN) )
+p2m->write_p2m_entry_pre(d, gfn, *p, new, level);
+
+oflags = l1e_get_flags(*p);
+omfn = l1e_get_mfn(*p);
+
+rc = p2m_entry_modify(p2m, p2m_flags_to_type(l1e_get_flags(new)),
+  p2m_flags_to_type(oflags), l1e_get_mfn(new),
+  omfn, level);
+if ( !rc )
 {
-unsigned int oflags;
-mfn_t omfn;
-int rc;
-
-paging_lock(d);
-
-if ( p2m->write_p2m_entry_pre && gfn != gfn_x(INVALID_GFN) )
-p2m->write_p2m_entry_pre(d, gfn, *p, new, level);
-
-oflags = l1e_get_flags(*p);
-omfn = l1e_get_mfn(*p);
-
-rc = p2m_entry_modify(p2m, p2m_flags_to_type(l1e_get_flags(new)),
-  p2m_flags_to_type(oflags), l1e_get_mfn(new),
-  omfn, level);
-if ( rc )
-{
-paging_unlock(d);
-return rc;
-}
-
 safe_write_pte(p, new);
 
 if ( p2m->write_p2m_entry_post )
 p2m->write_p2m_entry_post(p2m, oflags);
+}
 
-paging_unlock(d);
+paging_unlock(d);
 
-if ( nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) &&
- (oflags & _PAGE_PRESENT) &&
- !p2m_get_hostp2m(d)->defer_nested_flush &&
- /*
-  * We are replacing a valid entry so we need to flush nested p2ms,
-  * unless the only change is an increase in access rights.
-  */
- (!mfn_eq(omfn, l1e_get_mfn(new)) ||
-  !perms_strictly_increased(oflags, l1e_get_flags(new))) )
-p2m_flush_nestedp2m(d);
-}
+if ( !rc && nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) &&
+ (oflags & _PAGE_PRESENT) &&
+ !p2m_get_hostp2m(d)->defer_nested_flush &&
+ /*
+  * We are replacing a valid entry so we need to flush nested p2ms,
+  * unless the only change is an increase in access rights.
+  */
+ (!mfn_eq(omfn, l1e_get_mfn(new)) ||
+  !perms_strictly_increased(oflags, l1e_get_flags(new))) )
+p2m_flush_nestedp2m(d);
 
-return 0;
+return rc;
 }
 
 // Find the next level's P2M entry, checking for out-of-range gfn's...