[PATCH 0/5] Fix fallout from IVMD/RMRR unification checks

[PATCH 0/5] Fix fallout from IVMD/RMRR unification checks

[Roger Pau Monne]

Hello,

First patch is a fix for a silly mistake I introduced in
iommu_unity_region_ok().  The rest are additional chances requested in
that context.  Last patch adds __must_check to the gfn/mfn addition
handlers.

Thanks, Roger.

Roger Pau Monne (5):
  iommu/x86: fix IVMD/RMRR range checker loop increment
  iommu/x86: print RMRR/IVMD ranges using full addresses
  iommu/x86: use full addresses internally for the IVMD/RMRR range
    checks
  iommu/x86: print page type in IVMD/RMRR check in case of error
  mm: add the __must_check attribute to {gfn,mfn}_add()

 xen/drivers/passthrough/x86/iommu.c | 25 +++++++++++--------------
 xen/include/xen/mm-frame.h          |  4 ++--
 2 files changed, 13 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH 1/5] iommu/x86: fix IVMD/RMRR range checker loop increment

[Roger Pau Monne]

mfn_add() doesn't store the incremented value in the parameter, and instead
returns it to the caller.  As a result, the loop in iommu_unity_region_ok()
didn't make progress.  Fix it by storing the incremented value.

Fixes: e45801dea17b ('iommu/x86: introduce a generic IVMD/RMRR range validity helper')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/x86/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 1c8cf3271a09..a3fa0aef7c37 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -804,7 +804,7 @@ bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
            "%s: [%#" PRI_mfn " ,%#" PRI_mfn "] is not (entirely) in reserved memory\n",
            prefix, mfn_x(start), mfn_x(end));
 
-    for ( addr = start; mfn_x(addr) <= mfn_x(end); mfn_add(addr, 1) )
+    for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
     {
         unsigned int type = page_get_ram_type(addr);
 
-- 
2.43.0

[PATCH 2/5] iommu/x86: print RMRR/IVMD ranges using full addresses

[Roger Pau Monne]

It's easier to correlate with the physical memory map if the addresses are
fully printed, instead of using frame numbers.

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/x86/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index a3fa0aef7c37..304a2f5480c7 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -801,8 +801,8 @@ bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
         return true;
 
     printk(XENLOG_WARNING
-           "%s: [%#" PRI_mfn " ,%#" PRI_mfn "] is not (entirely) in reserved memory\n",
-           prefix, mfn_x(start), mfn_x(end));
+           "%s: [%#lx, %#lx] is not (entirely) in reserved memory\n",
+           prefix, mfn_to_maddr(start), mfn_to_maddr(end));
 
     for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
     {
-- 
2.43.0

[PATCH 3/5] iommu/x86: use full addresses internally for the IVMD/RMRR range checks

[Roger Pau Monne]

Adjust the code in the checker to use full addresses rather than frame numbers,
as it's only page_get_ram_type() that requires an mfn parameter.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/x86/iommu.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 304a2f5480c7..e713cf803e8a 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -794,28 +794,26 @@ __initcall(adjust_irq_affinities);
 
 bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
 {
-    mfn_t addr;
+    paddr_t s = mfn_to_maddr(start), e = mfn_to_maddr(end);
 
-    if ( e820_all_mapped(mfn_to_maddr(start), mfn_to_maddr(end) + PAGE_SIZE,
-                         E820_RESERVED) )
+    if ( e820_all_mapped(s, e + PAGE_SIZE, E820_RESERVED) )
         return true;
 
     printk(XENLOG_WARNING
            "%s: [%#lx, %#lx] is not (entirely) in reserved memory\n",
-           prefix, mfn_to_maddr(start), mfn_to_maddr(end));
+           prefix, s, e);
 
-    for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
+    for ( paddr_t addr = s; addr <= e; addr += PAGE_SIZE )
     {
-        unsigned int type = page_get_ram_type(addr);
+        unsigned int type = page_get_ram_type(maddr_to_mfn(addr));
 
         if ( type == RAM_TYPE_UNKNOWN )
         {
-            if ( e820_add_range(mfn_to_maddr(addr),
-                                mfn_to_maddr(addr) + PAGE_SIZE, E820_RESERVED) )
+            if ( e820_add_range(addr, addr + PAGE_SIZE, E820_RESERVED) )
                 continue;
             printk(XENLOG_ERR
-                   "%s: page at %#" PRI_mfn " couldn't be reserved\n",
-                   prefix, mfn_x(addr));
+                   "%s: page at %#lx couldn't be reserved\n",
+                   prefix, paddr_to_pfn(addr));
             return false;
         }
 
@@ -829,9 +827,8 @@ bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
                      RAM_TYPE_UNUSABLE) )
             continue;
 
-        printk(XENLOG_ERR
-               "%s: page at %#" PRI_mfn " can't be converted\n",
-               prefix, mfn_x(addr));
+        printk(XENLOG_ERR "%s: page at %#lx can't be converted\n",
+               prefix, paddr_to_pfn(addr));
         return false;
     }
 
-- 
2.43.0

[PATCH 4/5] iommu/x86: print page type in IVMD/RMRR check in case of error

[Roger Pau Monne]

Provide more information in case the page can't be converted, and print the
original type(s).

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/passthrough/x86/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index e713cf803e8a..217409c29644 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -827,8 +827,8 @@ bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
                      RAM_TYPE_UNUSABLE) )
             continue;
 
-        printk(XENLOG_ERR "%s: page at %#lx can't be converted\n",
-               prefix, paddr_to_pfn(addr));
+        printk(XENLOG_ERR "%s: page at %#lx can't be converted (type %#x)\n",
+               prefix, paddr_to_pfn(addr), type);
         return false;
     }
 
-- 
2.43.0

[PATCH 5/5] mm: add the __must_check attribute to {gfn,mfn}_add()

[Roger Pau Monne]

It's not obvious from the function itself whether the incremented value will be
stored in the parameter, or returned to the caller.  That has leads to bugs in
the past as callers assume the incremented value is stored in the parameter.

Add the __must_check attribute to the function to easily spot callers that
don't consume the returned value, which signals an error in the caller logic.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/include/xen/mm-frame.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h
index 922ae418807a..c25e836f255a 100644
--- a/xen/include/xen/mm-frame.h
+++ b/xen/include/xen/mm-frame.h
@@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn);
 #undef mfn_x
 #endif
 
-static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
+static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i)
 {
     return _mfn(mfn_x(mfn) + i);
 }
@@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn);
 #undef gfn_x
 #endif
 
-static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
+static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i)
 {
     return _gfn(gfn_x(gfn) + i);
 }
-- 
2.43.0

Re: [PATCH 1/5] iommu/x86: fix IVMD/RMRR range checker loop increment

[Jan Beulich]

On 14.02.2024 11:37, Roger Pau Monne wrote:
> mfn_add() doesn't store the incremented value in the parameter, and instead
> returns it to the caller.  As a result, the loop in iommu_unity_region_ok()
> didn't make progress.  Fix it by storing the incremented value.
> 
> Fixes: e45801dea17b ('iommu/x86: introduce a generic IVMD/RMRR range validity helper')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Should Andrew get a Reported-by here? And surely we want to list the
Coverity ID as well? (Happy to take of both while committing, so long
as you agree.)

Jan

Re: [PATCH 5/5] mm: add the __must_check attribute to {gfn,mfn}_add()

[Julien Grall]

Hi,

On 14/02/2024 10:37, Roger Pau Monne wrote:
> It's not obvious from the function itself whether the incremented value will be
> stored in the parameter, or returned to the caller.  That has leads to bugs in
> the past as callers assume the incremented value is stored in the parameter.
> 
> Add the __must_check attribute to the function to easily spot callers that
> don't consume the returned value, which signals an error in the caller logic.

I like the patch. We should add more __must_check in the code :). I 
think ECLAIR will also help us to spot any place where we don't use the 
returned value.

> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
>   xen/include/xen/mm-frame.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h
> index 922ae418807a..c25e836f255a 100644
> --- a/xen/include/xen/mm-frame.h
> +++ b/xen/include/xen/mm-frame.h
> @@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn);
>   #undef mfn_x
>   #endif
>   
> -static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
> +static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i)
>   {
>       return _mfn(mfn_x(mfn) + i);
>   }
> @@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn);
>   #undef gfn_x
>   #endif
>   
> -static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
> +static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i)
>   {
>       return _gfn(gfn_x(gfn) + i);
>   }

Cheers,

-- 
Julien Grall

Re: [PATCH 1/5] iommu/x86: fix IVMD/RMRR range checker loop increment

[Roger Pau Monné]

On Wed, Feb 14, 2024 at 12:51:36PM +0100, Jan Beulich wrote:
> On 14.02.2024 11:37, Roger Pau Monne wrote:
> > mfn_add() doesn't store the incremented value in the parameter, and instead
> > returns it to the caller.  As a result, the loop in iommu_unity_region_ok()
> > didn't make progress.  Fix it by storing the incremented value.
> > 
> > Fixes: e45801dea17b ('iommu/x86: introduce a generic IVMD/RMRR range validity helper')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Should Andrew get a Reported-by here? And surely we want to list the
> Coverity ID as well? (Happy to take of both while committing, so long
> as you agree.)

Oh, I didn't add those here, yes, sure, feel free to add.

Thanks, Roger.

Re: [PATCH 2/5] iommu/x86: print RMRR/IVMD ranges using full addresses

[Jan Beulich]

On 14.02.2024 11:37, Roger Pau Monne wrote:
> It's easier to correlate with the physical memory map if the addresses are
> fully printed, instead of using frame numbers.
> 
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

In principle
Reviewed-by: Jan Beulich <jbeulich@suse.com>
I'm not sure though that this fully matches Andrew's intentions:

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -801,8 +801,8 @@ bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
>          return true;
>  
>      printk(XENLOG_WARNING
> -           "%s: [%#" PRI_mfn " ,%#" PRI_mfn "] is not (entirely) in reserved memory\n",
> -           prefix, mfn_x(start), mfn_x(end));
> +           "%s: [%#lx, %#lx] is not (entirely) in reserved memory\n",
> +           prefix, mfn_to_maddr(start), mfn_to_maddr(end));

e820.c uses [%016Lx, %016Lx] instead, i.e. full 16-digit numbers. For
easiest cross matching it may be desirable to do the same here. Then
of course the 0x prefixes may also better disappear.

Jan

Re: [PATCH 3/5] iommu/x86: use full addresses internally for the IVMD/RMRR range checks

[Jan Beulich]

On 14.02.2024 11:37, Roger Pau Monne wrote:
> Adjust the code in the checker to use full addresses rather than frame numbers,
> as it's only page_get_ram_type() that requires an mfn parameter.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

In this very shape I'd like to leave this to Paul or Andrew: I'm not outright
opposed, but I think this either goes too far (most type-safety being lost
again), or not far enough (both callers convert addresses to MFNs, just for
them to be converted back here).

Jan

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -794,28 +794,26 @@ __initcall(adjust_irq_affinities);
>  
>  bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
>  {
> -    mfn_t addr;
> +    paddr_t s = mfn_to_maddr(start), e = mfn_to_maddr(end);
>  
> -    if ( e820_all_mapped(mfn_to_maddr(start), mfn_to_maddr(end) + PAGE_SIZE,
> -                         E820_RESERVED) )
> +    if ( e820_all_mapped(s, e + PAGE_SIZE, E820_RESERVED) )
>          return true;
>  
>      printk(XENLOG_WARNING
>             "%s: [%#lx, %#lx] is not (entirely) in reserved memory\n",
> -           prefix, mfn_to_maddr(start), mfn_to_maddr(end));
> +           prefix, s, e);
>  
> -    for ( addr = start; mfn_x(addr) <= mfn_x(end); addr = mfn_add(addr, 1) )
> +    for ( paddr_t addr = s; addr <= e; addr += PAGE_SIZE )
>      {
> -        unsigned int type = page_get_ram_type(addr);
> +        unsigned int type = page_get_ram_type(maddr_to_mfn(addr));
>  
>          if ( type == RAM_TYPE_UNKNOWN )
>          {
> -            if ( e820_add_range(mfn_to_maddr(addr),
> -                                mfn_to_maddr(addr) + PAGE_SIZE, E820_RESERVED) )
> +            if ( e820_add_range(addr, addr + PAGE_SIZE, E820_RESERVED) )
>                  continue;
>              printk(XENLOG_ERR
> -                   "%s: page at %#" PRI_mfn " couldn't be reserved\n",
> -                   prefix, mfn_x(addr));
> +                   "%s: page at %#lx couldn't be reserved\n",
> +                   prefix, paddr_to_pfn(addr));
>              return false;
>          }
>  
> @@ -829,9 +827,8 @@ bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
>                       RAM_TYPE_UNUSABLE) )
>              continue;
>  
> -        printk(XENLOG_ERR
> -               "%s: page at %#" PRI_mfn " can't be converted\n",
> -               prefix, mfn_x(addr));
> +        printk(XENLOG_ERR "%s: page at %#lx can't be converted\n",
> +               prefix, paddr_to_pfn(addr));
>          return false;
>      }
>  

Re: [PATCH 4/5] iommu/x86: print page type in IVMD/RMRR check in case of error

[Jan Beulich]

On 14.02.2024 11:37, Roger Pau Monne wrote:
> Provide more information in case the page can't be converted, and print the
> original type(s).
> 
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 5/5] mm: add the __must_check attribute to {gfn,mfn}_add()

[Jan Beulich]

On 14.02.2024 11:37, Roger Pau Monne wrote:
> It's not obvious from the function itself whether the incremented value will be

s/the function itself/just the function name/ ?

> stored in the parameter, or returned to the caller.  That has leads to bugs in
> the past as callers assume the incremented value is stored in the parameter.

... callers may assume ... ?

> --- a/xen/include/xen/mm-frame.h
> +++ b/xen/include/xen/mm-frame.h
> @@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn);
>  #undef mfn_x
>  #endif
>  
> -static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
> +static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i)
>  {
>      return _mfn(mfn_x(mfn) + i);
>  }
> @@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn);
>  #undef gfn_x
>  #endif
>  
> -static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
> +static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i)
>  {
>      return _gfn(gfn_x(gfn) + i);
>  }

What about dfn_add() (in xen/iommu.h)?

Jan

Re: [PATCH 5/5] mm: add the __must_check attribute to {gfn,mfn}_add()

[Roger Pau Monné]

On Wed, Feb 14, 2024 at 02:42:46PM +0100, Jan Beulich wrote:
> On 14.02.2024 11:37, Roger Pau Monne wrote:
> > It's not obvious from the function itself whether the incremented value will be
> 
> s/the function itself/just the function name/ ?
> 
> > stored in the parameter, or returned to the caller.  That has leads to bugs in
> > the past as callers assume the incremented value is stored in the parameter.
> 
> ... callers may assume ... ?
> 
> > --- a/xen/include/xen/mm-frame.h
> > +++ b/xen/include/xen/mm-frame.h
> > @@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn);
> >  #undef mfn_x
> >  #endif
> >  
> > -static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
> > +static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i)
> >  {
> >      return _mfn(mfn_x(mfn) + i);
> >  }
> > @@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn);
> >  #undef gfn_x
> >  #endif
> >  
> > -static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
> > +static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i)
> >  {
> >      return _gfn(gfn_x(gfn) + i);
> >  }
> 
> What about dfn_add() (in xen/iommu.h)?

Oh, didn't spot that one.

Thanks, Roger.

Re: [PATCH 2/5] iommu/x86: print RMRR/IVMD ranges using full addresses

[Roger Pau Monné]

On Wed, Feb 14, 2024 at 02:22:09PM +0100, Jan Beulich wrote:
> On 14.02.2024 11:37, Roger Pau Monne wrote:
> > It's easier to correlate with the physical memory map if the addresses are
> > fully printed, instead of using frame numbers.
> > 
> > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> In principle
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> I'm not sure though that this fully matches Andrew's intentions:
> 
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -801,8 +801,8 @@ bool __init iommu_unity_region_ok(const char *prefix, mfn_t start, mfn_t end)
> >          return true;
> >  
> >      printk(XENLOG_WARNING
> > -           "%s: [%#" PRI_mfn " ,%#" PRI_mfn "] is not (entirely) in reserved memory\n",
> > -           prefix, mfn_x(start), mfn_x(end));
> > +           "%s: [%#lx, %#lx] is not (entirely) in reserved memory\n",
> > +           prefix, mfn_to_maddr(start), mfn_to_maddr(end));
> 
> e820.c uses [%016Lx, %016Lx] instead, i.e. full 16-digit numbers. For
> easiest cross matching it may be desirable to do the same here. Then
> of course the 0x prefixes may also better disappear.

Yes, I also saw that format, but wasn't sure whether it was desirable
to use here, as for example I would expect RMRR/IVMD regions to be
below the 4GB boundary.  Also the leading zeros are useful to have a
uniform table when printing the memory map that contains more than
one entry, but here I expect printing will be limited to a very small
set of entries, or maybe just one (as we only print the misplaced
ones).

Thanks, Roger.

Re: [PATCH 3/5] iommu/x86: use full addresses internally for the IVMD/RMRR range checks

[Roger Pau Monné]

On Wed, Feb 14, 2024 at 02:29:35PM +0100, Jan Beulich wrote:
> On 14.02.2024 11:37, Roger Pau Monne wrote:
> > Adjust the code in the checker to use full addresses rather than frame numbers,
> > as it's only page_get_ram_type() that requires an mfn parameter.
> > 
> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> In this very shape I'd like to leave this to Paul or Andrew: I'm not outright
> opposed, but I think this either goes too far (most type-safety being lost
> again), or not far enough (both callers convert addresses to MFNs, just for
> them to be converted back here).

I don't have a strong opinion, given this would you like me to adjust
4/5 so it can applied without this patch?

Thanks, Roger.

[PATCH v2 5/5] mm: add the __must_check attribute to {gfn,mfn,dfn}_add()

[Roger Pau Monne]

It's not obvious from just the function name whether the incremented value will
be stored in the parameter, or returned to the caller.  That has leads to bugs
in the past as callers may assume the incremented value is stored in the
parameter.

Add the __must_check attribute to the function to easily spot callers that
don't consume the returned value, which signals an error in the caller logic.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
Changes since v1:
 - Also add attribute to dfn_add().
---
 xen/include/xen/iommu.h    | 2 +-
 xen/include/xen/mm-frame.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 7aa6a7720977..9621459c63ee 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -42,7 +42,7 @@ TYPE_SAFE(uint64_t, dfn);
 #undef dfn_x
 #endif
 
-static inline dfn_t dfn_add(dfn_t dfn, unsigned long i)
+static inline dfn_t __must_check dfn_add(dfn_t dfn, unsigned long i)
 {
     return _dfn(dfn_x(dfn) + i);
 }
diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h
index 922ae418807a..c25e836f255a 100644
--- a/xen/include/xen/mm-frame.h
+++ b/xen/include/xen/mm-frame.h
@@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn);
 #undef mfn_x
 #endif
 
-static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
+static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i)
 {
     return _mfn(mfn_x(mfn) + i);
 }
@@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn);
 #undef gfn_x
 #endif
 
-static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
+static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i)
 {
     return _gfn(gfn_x(gfn) + i);
 }
-- 
2.43.0

Re: [PATCH 3/5] iommu/x86: use full addresses internally for the IVMD/RMRR range checks

[Jan Beulich]

On 14.02.2024 15:05, Roger Pau Monné wrote:
> On Wed, Feb 14, 2024 at 02:29:35PM +0100, Jan Beulich wrote:
>> On 14.02.2024 11:37, Roger Pau Monne wrote:
>>> Adjust the code in the checker to use full addresses rather than frame numbers,
>>> as it's only page_get_ram_type() that requires an mfn parameter.
>>>
>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> In this very shape I'd like to leave this to Paul or Andrew: I'm not outright
>> opposed, but I think this either goes too far (most type-safety being lost
>> again), or not far enough (both callers convert addresses to MFNs, just for
>> them to be converted back here).
> 
> I don't have a strong opinion, given this would you like me to adjust
> 4/5 so it can applied without this patch?

That or we first wait what Paul/Andrew think.

Jan

Re: [PATCH v2 5/5] mm: add the __must_check attribute to {gfn,mfn,dfn}_add()

[Jan Beulich]

On 14.02.2024 15:11, Roger Pau Monne wrote:
> It's not obvious from just the function name whether the incremented value will
> be stored in the parameter, or returned to the caller.  That has leads to bugs
> in the past as callers may assume the incremented value is stored in the
> parameter.
> 
> Add the __must_check attribute to the function to easily spot callers that
> don't consume the returned value, which signals an error in the caller logic.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Acked-by: Julien Grall <jgrall@amazon.com>

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

Re: [PATCH 5/5] mm: add the __must_check attribute to {gfn,mfn}_add()

[George Dunlap]

On Wed, Feb 14, 2024 at 7:42 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> It's not obvious from the function itself whether the incremented value will be
> stored in the parameter, or returned to the caller.  That has leads to bugs in
> the past as callers assume the incremented value is stored in the parameter.
>
> Add the __must_check attribute to the function to easily spot callers that
> don't consume the returned value, which signals an error in the caller logic.
>
> No functional change intended.

Just thinking out loud here... I wonder if "mfn_plus()" would be less
likely to be misunderstood.  Catching this during compile is def
better than catching it w/ Coverity or Eclair, but even better to help
people not make the mistake in the first place.

 -George

Re: [PATCH 5/5] mm: add the __must_check attribute to {gfn,mfn}_add()

[Jan Beulich]

On 19.02.2024 06:07, George Dunlap wrote:
> On Wed, Feb 14, 2024 at 7:42 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>>
>> It's not obvious from the function itself whether the incremented value will be
>> stored in the parameter, or returned to the caller.  That has leads to bugs in
>> the past as callers assume the incremented value is stored in the parameter.
>>
>> Add the __must_check attribute to the function to easily spot callers that
>> don't consume the returned value, which signals an error in the caller logic.
>>
>> No functional change intended.
> 
> Just thinking out loud here... I wonder if "mfn_plus()" would be less
> likely to be misunderstood.  Catching this during compile is def
> better than catching it w/ Coverity or Eclair, but even better to help
> people not make the mistake in the first place.

To me while mfn_plus() would indeed be somewhat less likely to cause this
kind of confusion, I don't think that naming would entirely eliminate the
risk.

I was actually considering to add mfn_inc() for the common case of
incrementing by 1, yet I can't see an alternative name avoiding the issue
there.

Jan