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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Mon, Jul 31, 2023 at 05:27:22PM +0200, Jan Beulich wrote:
> 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.
Ack.

> 
> > @@ -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?
No good reason. I defaulted to the top of the function because C90 was
picky about declaring midway through a scope and I didn't consider going to
the top of the scope instead. Ack.

> 
> > --- 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.
It's not so much that I want it rather than I need it. hotadd_mem_check()
uses it and it's not part of the init process. That's the only non-init use
of it, and it hardly qualifies as "common". IMO, 
don't matter a whole

> Then again I wonder why original
> checking all got away without using this function ...
> 
> Jan
My guess is that most real systems don't exercise the corner cases, and so
the code was just silently broken passing the checks all the time. I could
not find an explanation for the OR check in hotadd_mem_check(), so I just
attributed it to being a bug introduced out of lack of knowledge of pdx.

Thanks,
Alejandro