Re: [PATCH v2 5/5] pdx: Add CONFIG_HAS_PDX_COMPRESSION as a common Kconfig option

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2023 09:59, Alejandro Vallejo wrote:
> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -31,11 +31,15 @@ unsigned long __read_mostly pdx_group_valid[BITS_TO_LONGS(
>  
>  bool __mfn_valid(unsigned long mfn)
>  {
> -    if ( unlikely(evaluate_nospec(mfn >= max_page)) )
> +    bool invalid = mfn >= max_page;
> +#ifdef CONFIG_PDX_COMPRESSION
> +    invalid |= mfn & pfn_hole_mask;
> +#endif

Nit: Declaration(s) and statement(s) separated by a blank line please.

> @@ -49,6 +53,8 @@ void set_pdx_range(unsigned long smfn, unsigned long emfn)
>          __set_bit(idx, pdx_group_valid);
>  }
>  
> +#ifdef CONFIG_PDX_COMPRESSION
> +
>  /*
>   * Diagram to make sense of the following variables. The masks and shifts
>   * are done on mfn values in order to convert to/from pdx:

Nit: With a blank line after #ifdef, 

> @@ -175,6 +181,7 @@ void __init pfn_pdx_hole_setup(unsigned long mask)
>      pfn_top_mask        = ~(pfn_pdx_bottom_mask | pfn_hole_mask);
>      ma_top_mask         = pfn_top_mask << PAGE_SHIFT;
>  }
> +#endif /* CONFIG_PDX_COMPRESSION */
>  
>  
>  /*

... we would typically also have one before #endif. In the case here
you could even leverage that there are already (wrongly) two consecutive
blank lines.

> @@ -100,6 +98,8 @@ bool __mfn_valid(unsigned long mfn);
>  #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
>  #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
>  
> +#ifdef CONFIG_PDX_COMPRESSION
> +
>  extern unsigned long pfn_pdx_bottom_mask, ma_va_bottom_mask;
>  extern unsigned int pfn_pdx_hole_shift;
>  extern unsigned long pfn_hole_mask;
> @@ -205,8 +205,39 @@ static inline uint64_t directmapoff_to_maddr(unsigned 
> long offset)
>   *             position marks a potentially compressible bit.
>   */
>  void pfn_pdx_hole_setup(unsigned long mask);
> +#else /* CONFIG_PDX_COMPRESSION */
> +
> +/* Without PDX compression we can skip some computations */

Same here for the #else then.

Jan