[coreboot-gerrit] [XL] Change in coreboot[main]: mb/emualtion: Add SIMICS QSP support

[Leon Groß (Code Review)]

Attention is currently required from: Angel Pons, Arthur Heymans, Benjamin Doron, Christian Walter, Lean Sheng Tan, Patrick Rudolph.

Leon Groß has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77905?usp=email )

Change subject: mb/emualtion: Add SIMICS QSP support
......................................................................

Patch Set 9:

(10 comments)

File src/mainboard/emulation/simics-qsp/Kconfig:

https://review.coreboot.org/c/coreboot/+/77905/comment/18be090d_7898b211 :
PS4, Line 22: ironlake
> it is way more similar to ironlake using QPI.

Done

File src/mainboard/emulation/simics-qsp/Kconfig.name:

https://review.coreboot.org/c/coreboot/+/77905/comment/441d258c_6f5fc012 :
PS4, Line 1: BOARD_EMULATION_SIMICS_QSP_X86_BoardX58Ich10
> upper case only.

Done

File src/mainboard/emulation/simics-qsp/gpio.c:

https://review.coreboot.org/c/coreboot/+/77905/comment/80a66a12_94e4276e :
PS4, Line 87: mainboard_gpio_map
> Is there any reason to set up GPIOs on virtualized hardware?

No, I think we can remove this.

File src/mainboard/emulation/simics-qsp/memmap.c:

https://review.coreboot.org/c/coreboot/+/77905/comment/05be2997_39769010 :
PS4, Line 39: uint32_t make_pciexbar(void)
> unused?

Yes, we can remove this as well as the encoding and the defintion in the header.

https://review.coreboot.org/c/coreboot/+/77905/comment/4f1b1241_fa82585e :
PS4, Line 97: postcar_frame_add_mtrr(pcf, top_of_ram - 8*MiB, 8*MiB, MTRR_TYPE_WRBACK);
            : 	postcar_frame_add_mtrr(pcf, top_of_ram, 8*MiB, MTRR_TYPE_WRBACK);
> This is an emulator. […]

The simulator itself implements MTRR capabilities, why wouldn't we set them up properly? I think this is a better match for the simulated hardware capabilities.

File src/mainboard/emulation/simics-qsp/northbridge.c:

https://review.coreboot.org/c/coreboot/+/77905/comment/4e0486c4_eea15299 :
PS4, Line 34: uint64_t tomk = 0;
> why not initialize where it's used and use const.

Done

https://review.coreboot.org/c/coreboot/+/77905/comment/a7340688_c367cb35 :
PS4, Line 42: uint64_t high
> const.

Done

https://review.coreboot.org/c/coreboot/+/77905/comment/34242eff_8843f1f3 :
PS4, Line 124: uintptr_t ending_address_below_4g = (simics_get_memory_size() >> 10) - 1;
             : 	u64 ending_address_above_4g = simics_get_high_memory_size() + 0xFFFFFFFF - 1;;
> const

Done

https://review.coreboot.org/c/coreboot/+/77905/comment/c991754c_b8360e0b :
PS4, Line 178: cpu_pci_domain_set_resources,
> why not reuse pci_domain_set_resources?

Done

https://review.coreboot.org/c/coreboot/+/77905/comment/0ae110da_ac7b9e4d :
PS4, Line 207: static void cpu_bus_init(struct device *dev)
             : {
             : 	mp_cpu_bus_init(dev);
             : }
             :
> Why use a separate function and not set . […]

Done