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

[Jan Beulich <jbeulich@suse.com>]

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. 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.

Jan