On 28.08.2020 16:45, Igor Druzhinin wrote: > On 28/08/2020 08:51, Jan Beulich wrote: >> On 28.08.2020 02:13, Igor Druzhinin wrote: >>> Guest kernel does need to know in some cases where the tables are located >>> to treat these regions properly. One example is kexec process where >>> the first kernel needs to pass firmware region locations to the second >>> kernel which is now a requirement after 02a3e3cdb7f12 ("x86/boot: Parse SRAT >>> table and count immovable memory regions"). >>> >>> The memory that hvmloader allocates in the reserved region mostly contains >>> these useful tables and could be safely indicated as ACPI without the need >>> to designate a sub-region specially for that. Making it non-reclaimable >>> (ACPI NVS) would avoid potential reuse of this memory by the guest. >>> Swtiching from Reserved to ACPI NVS type for this memory would also mean >>> its content is preserved across S4 (which is true for any ACPI type >>> according >>> to the spec). >> >> Marking the range as ACPI is certainly fine, but why did you choose NVS? >> There's nothing non-volatile there afaict. And the part of the last >> sentence in parentheses suggests to me that the "normal" ACPI type is as >> suitable for the purpose you're after. > > The problem with normal ACPI type is that according to the spec it's > reclaimable > by the OS: > "This range is available RAM usable by the OS after it reads the ACPI tables." > while NVS type is specifically designated as non-reclaimable. The spec is also > denotes that both normal and NVS types should be preserved across S4 - that > sounds > ambiguous to me. But it might mean that non-NVS preservation is not OS > responsibility as it's assumed to be static.
I've taken a random system and found that all of its "real" ACPI tables, in particular the DSDT, live in "ACPI data", not "ACPI NVS". The DSDT includes AML, which I understand is needed at runtime. So "after it reads the ACPI tables" is even more ambiguous than just wrt S4 as you say. > Our region also contains VM86 TSS which we certainly need at runtime > (although that > could be allocated from the reserved region above if desirable). > > To stay on the safe side and do not rely on perceived sensible OS behavior > NVS type > was chosen which OS shouldn't touch under any circumstances. Can you at the very least emphasize this in the description? >>> --- a/tools/firmware/hvmloader/e820.c >>> +++ b/tools/firmware/hvmloader/e820.c >>> @@ -155,6 +155,8 @@ int build_e820_table(struct e820entry *e820, >>> { >>> unsigned int nr = 0, i, j; >>> uint32_t low_mem_end = hvm_info->low_mem_pgend << PAGE_SHIFT; >>> + uint32_t firmware_mem_end = >>> + RESERVED_MEMORY_DYNAMIC_START + (mem_mfns_allocated() << >>> PAGE_SHIFT); >> >> Here and elsewhere - please avoid uint32_t and friends when a plain >> C type (unsigned int or unsigned long in this case) will do. > > Ok, should I also take a chance and convert some of the surroundings? In general I'd recommend only adjusting code which gets touched anyway; in a few cases adjacent code better also gets adjusted so everything together looks reasonably consistent afterwards. But I didn't think this was necessary here. IOW - I'd suggest you don't, but in the end it's up to you (at the risk of needing to undo parts if you end up going too far). Jan