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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan