Re: [PATCH v15 5/6] x86/boot: Parse SRAT address from RSDP and store immovable memory

[Kairui Song]

On Thu, Jan 17, 2019 at 3:58 PM Chao Fan <fanc.fnst@xxxxxxxxxxxxxx> wrote:
>
> On Wed, Jan 16, 2019 at 03:28:52PM +0800, Kairui Song wrote:
> >On Mon, Jan 7, 2019 at 11:24 AM Chao Fan <fanc.fnst@xxxxxxxxxxxxxx> wrote:
> >>
> >> +
> >> +/* Determine RSDP, based on acpi_os_get_root_pointer(). */
> >> +static acpi_physical_address get_rsdp_addr(void)
> >> +{
> >> +       acpi_physical_address pa;
> >> +
> >> +       pa = get_acpi_rsdp();
> >> +
> >> +       if (!pa)
> >> +               pa = efi_get_rsdp_addr();
> >> +
> >> +       if (!pa)
> >> +               pa = bios_get_rsdp_addr();
> >> +
> >> +       return pa;
> >> +}
> >
> >acpi_rsdp might be provided by boot_params.acpi_rsdp_addr too,
> >it's introduced in ae7e1238e68f2a for Xen PVH guest and later move to
> >boot_params in e6e094e053af,
> >kexec could use it to pass RSDP to second kernel as well. Please check
> >it as well make sure it always works.
> >
>
> Hi Kairui,
>
> I saw the parsing code has been added to kernel, but I didn't see
> where to fill in the 'acpi_rsdp_addr'. If only you(KEXEC) use it,
> I can add "#ifdef CONFIG_KEXEC", but you said Xen will use it also,
> so I didn't add ifdef to control it. I was trying to do as below:
>
> static inline acpi_physical_address get_boot_params_rsdp(void)
> {
>         return boot_params->acpi_rsdp_addr;
> }
>
> static acpi_physical_address get_rsdp_addr(void)
> {
>         bool boot_params_rsdp_exist;
>         acpi_physical_address pa;
>
>         pa = get_acpi_rsdp();
>
>         if (!pa)
>                 pa = get_boot_params_rsdp();
>
>         if (!pa) {
>                 pa = efi_get_rsdp_addr();
>                 boot_params_rsdp_exist = false;
>         }
>         else
>                 boot_params_rsdp_exist = true;
>
>         if (!pa)
>                 pa = bios_get_rsdp_addr();
>
>         if (pa && !boot_params_rsdp_exist)
>                 boot_params.acpi_rsdp_addr = pa;
>
>         return pa;
> }
>
> At the same time, I notice kernel only parses it when
> "#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
> we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
> update the define of EARLY_SRAT_PARSE:
>
> config EARLY_SRAT_PARSE
>         bool "EARLY SRAT parsing"
>         def_bool y
>         depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI
>
> Boris, how do you think the update for the new acpi_rsdp_addr?
> If I misunderstand something, please let me know.
>
> Thanks,
> Chao Fan
>

Hi, thanks for considering kexec usage,

but I think "boot_params_rsdp_exist" is not necessary,
boot_params->acpi_rsdp_addr should be either NULL or a valid value if
I, later initialization code considers it a valid value if it's not
NULL.

For the usage for Xen I'm not sure either, the info comes from commit
message of ae7e1238e68f2a that's also where boot_params.acpi_rsdp_addr
is first introduced, lets cc Juergen as well.


--
Best Regards,
Kairui Song