Re: [PATCH 6/8] mm/pdx: Standardize region validation wrt pdx compression

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Fri, Jul 21, 2023 at 06:05:51PM +0100, Julien Grall wrote:
> Hi Alejandro,
> 
> On 17/07/2023 17:03, Alejandro Vallejo wrote:
> > +bool pdx_is_region_compressible(unsigned long smfn, unsigned long emfn)
> 
> For newer interface, I would rather prefer if we use start + size. It is
> easier to reason (you don't have to wonder whether 'emfn' is inclusive or
> not) and avoid issue in the case you are trying to handle a region right at
> the end of the address space as emfn would be 0 in the non-inclusive case
> (not much a concern for MFNs as the last one should be invalid, but it makes
> harder to reason).
I could agree on this, but every single caller is based on (smfn, emfn),
so it needlessly forces every caller to perform conversions where the
callee can do it just once. That said, I think your point makes sense and
it ought to be done. Probably as as part of a bigger refactor where
(smfn, emfn)-based functions are turned into (base, len) variants.

> 
> > +{
> > +    uint64_t base = smfn << PAGE_SHIFT;
> 
> On Arm32, physical address are up to 40-bit. So you want to cast smfn to
> uint64_t before shifting. That said, it would be best to use pfn_to_paddr()
> and possibly switch to paddr_t for the type.
Ack. I keep forgetting 32 bits is not gone on non-x86 ports :)

> 
> Note that I understand that the rest of the PDX code is using uint64_t. So I
> would be ok if you don't want to switch to paddr_t.
Not for this patch, but I will keep "Turn all u64 maddr variables in
pdx.[ch] into paddr_t" in the back of my head for a rainy day.

> 
> > +    uint64_t len = (emfn - smfn) << PAGE_SHIFT;
> 
> Same here.
Ack.

Thanks,
Alejandro