Re: [PATCH v2 3/5] mm/pdx: Standardize region validation wrt pdx compression

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2023 09:59, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1159,6 +1159,9 @@ static int mem_hotadd_check(unsigned long spfn, 
> unsigned long epfn)
>  {
>      unsigned long s, e, length, sidx, eidx;
>  
> +    paddr_t mem_base = pfn_to_paddr(spfn);
> +    unsigned long mem_npages = epfn - spfn;
> +
>      if ( (spfn >= epfn) )
>          return 0;

While occasionally groups of declarations indeed want separating, the
rule of thumb is that the first blank line after declarations separates
them from statements. I don't see reason here to diverge from this.

> @@ -1660,6 +1663,8 @@ static bool __init cf_check rt_range_valid(unsigned 
> long smfn, unsigned long emf
>  
>  void __init efi_init_memory(void)
>  {
> +    paddr_t mem_base;
> +    unsigned long mem_npages;

Why in the outermost scope when ...

> @@ -1732,6 +1737,9 @@ void __init efi_init_memory(void)
>          smfn = PFN_DOWN(desc->PhysicalStart);
>          emfn = PFN_UP(desc->PhysicalStart + len);
>  
> +        mem_base = pfn_to_paddr(smfn);
> +        mem_npages = emfn - smfn;
> +
>          if ( desc->Attribute & EFI_MEMORY_WB )
>              prot |= _PAGE_WB;
>          else if ( desc->Attribute & EFI_MEMORY_WT )
> @@ -1759,8 +1767,7 @@ void __init efi_init_memory(void)
>              prot |= _PAGE_NX;
>  
>          if ( pfn_to_pdx(emfn - 1) < (DIRECTMAP_SIZE >> PAGE_SHIFT) &&
> -             !(smfn & pfn_hole_mask) &&
> -             !((smfn ^ (emfn - 1)) & ~pfn_pdx_bottom_mask) )
> +             pdx_is_region_compressible(mem_base, mem_npages))
>          {
>              if ( (unsigned long)mfn_to_virt(emfn - 1) >= HYPERVISOR_VIRT_END 
> )
>                  prot &= ~_PAGE_GLOBAL;

... you use the variables only in an inner one?

> --- a/xen/common/pdx.c
> +++ b/xen/common/pdx.c
> @@ -88,7 +88,7 @@ bool __mfn_valid(unsigned long mfn)
>  }
>  
>  /* Sets all bits from the most-significant 1-bit down to the LSB */
> -static uint64_t __init fill_mask(uint64_t mask)
> +static uint64_t fill_mask(uint64_t mask)
>  {
>      while (mask & (mask + 1))
>          mask |= mask + 1;

I see why you want __init dropped here, but the function wasn't written
for "common use" and hence may want improving first when intended for
more frequent (post-init) use as well. Then again I wonder why original
checking all got away without using this function ...

Jan