Attention is currently required from: Nico Huber, Tarun Tuli, Martin L Roth, Angel Pons, Andrey Petrov.
Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69757 )
Change subject: [RFC] Pull ligfxinit in romstage ......................................................................
Patch Set 1:
(1 comment)
File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/69757/comment/4cf49224_fce183ec PS1, Line 122:
Hmmm, don't we have some arcane magic for C code to put zero-initialized variables in a special sect […]
My understanding is that global **initialized** variable are not permitted in cache-as-ram. I don't see why global variable as a whole would be forbidden (please correct me if I am wrong). I feel like this is because we do not perform any data loading operations to transfer from code (`.text`) to the cache region.
When I compile without the `car.ld` hack, I get the exhaustive list of `.data` variables. Using objdump I collected their initial value (note that I did not exhaustively checked all the `[...]_E` ones but only a sample).
| Address | Type | Name | Value | |----------+------+----------------------------------------------+-------------| | ffffff60 | d | hw__debug__register_write_delay_nanoseconds | 00 | | ffffff68 | d | hw__debug__start_of_line | 01 | | ffffff2c | d | hw__gfx__gma__registers__gtt_32__range_aXnnn | 00 20 00 00 | | ffffff28 | d | hw__gfx__gma__registers__gtt_64__range_aXnnn | 00 80 00 00 | | ffffff30 | d | hw__gfx__gma__registers__regs__range_aXnnn | 00 00 00 00 | | ffffff6e | D | gma_E | 00 ... | | ffffff70 | D | gma__gfx_init_E | 00 ... | | ffffff6a | D | hw__debug_E | 00 ... | | ffffff6c | D | hw__debug_sink_E | 00 ... | | ffffff02 | D | hw__gfx__dp_aux_ch_E | 00 ... | | ffffff04 | D | hw__gfx__dp_dual_mode_E | 00 ... | | ffffff06 | D | hw__gfx__dp_info_E | 00 ... | | ffffff08 | D | hw__gfx__dp_training_E | 00 ... | | ffffff0a | D | hw__gfx__edid_E | 00 ... | | ffffff0c | D | hw__gfx__framebuffer_filler_E | 00 ... | | ffffff3a | D | hw__gfx__gma_E | 00 ... | | ffffff3c | D | hw__gfx__gma__combo_phy_E | 00 ... | | ffffff00 | D | hw__gfx__gma__config_E | 00 ... | | ffffff0e | D | hw__gfx__gma__config_helpers_E | 00 ... | | ffffff10 | D | hw__gfx__gma__connector_info_E | 00 ... | | ffffff42 | D | hw__gfx__gma__connectors_E | 00 ... | | ffffff3e | D | hw__gfx__gma__connectors__combo_phy_E | 00 ... | | ffffff40 | D | hw__gfx__gma__connectors__tc_E | 00 ... | | ffffff12 | D | hw__gfx__gma__display_probing_E | 00 ... | | ffffff14 | D | hw__gfx__gma__dp_aux_request_E | 00 ... | | ffffff16 | D | hw__gfx__gma__i2c_E | 00 ... | | ffffff38 | D | hw__gfx__gma__initialized | 00 ... | | ffffff18 | D | hw__gfx__gma__panel_E | 00 ... | | ffffff1a | D | hw__gfx__gma__pch__fdi_E | 00 ... | | ffffff1c | D | hw__gfx__gma__pch__sideband_E | 00 ... | | ffffff1e | D | hw__gfx__gma__pch__transcoder_E | 00 ... | | ffffff20 | D | hw__gfx__gma__pch__vga_E | 00 ... | | ffffff22 | D | hw__gfx__gma__pcode_E | 00 ... | | ffffff24 | D | hw__gfx__gma__pipe_setup_E | 00 ... | | ffffff48 | D | hw__gfx__gma__plls_E | 00 ... | | ffffff44 | D | hw__gfx__gma__plls__combo_phy_E | 00 ... | | ffffff46 | D | hw__gfx__gma__plls__dkl_E | 00 ... | | ffffff4a | D | hw__gfx__gma__port_detect_E | 00 ... | | ffffff50 | D | hw__gfx__gma__power_and_clocks_E | 00 ... | | ffffff4c | D | hw__gfx__gma__power_and_clocks__tgl_E | 00 ... | | ffffff4e | D | hw__gfx__gma__power_and_clocks__xelpd_E | 00 ... | | ffffff34 | D | hw__gfx__gma__registers_E | 00 ... | | ffffff36 | D | hw__gfx__gma__transcoder_E | 00 ... | | ffffff52 | D | hw__mmio_range_E | 00 ... | | ffffff56 | D | hw__mmio_regs_E | 00 ... | | ffffff54 | D | hw__pci__dev_E | 00 ... | | ffffff58 | D | hw__pci__mmconf_E | 00 ... | | ffffff5a | D | hw__port_io_E | 00 ... | | ffffff5c | D | hw__time_E | 00 ... | | ffffff72 | D | hw__time__timer_E | 00 ... |
The symbols ending with `_E` are empty and initialized by GNAT generated code (cf. `___elabs` functions) which is called using the `$stage_adainit()` function. Therefor, I think it is safe to places these into the `_bss` region of the `.car.data` section. It would help if we could make the ADA compiler put directly these directly into the `.bss` region of the object file as it would simplify the changes in the linker script. Any suggestion ? I could not find any GNAT compiler option for this but I am clearly not an ADA compiler expert :-)
We are left with the following:
- `hw__debug__register_write_delay_nanoseconds` is zero so installing it in the `_bss` region of the `.car.data` section should be fine. - `hw__debug__start_of_line`: Reverting the logic of this boolean should make it work - The following three are the challenging ones:
- `hw__gfx__gma__registers__gtt_32__range_aXnnnx` - `hw__gfx__gma__registers__gtt_64__range_aXnnn` - `hw__gfx__gma__registers__regs__range_aXnnn`
They seem to be entirely generated by the compiler and I don't know how to control them. Surprisingly, putting them in the `_bss` region of the `.car.data` section which I expected to be filled with zeros at runtime is apparently not causing any problem while their initial value should not be zeros. Any suggestion ?
-
Jérémy Compostella (Code Review)