Re: [PATCH] pdx: cast PAGE_SIZE value ahead of shifting

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 14, 2025 at 12:45:40PM +0200, Jan Beulich wrote:
> On 14.08.2025 12:28, Roger Pau Monné wrote:
> > On Thu, Aug 14, 2025 at 09:18:45AM +0200, Jan Beulich wrote:
> >> On 13.08.2025 14:55, Roger Pau Monne wrote:
> >>> --- a/xen/common/pdx.c
> >>> +++ b/xen/common/pdx.c
> >>> @@ -288,7 +288,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
> >>>  
> >>>      pfn_pdx_hole_shift  = hole_shift;
> >>>      pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
> >>> -    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
> >>> +    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
> >>
> >> Given
> >>
> >> #define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> >>
> >> this shouldn't be needed, except maybe for Arm32. There, however, ...
> >>
> >>>      pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
> >>
> >> ... this and the shift immediately ahead would also be a problem afaict,
> >> which makes me conclude this isn't what Coverity has looked at. I expect
> >> the problem is with the toolstack side definition of PAGE_SIZE, which imo
> >> would rather be addressed there. (And yes, I'm pretty averse to arbitrary
> >> casts like this being introduced.)
> > 
> > As I've realized while looking at this, wouldn't ma_va_bottom_mask
> > also better be of type paddr_t, since it's not operating on pfns, but
> > physical addresses.  I didn't adjust the type of ma_va_bottom_mask,
> > but I would be happy to do it if you agree.
> 
> No, as its name says it's also used on virtual addresses (really: offsets
> into the direct map). It hence would better not have any bits set outside
> of the range that VAs can cover.

It's confusing that it's sometimes used against a paddr_t or an
unsigned long type.  The logic itself already limits the shift so it's
below the width of unsigned long AFAICT.

> With that, imo the cast (if any) also
> should have been to unsigned long, not paddr_t. Yet as said, im the cast
> would better not be there in the first place. Just that meanwhile I've
> learned that this was committed already.

Sorry, I should have waited for your opinion.

I think you would prefer the patch below.  I can send this formally,
not sure whether you would prefer a formal revert of the previous
patch, plus the new fix applied, or doing the revert in the new patc
(like below) is fine.

Thanks, Roger.
---
diff --git a/tools/tests/pdx/harness.h b/tools/tests/pdx/harness.h
index 5bef7df650d2..a0fe33b4f1e0 100644
--- a/tools/tests/pdx/harness.h
+++ b/tools/tests/pdx/harness.h
@@ -33,7 +33,7 @@
 #define PAGE_SHIFT    12
 /* Some libcs define PAGE_SIZE in limits.h. */
 #undef  PAGE_SIZE
-#define PAGE_SIZE     (1 << PAGE_SHIFT)
+#define PAGE_SIZE     (1UL << PAGE_SHIFT)
 #define MAX_ORDER     18 /* 2 * PAGETABLE_ORDER (9) */
 
 #define PFN_DOWN(x)   ((x) >> PAGE_SHIFT)
diff --git a/xen/common/pdx.c b/xen/common/pdx.c
index 06536cc639f3..9e6b36086fbd 100644
--- a/xen/common/pdx.c
+++ b/xen/common/pdx.c
@@ -274,7 +274,7 @@ bool __init pfn_pdx_compression_setup(paddr_t base)
 
     pfn_pdx_hole_shift  = hole_shift;
     pfn_pdx_bottom_mask = (1UL << bottom_shift) - 1;
-    ma_va_bottom_mask   = ((paddr_t)PAGE_SIZE << bottom_shift) - 1;
+    ma_va_bottom_mask   = (PAGE_SIZE << bottom_shift) - 1;
     pfn_hole_mask       = ((1UL << hole_shift) - 1) << bottom_shift;
     pfn_top_mask        = ~(pfn_pdx_bottom_mask | pfn_hole_mask);
     ma_top_mask         = pfn_top_mask << PAGE_SHIFT;