Re: [PATCH 0/6] Boot-time switching between 4- and 5-level paging for 4.15, Part 1

From: Ingo Molnar
Date: Thu Oct 26 2017 - 03:38:04 EST



* Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:

> On Tue, Oct 24, 2017 at 02:47:41PM +0200, Ingo Molnar wrote:
> > > > > > > > Making a variable that 'looks' like a constant macro dynamic in a rare Kconfig
> > > > > > > > scenario is asking for trouble.
> > > > > > >
> > > > > > > We expect boot-time page mode switching to be enabled in kernel of next
> > > > > > > generation enterprise distros. It shoudn't be that rare.
> > > > > >
> > > > > > My point remains even with not-so-rare Kconfig dependency.
> > > > >
> > > > > I don't follow how introducing new variable that depends on Kconfig option
> > > > > would help with the situation.
> > > >
> > > > A new, properly named variable or function (max_physmem_bits or
> > > > max_physmem_bits()) that is not all uppercase would make it abundantly clear that
> > > > it is not a constant but a runtime value.
> > >
> > > Would we need to rename every uppercase macros that would depend on
> > > max_physmem_bits()? Like MAXMEM.
> >
> > MAXMEM isn't used in too many places either - what's the total impact of it?
>
> The impact is not very small. The tree of macros dependent on
> MAX_PHYSMEM_BITS:
>
> MAX_PHYSMEM_BITS
> MAXMEM
> KEXEC_SOURCE_MEMORY_LIMIT
> KEXEC_DESTINATION_MEMORY_LIMIT
> KEXEC_CONTROL_MEMORY_LIMIT
> SECTIONS_SHIFT
> ZONEID_SHIFT
> ZONEID_PGSHIFT
> ZONEID_MASK
>
> The total number of users of them is not large. It's doable. But I expect
> it to be somewhat ugly, since we're partly in generic code and it would
> require some kind of compatibility layer for other archtectures.
>
> Do you want me to rename them all?

Yeah, I think these former constants should be organized better.

Here's their usage frequency:

triton:~/tip> for N in MAX_PHYSMEM_BITS MAXMEM KEXEC_SOURCE_MEMORY_LIMIT \
KEXEC_DESTINATION_MEMORY_LIMIT KEXEC_CONTROL_MEMORY_LIMIT SECTIONS_SHIFT \
ZONEID_SHIFT ZONEID_PGSHIFT ZONEID_MASK; do printf " %-40s: " $N; git grep -w $N | grep -vE 'define| \* ' | wc -l; done

MAX_PHYSMEM_BITS : 10
MAXMEM : 5
KEXEC_SOURCE_MEMORY_LIMIT : 2
KEXEC_DESTINATION_MEMORY_LIMIT : 2
KEXEC_CONTROL_MEMORY_LIMIT : 2
SECTIONS_SHIFT : 2
ZONEID_SHIFT : 1
ZONEID_PGSHIFT : 1
ZONEID_MASK : 1

So it's not too bad to clean up, I think.

How about something like this:

machine.physmem.max_bytes /* ex MAXMEM */
machine.physmem.max_bits /* bit count of the highest in-use physical address */
machine.physmem.zones.id_shift /* ZONEID_SHIFT */
machine.physmem.zones.pg_shift /* ZONEID_PGSHIFT */
machine.physmem.zones.id_mask /* ZONEID_MASK */

machine.kexec.physmem_bytes_src /* KEXEC_SOURCE_MEMORY_LIMIT */
machine.kexec.physmem_bytes_dst /* KEXEC_DESTINATION_MEMORY_LIMIT */

( With perhaps 'physmem' being an alias to '&machine->physmem', so that
physmem->max_bytes and physmem->max_bits would be a natural thing to write. )

I'd suggest doing this in a finegrained fashion, one step at a time, introducing
'struct machine' and 'struct physmem' and extending it gradually with new fields.

To re-discuss the virt_addr_valid() concern you raised:

> > For instance, virt_addr_valid() depends indirectly on it:
> >
> > virt_addr_valid()
> > __virt_addr_valid()
> > phys_addr_valid()
> > boot_cpu_data.x86_phys_bits (initialized with MAX_PHYSMEM_BITS)
> >
> > virt_addr_valid() is used in things like implementation /dev/kmem.
> >
> > To me it's far more risky than occasional build breakage for
> > CONFIG_X86_5LEVEL=y.
>
> So why do we have two variables here, one boot_cpu_data.x86_phys_bits and the
> other MAX_PHYSMEM_BITS - both set once during boot?

So it's still unclear to me why virt_addr_valid() would be a problem: this
function could probably (in a separate patch) use physmem->max_bits, which would
make it more secure than using even a dynamic MAX_PHYSMEM_BITS: it would detect
any physical addresses that are beyond the recognized maximum range.

I.e. all this would result in further improvements.

Thanks,

Ingo