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
