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