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

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

On Thu, Aug 14, 2025 at 03:15:26PM +0200, Jan Beulich wrote:
> On 14.08.2025 15:01, Roger Pau Monné wrote:
> > 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.
> 
> Well, the variable simply doesn't need to be wider than the narrowest type
> it's used with.
> 
> >> 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.
> 
> Yes.
> 
> >  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.
> 
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I don't see a strong need for an outright revert.

I've adjusted UL -> L as requested by Andrew, and added the following
commit message:

tests/pdx: define PAGE_SIZE as long

Otherwise Coverity complains about possibly shifting an integer more than
31 bits.

This also reverts the previous attempt to fix this Coverity reported
issue, commit 4dd323029094d93dbc8d174fe744fd7f54f0a7a4.

Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
Coverity ID: 1662707
Fixes: cb50e4033717 ('test/pdx: add PDX compression unit tests')
Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Let me know if you are OK with the adjustment and commit message.

Thanks, Roger.