Re: [PATCH v2 0/8] pdx: introduce a new compression algorithm

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 02/07/2025 09:26, Roger Pau Monné wrote:
> On Wed, Jul 02, 2025 at 09:52:45AM +0200, Orzel, Michal wrote:
> > We have a few issues on Arm. First, we don't check whether direct map is big
> > enough provided max_pdx that we don't set at all. Second, we don't really use
> > PDX grouping (can be also used without compression). My patch (that Stefano
> > attached previously) fixes the second issue (Allejandro will take it over to
> > come up with common solution).
> You probably can handle those as different issues, as PDX grouping is
> completely disjoint from PDX compression.  It might be helpful if
> we could split the PDX grouping into a separate file from the PDX
> compression.
>
> One weirdness I've noticed with ARM is the addition of start offsets
> to the existing PDX compression, by using directmap_base_pdx,
> directmap_mfn_start, directmap_base_pdx &c.  I'm not sure whether this will
> interfere with the PDX compression, but it looks like a bodge.  This
> should be part of the generic PDX compression implementation, not an
> extra added on a per-arch basis.
They were introduced right at the beginning of the ARM port because we 
have quite a few platforms where the memory doesn't start at 0 and there 
was still a fairly large hole between two banks. IIRC until this series 
we would have been able to handle the hole but not the offset.
This is can be handled in common, then I would be happy with that.

> FWIW, PDX offset translation should already compress any gaps from 0
> to the first RAM range, and hence this won't be needed (in fact it
> would just make ARM translations slower by doing an extra unneeded
> operation).  My recommendation would be to move this initial offset
> compression inside the PDX mask translation.
>
> > For the first issue, we need to know max_page (at
> > the moment we calculate it in setup_mm() at the very end but we could do it in
> > init_pdx() to know it ahead of setting direct map) and PDX offset (on x86 there
> > is no offset). I also think that on Arm we should just panic if direct map is
> > too small.
> Hm, that's up to the ARM folks, but my opinion is that you should
> simply ignore memory above the threshold.  Panicking should IMO be a
> last resort option when there's no way to workaround the issue.
This is following the other pattern within the Arm port. We want to fail 
early with a clear error rather than booting an half broken system.
Cheers,

--
Julien Grall