Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34995 )
Change subject: arch/x86: Cache the TSEG region at the top of ram ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
I have analysis the boot average time, here is my observation
- Average boot time with POSTCAR_STAGE=y is ~815ms (with 1100:finished vboot kernel verification." value ~147ms)
All references by commit hash also please. We will find the source from that.
- Average boot time with POSTCAR_STAGE=n is ~811ms (with 1100:finished vboot kernel verification." value ~137ms)
So its consistent that end of end, we will save ~4ms for sure.
Try to explain the case of 1100 being slower with #1. 4ms of 800ms is 0.5% saving?
It sounds like the code caching related behavior, the more code cache range we enable in cb code the more time it takes in "1100:finished vboot kernel verification". With additional MTRRs with multiple of 16MB caching range i could see this time is actually increasing. I'm just hoping we are not exhausting the max cache capacity for SKU/core.
Something like that, yes. But my expectation was that since MTRRs are reprogrammed during MP INIT near timestamp 30, and we have called wbinvd and toggled CD bits in CR0(?), availability of cachelines for step 1100 should have remained the same. Unless there is some bug with non-evict mode being disabled and none of the modified cachelines for MTRR WB regions get evicted like we assume. Type WC would not fill cachelines, did you experiment with it for TSEG?
Now the benefit of stage_cache_add() is to minimize S3 resume time. On these devices where we are running POC are with S0ix default enable hence we are not even running S3 to utilizes this stage_cache benefits, IMHO, we should add a check based on s0ix_enable chip.h config option to add stage_cache_add entries and don't add into stage_cache if S0ix is enable by mainboard.
I would prefer to make HAVE_ACPI_RESUME a user selectable option in menuconfig. Don't select it and stage cache will remain disabled. We can have devicetree config to override S3 support off.
Don't select HAVE_ACPI_RESUME would impact S3 resume failure isn't it ? we might still like to continue to single CB image supports s0ix and S3 but we might agree to drop the S3 resume savings in cost of increase in normal boot time stage_cache_add.
Yes, you could disable stage cache based on devicetree config.