Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34991 )
Change subject: arch/x86: Include stack_top region into postcar_frame ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
@Aaron, do you think this is more specific to APL as you have implemented platform_segment_loaded() in apl/car.c ?
/* Bail out if loaded program segment does not lie in CAR region. */ if (!start_car_check && !end_car_check) return;
That's what apollolake/car.c currently has, maybe you have followup work to change it, but with pcf->stack in CBMEM this commit does not do anything?
This commit helps to mark a region where we have stored MTRR values in stack which was getting ignored previously. if you could try to dump value at pcf->stack, will help to understand. i have debugger connected which helped me to understand what we are keeping in this region and later not informing platform_segment_loaded(). So far only APL implemented the weak it might bother only APL but it might applicable for all.
Check this line https://github.com/coreboot/coreboot/blob/b1af16a4242d42feb0150c3a8c6fef41c7...
we are basically doing like mov esp, dword ptr [post_car_stack_top]; where post_car_stack_top = 0x99c243c0 [this we are signaling using platform_segment_loaded(stack_top_ptr) code below]
prog_segment_loaded((uintptr_t)stack_top_ptr, sizeof(uintptr_t), SEG_FINAL);
but after this instruction execute, what value will load into esp is 0x99c29fd8 = pcf->stack, we are not signaling this address using platform_segment_loaded()
purpose of this CL is to inform about the pcf->stack till its size where we have those postcar_frame been written
prog_segment_loaded(pcf->stack, pcf->stack_top - pcf->stack, SEG_FINAL);