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 <jbeul...@suse.com>
>
> 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 <jbeul...@suse.com>
Coverity ID: 1662707
Fixes: cb50e4033717 ('test/pdx: add PDX compression unit tests')
Signed-off-by: Roger Pau Monné <roger....@citrix.com>
Acked-by: Jan Beulich <jbeul...@suse.com>
Let me know if you are OK with the adjustment and commit message.
Thanks, Roger.
