[XEN PATCH 0/2] x86/p2m: address a violation of MISRA C:2012 Rule 8.3

[XEN PATCH 0/2] x86/p2m: address a violation of MISRA C:2012 Rule 8.3

[Federico Serafini]

Patch 1/2 does some preparation work, hence it needs to be committed as first.
Patch 2/2 addresses the violation.

Federico Serafini (2):
  x86/p2m: preparation work for xenmem_add_to_physmap_one()
  x86/p2m: address a violation of MISRA C:2012 Rule 8.3

 xen/arch/x86/mm/p2m.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
2.34.1

[XEN PATCH 1/2] x86/p2m: preparation work for xenmem_add_to_physmap_one()

[Federico Serafini]

The objective is to use parameter name "gfn" for
xenmem_add_to_physmap_one().
Since the name "gfn" is currently used as identifier for a local
variable, bad things could happen if new uses of such variable are
committed while a renaming patch is waiting for the approval.
To avoid such danger, as first thing rename the local variable from
"gfn" to "gmfn".

No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/mm/p2m.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index fe9ccabb87..42508e1e7e 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2417,7 +2417,7 @@ int xenmem_add_to_physmap_one(
     gfn_t gpfn)
 {
     struct page_info *page = NULL;
-    unsigned long gfn = 0 /* gcc ... */, old_gpfn;
+    unsigned long gmfn = 0 /* gcc ... */, old_gpfn;
     mfn_t prev_mfn;
     int rc = 0;
     mfn_t mfn = INVALID_MFN;
@@ -2440,12 +2440,12 @@ int xenmem_add_to_physmap_one(
 
     case XENMAPSPACE_gmfn:
     {
-        gfn = idx;
-        mfn = get_gfn_unshare(d, gfn, &p2mt);
+        gmfn = idx;
+        mfn = get_gfn_unshare(d, gmfn, &p2mt);
         /* If the page is still shared, exit early */
         if ( p2m_is_shared(p2mt) )
         {
-            put_gfn(d, gfn);
+            put_gfn(d, gmfn);
             return -ENOMEM;
         }
         page = get_page_from_mfn(mfn, d);
@@ -2480,7 +2480,7 @@ int xenmem_add_to_physmap_one(
     /* XENMAPSPACE_gmfn: Check if the MFN is associated with another GFN. */
     old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
     ASSERT(!SHARED_M2P(old_gpfn));
-    if ( space == XENMAPSPACE_gmfn && old_gpfn != gfn )
+    if ( space == XENMAPSPACE_gmfn && old_gpfn != gmfn )
     {
         rc = -EXDEV;
         goto put_all;
@@ -2518,7 +2518,7 @@ int xenmem_add_to_physmap_one(
      */
     if ( space == XENMAPSPACE_gmfn )
     {
-        put_gfn(d, gfn);
+        put_gfn(d, gmfn);
         if ( !rc && extra.ppage )
         {
             *extra.ppage = page;
-- 
2.34.1

Re: [XEN PATCH 1/2] x86/p2m: preparation work for xenmem_add_to_physmap_one()

[Jan Beulich]

On 30.11.2023 16:48, Federico Serafini wrote:
> The objective is to use parameter name "gfn" for
> xenmem_add_to_physmap_one().
> Since the name "gfn" is currently used as identifier for a local
> variable, bad things could happen if new uses of such variable are
> committed while a renaming patch is waiting for the approval.
> To avoid such danger, as first thing rename the local variable from
> "gfn" to "gmfn".

"..., in line with XENMAPSPACE_gmfn which is the only case it is used
with."

This is to justify the name not matching our generally aimed at "gfn"
and "mfn" scheme.

> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

Jan

Re: [XEN PATCH 1/2] x86/p2m: preparation work for xenmem_add_to_physmap_one()

[Federico Serafini]

On 04/12/23 15:51, Jan Beulich wrote:
> On 30.11.2023 16:48, Federico Serafini wrote:
>> The objective is to use parameter name "gfn" for
>> xenmem_add_to_physmap_one().
>> Since the name "gfn" is currently used as identifier for a local
>> variable, bad things could happen if new uses of such variable are
>> committed while a renaming patch is waiting for the approval.
>> To avoid such danger, as first thing rename the local variable from
>> "gfn" to "gmfn".
> 
> "..., in line with XENMAPSPACE_gmfn which is the only case it is used
> with."
> 
> This is to justify the name not matching our generally aimed at "gfn"
> and "mfn" scheme.
> 
>> No functional change.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

There is an use of "gfn" also few lines outside of the
switch statement, within an if condition where also XENMAPSPACE_gmfn is
involved:
what is true is that "gfn" is used only when space == XENMAPSPACE_gmfn.

What do you think about improve the description by adding:
"..., in line with XENMAPSPACE_gmfn which is the only *space* it is used
with."

However, the description improvement can be done on commit?

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 1/2] x86/p2m: preparation work for xenmem_add_to_physmap_one()

[Jan Beulich]

On 04.12.2023 16:42, Federico Serafini wrote:
> On 04/12/23 15:51, Jan Beulich wrote:
>> On 30.11.2023 16:48, Federico Serafini wrote:
>>> The objective is to use parameter name "gfn" for
>>> xenmem_add_to_physmap_one().
>>> Since the name "gfn" is currently used as identifier for a local
>>> variable, bad things could happen if new uses of such variable are
>>> committed while a renaming patch is waiting for the approval.
>>> To avoid such danger, as first thing rename the local variable from
>>> "gfn" to "gmfn".
>>
>> "..., in line with XENMAPSPACE_gmfn which is the only case it is used
>> with."
>>
>> This is to justify the name not matching our generally aimed at "gfn"
>> and "mfn" scheme.
>>
>>> No functional change.
>>>
>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> There is an use of "gfn" also few lines outside of the
> switch statement, within an if condition where also XENMAPSPACE_gmfn is
> involved:
> what is true is that "gfn" is used only when space == XENMAPSPACE_gmfn.

Well, sure - me saying "case" wasn't meant to limit things to the switch()
statement.

> What do you think about improve the description by adding:
> "..., in line with XENMAPSPACE_gmfn which is the only *space* it is used
> with."

Fine with me.

> However, the description improvement can be done on commit?

It can. Nevertheless you want to avoid getting into the habit of asking
for things to be done while committing. Strictly speaking on-commit
editing isn't entirely correct, as committing ought to be a purely
mechanical operation. In how far a particular committer is willing to
deviate from that should be left to them.

Jan

[XEN PATCH 2/2] x86/p2m: address a violation of MISRA C:2012 Rule 8.3

[Federico Serafini]

Make function declaration and definition consistent changing
parameter name from "gpfn" to "gfn".
For consistency, rename also "old_gpfn" to "old_gfn".
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
This patch depends on patch 1/2 of the same series.
---
 xen/arch/x86/mm/p2m.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 42508e1e7e..6eb446e437 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2414,10 +2414,10 @@ int xenmem_add_to_physmap_one(
     unsigned int space,
     union add_to_physmap_extra extra,
     unsigned long idx,
-    gfn_t gpfn)
+    gfn_t gfn)
 {
     struct page_info *page = NULL;
-    unsigned long gmfn = 0 /* gcc ... */, old_gpfn;
+    unsigned long gmfn = 0 /* gcc ... */, old_gfn;
     mfn_t prev_mfn;
     int rc = 0;
     mfn_t mfn = INVALID_MFN;
@@ -2431,7 +2431,7 @@ int xenmem_add_to_physmap_one(
         break;
 
     case XENMAPSPACE_grant_table:
-        rc = gnttab_map_frame(d, idx, gpfn, &mfn);
+        rc = gnttab_map_frame(d, idx, gfn, &mfn);
         if ( rc )
             return rc;
         /* Need to take care of the reference obtained in gnttab_map_frame(). */
@@ -2455,7 +2455,7 @@ int xenmem_add_to_physmap_one(
     }
 
     case XENMAPSPACE_gmfn_foreign:
-        return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
+        return p2m_add_foreign(d, idx, gfn_x(gfn), extra.foreign_domid);
     }
 
     if ( mfn_eq(mfn, INVALID_MFN) )
@@ -2475,12 +2475,12 @@ int xenmem_add_to_physmap_one(
      * Upon freeing we wouldn't be able to find other mappings in the P2M
      * (unless we did a brute force search).
      */
-    prev_mfn = get_gfn(d, gfn_x(gpfn), &p2mt);
+    prev_mfn = get_gfn(d, gfn_x(gfn), &p2mt);
 
     /* XENMAPSPACE_gmfn: Check if the MFN is associated with another GFN. */
-    old_gpfn = get_gpfn_from_mfn(mfn_x(mfn));
-    ASSERT(!SHARED_M2P(old_gpfn));
-    if ( space == XENMAPSPACE_gmfn && old_gpfn != gmfn )
+    old_gfn = get_gpfn_from_mfn(mfn_x(mfn));
+    ASSERT(!SHARED_M2P(old_gfn));
+    if ( space == XENMAPSPACE_gmfn && old_gfn != gmfn )
     {
         rc = -EXDEV;
         goto put_all;
@@ -2493,22 +2493,22 @@ int xenmem_add_to_physmap_one(
     {
         if ( is_special_page(mfn_to_page(prev_mfn)) )
             /* Special pages are simply unhooked from this phys slot. */
-            rc = p2m_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K);
+            rc = p2m_remove_page(d, gfn, prev_mfn, PAGE_ORDER_4K);
         else if ( !mfn_eq(mfn, prev_mfn) )
             /* Normal domain memory is freed, to avoid leaking memory. */
-            rc = guest_remove_page(d, gfn_x(gpfn));
+            rc = guest_remove_page(d, gfn_x(gfn));
     }
 
     /* Unmap from old location, if any. */
-    if ( !rc && old_gpfn != INVALID_M2P_ENTRY && !gfn_eq(_gfn(old_gpfn), gpfn) )
-        rc = p2m_remove_page(d, _gfn(old_gpfn), mfn, PAGE_ORDER_4K);
+    if ( !rc && old_gfn != INVALID_M2P_ENTRY && !gfn_eq(_gfn(old_gfn), gfn) )
+        rc = p2m_remove_page(d, _gfn(old_gfn), mfn, PAGE_ORDER_4K);
 
     /* Map at new location. */
     if ( !rc )
-        rc = p2m_add_page(d, gpfn, mfn, PAGE_ORDER_4K, p2m_ram_rw);
+        rc = p2m_add_page(d, gfn, mfn, PAGE_ORDER_4K, p2m_ram_rw);
 
  put_all:
-    put_gfn(d, gfn_x(gpfn));
+    put_gfn(d, gfn_x(gfn));
 
  put_both:
     /*
-- 
2.34.1

Re: [XEN PATCH 2/2] x86/p2m: address a violation of MISRA C:2012 Rule 8.3

[Jan Beulich]

On 30.11.2023 16:48, Federico Serafini wrote:
> Make function declaration and definition consistent changing
> parameter name from "gpfn" to "gfn".
> For consistency, rename also "old_gpfn" to "old_gfn".
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

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

> ---
> This patch depends on patch 1/2 of the same series.

There's no need to state this, btw. Within a series later patches depending
on earlier ones if the default. There instead it can help committers if it
is made clear when patches do not depend on one another (and hence can be
committed in a order different from the submission's).

Jan

Re: [XEN PATCH 2/2] x86/p2m: address a violation of MISRA C:2012 Rule 8.3

[Federico Serafini]

On 04/12/23 15:54, Jan Beulich wrote:
> On 30.11.2023 16:48, Federico Serafini wrote:
>> Make function declaration and definition consistent changing
>> parameter name from "gpfn" to "gfn".
>> For consistency, rename also "old_gpfn" to "old_gfn".
>> No functional change.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
>> ---
>> This patch depends on patch 1/2 of the same series.
> 
> There's no need to state this, btw. Within a series later patches depending
> on earlier ones if the default. There instead it can help committers if it
> is made clear when patches do not depend on one another (and hence can be
> committed in a order different from the submission's).

Thanks for the information.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 0/2] x86/p2m: address a violation of MISRA C:2012 Rule 8.3

[Federico Serafini]

On 30/11/23 16:48, Federico Serafini wrote:
> Patch 1/2 does some preparation work, hence it needs to be committed as first.
> Patch 2/2 addresses the violation.
> 
> Federico Serafini (2):
>    x86/p2m: preparation work for xenmem_add_to_physmap_one()
>    x86/p2m: address a violation of MISRA C:2012 Rule 8.3
> 
>   xen/arch/x86/mm/p2m.c | 36 ++++++++++++++++++------------------
>   1 file changed, 18 insertions(+), 18 deletions(-)

Adding maintainers in CC.

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)