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;
