Re: [PATCH v10 05/15] x86/cpu: Defer CR pinning enforcement until late_initcall()

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Tue, 2025-10-07 at 16:05 -0700, Sohil Mehta wrote:
> On 10/7/2025 10:23 AM, Edgecombe, Rick P wrote:
> 
> > 
> > Why is only set_virtual_address_map problematic? Some of the other ones get
> > called after boot by a bunch of modules by the looks of it.
> > 
> 
> AFAIU, efi_enter_virtual_mode()->set_virtual_address_map maps the
> runtime services from physical mode into virtual mode.
> 
> After that, all the other runtime services can get called using virtual
> addressing. I can find out more if you still have concerns.

Ah, looking into this more I see the:
ffffffef00000000 |  -68    GB | fffffffeffffffff |   64 GB | EFI region mapping
space

It looks like the services get mapped there in a special MM. The calls switch to
it, so should stay in high address space. But I also saw this snippet:
   /*
    * Certain firmware versions are way too sentimental and still believe
    * they are exclusive and unquestionable owners of the first physical page,
    * even though they explicitly mark it as EFI_CONVENTIONAL_MEMORY
    * (but then write-access it later during SetVirtualAddressMap()).
    *
    * Create a 1:1 mapping for this page, to avoid triple faults during early
    * boot with such firmware. We are free to hand this page to the BIOS,
    * as trim_bios_range() will reserve the first page and isolate it away
    * from memory allocators anyway.
    */
   if (kernel_map_pages_in_pgd(pgd, 0x0, 0x0, 1, pf)) {
   	pr_err("Failed to create 1:1 mapping for the first page!\n");
   	return 1;
   }

By leaving LASS enabled it seems the kernel will be constraining BIOSs to not do
strange things. Which seems reasonable.

> 
> > > @@ -476,8 +476,8 @@ void cr4_init(void)
> > >  
> > >  	if (boot_cpu_has(X86_FEATURE_PCID))
> > >  		cr4 |= X86_CR4_PCIDE;
> > > -	if (static_branch_likely(&cr_pinning))
> > > -		cr4 = (cr4 & ~cr4_pinned_mask) | cr4_pinned_bits;
> > > +
> > > +	cr4 = (cr4 & ~cr4_pinned_mask) | cr4_pinned_bits;
> > 
> > 
> > Can you explain why this change is needed? It relies on cr4_pinned_bits to be
> > already set, and kind of is "enforcement", but no longer depends on
> > enable_cr_pinning() being called.
> > 
> 
> cr4_init() is only called from APs during bring up. The pinned bits are
> saved on the BSP and then used to program the CR4 on the APs. It is
> independent of pinning *enforcement* which warns when these bits get
> modified.

Sorry, still not following. How is it independent of CR pinning enforcement if
the enforcement is still taking place in this function. And if we don't need to
enforce pinning, why drop the branch?

> 
> > 
> > >  
> > >  	__write_cr4(cr4);
> > >