Re: [PATCH v8 1/3] x86/boot: Add acpitb.c to parse acpi tables
On Thu, Oct 11, 2018 at 12:57:08PM +0200, Borislav Petkov wrote:
>On Wed, Oct 10, 2018 at 04:41:17PM +0800, Chao Fan wrote:
[...]
>> +#ifdef CONFIG_KEXEC
>> +static bool get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> +char *args = (char *)get_cmd_line_ptr();
>> +size_t len = strlen((char *)args);
>> +char *tmp_cmdline, *param, *val;
>> +unsigned long long addr = 0;
>> +char *endptr;
>> +
>> +if (!strstr(args, "acpi_rsdp="))
>> +return false;
>> +
>> +tmp_cmdline = malloc(len+1);
>> +if (!tmp_cmdline)
>> +error("Failed to allocate space for tmp_cmdline");
>
>Why do you even need to allocate a tmp cmdline?
>
>Ah, I see what you've done - you've copied handle_mem_options() in
>kaslr.c. Well no, not really.
>
>That functionality needs to get extracted into a separate facility. Oh
>look, there's arch/x86/boot/compressed/cmdline.c which is begging to get
>extended.
>
>:-)
>
Hi Boris,
Sorry for disturbing you again, I want to make sure this detail with you.
You mean that I need splite this as a function and put it to
cmdline.c, right?
If my understand is wrong, please let me know.
Thanks,
Chao Fan
Re: [PATCH v8 1/3] x86/boot: Add acpitb.c to parse acpi tables
On Mon, Oct 15, 2018 at 04:26:09PM -0400, Masayoshi Mizuma wrote:
>Hi Chao,
>
>Let me add some suggestions.
Thanks for your review and suggestion.
I will change them in next version.
Thanks,
Chao Fan
>
>On Wed, Oct 10, 2018 at 04:41:17PM +0800, Chao Fan wrote:
>> There is a bug that kaslr may randomly chooses some positions
>> which are located in movable memory regions. This will break memory
>> hotplug feature and make the movable memory chosen by KASLR can't be
>> removed. So dig SRAT table from ACPI tables to get memory information.
>>
>> Imitate the ACPI code of parsing ACPI tables to dig and read ACPI
>> tables. Since some operations are not needed here, functions are
>> simplified. Functions will be used to dig only SRAT tables to get
>> information of memory, so that KASLR can the memory in immovable node.
>>
>> And also, these functions won't influence the initialization of
>> ACPI after start_kernel().
>>
>> Since use physical address directely, so acpi_os_map_memory()
>> and acpi_os_unmap_memory() are not needed.
>>
>> Signed-off-by: Chao Fan
>> ---
>> arch/x86/boot/compressed/Makefile | 2 +
>> arch/x86/boot/compressed/acpitb.c | 405 ++
>> arch/x86/boot/compressed/misc.h | 8 +
>> 3 files changed, 415 insertions(+)
>> create mode 100644 arch/x86/boot/compressed/acpitb.c
>>
>...cut...
>> +static struct acpi_table_header *get_acpi_srat_table(void)
>> +{
>> +char *args = (char *)get_cmd_line_ptr();
>> +acpi_physical_address acpi_table;
>> +acpi_physical_address root_table;
>> +struct acpi_table_header *header;
>> +struct acpi_table_rsdp *rsdp;
>> +char *signature;
>> +u8 *entry;
>> +u32 count;
>> +u32 size;
>> +int i, j;
>> +u32 len;
>> +
>> +rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
>> +if (!rsdp)
>> +return NULL;
>> +
>> +/* Get rsdt or xsdt from rsdp. */
>> +if (!strstr(args, "acpi=rsdt") &&
>> +rsdp->xsdt_physical_address && rsdp->revision > 1) {
>> +root_table = rsdp->xsdt_physical_address;
>> +size = ACPI_XSDT_ENTRY_SIZE;
>> +} else {
>> +root_table = rsdp->rsdt_physical_address;
>> +size = ACPI_RSDT_ENTRY_SIZE;
>> +}
>> +
>> +/* Get ACPI root table from rsdt or xsdt.*/
>> +header = (struct acpi_table_header *)root_table;
>> +len = header->length;
>> +count = (u32)((len - sizeof(struct acpi_table_header)) / size);
>> +entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
>> +
>> +for (i = 0; i < count; i++) {
>> +u64 address64;
>> +
>> +if (size == ACPI_RSDT_ENTRY_SIZE)
>> +acpi_table = ((acpi_physical_address)
>> + (*ACPI_CAST_PTR(u32, entry)));
>> +else {
>> +*(u64 *)(void *) = *(u64 *)(void *)entry;
>> +acpi_table = (acpi_physical_address) address64;
>> +}
>> +
>> +if (acpi_table) {
>> +header = (struct acpi_table_header *)acpi_table;
>
>> +signature = header->signature;
>> +
>> +if (!strncmp(signature, "SRAT", 4))
>
> if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
>
>> +return header;
>> +}
>> +entry += size;
>> +}
>> +return NULL;
>> +}
>> +
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/*
>> + * According to ACPI table, filter the immvoable memory regions
>> + * and store them in immovable_mem[].
>> + */
>> +void get_immovable_mem(void)
>> +{
>> +char *args = (char *)get_cmd_line_ptr();
>> +struct acpi_table_header *table_header;
>> +struct acpi_subtable_header *table;
>> +struct acpi_srat_mem_affinity *ma;
>> +unsigned long table_end;
>> +int i = 0;
>> +
>> +if (!strstr(args, "movable_node") || strstr(args, "acpi=off"))
>> +return;
>> +
>> +table_header = get_acpi_srat_table();
>> +if (!table_header)
>> +return;
>> +
>> +table_end = (unsigned long)table_header + table_header->length;
>> +
>> +table = (struct acpi_subtable_header *)
>> +((unsigned long)table_header + sizeof(struct acpi_table_srat));
>> +
>
>> +while (((unsigned long)table) + table->length < table_end) {
>
> while (((unsigned long)table) +
> sizeof(struct acpi_subtable_header) < table_end) {
>
>> +if (table->type == 1) {
>
> if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
>
>> +ma = (struct acpi_srat_mem_affinity *)table;
>> +if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
>> +immovable_mem[i].start = ma->base_address;
>> +immovable_mem[i].size = ma->length;
>> +i++;
>> +}
>> +
>> +if (i >=
Re: [PATCH v8 1/3] x86/boot: Add acpitb.c to parse acpi tables
Hi Chao,
Let me add some suggestions.
On Wed, Oct 10, 2018 at 04:41:17PM +0800, Chao Fan wrote:
> There is a bug that kaslr may randomly chooses some positions
> which are located in movable memory regions. This will break memory
> hotplug feature and make the movable memory chosen by KASLR can't be
> removed. So dig SRAT table from ACPI tables to get memory information.
>
> Imitate the ACPI code of parsing ACPI tables to dig and read ACPI
> tables. Since some operations are not needed here, functions are
> simplified. Functions will be used to dig only SRAT tables to get
> information of memory, so that KASLR can the memory in immovable node.
>
> And also, these functions won't influence the initialization of
> ACPI after start_kernel().
>
> Since use physical address directely, so acpi_os_map_memory()
> and acpi_os_unmap_memory() are not needed.
>
> Signed-off-by: Chao Fan
> ---
> arch/x86/boot/compressed/Makefile | 2 +
> arch/x86/boot/compressed/acpitb.c | 405 ++
> arch/x86/boot/compressed/misc.h | 8 +
> 3 files changed, 415 insertions(+)
> create mode 100644 arch/x86/boot/compressed/acpitb.c
>
...cut...
> +static struct acpi_table_header *get_acpi_srat_table(void)
> +{
> + char *args = (char *)get_cmd_line_ptr();
> + acpi_physical_address acpi_table;
> + acpi_physical_address root_table;
> + struct acpi_table_header *header;
> + struct acpi_table_rsdp *rsdp;
> + char *signature;
> + u8 *entry;
> + u32 count;
> + u32 size;
> + int i, j;
> + u32 len;
> +
> + rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
> + if (!rsdp)
> + return NULL;
> +
> + /* Get rsdt or xsdt from rsdp. */
> + if (!strstr(args, "acpi=rsdt") &&
> + rsdp->xsdt_physical_address && rsdp->revision > 1) {
> + root_table = rsdp->xsdt_physical_address;
> + size = ACPI_XSDT_ENTRY_SIZE;
> + } else {
> + root_table = rsdp->rsdt_physical_address;
> + size = ACPI_RSDT_ENTRY_SIZE;
> + }
> +
> + /* Get ACPI root table from rsdt or xsdt.*/
> + header = (struct acpi_table_header *)root_table;
> + len = header->length;
> + count = (u32)((len - sizeof(struct acpi_table_header)) / size);
> + entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
> +
> + for (i = 0; i < count; i++) {
> + u64 address64;
> +
> + if (size == ACPI_RSDT_ENTRY_SIZE)
> + acpi_table = ((acpi_physical_address)
> + (*ACPI_CAST_PTR(u32, entry)));
> + else {
> + *(u64 *)(void *) = *(u64 *)(void *)entry;
> + acpi_table = (acpi_physical_address) address64;
> + }
> +
> + if (acpi_table) {
> + header = (struct acpi_table_header *)acpi_table;
> + signature = header->signature;
> +
> + if (!strncmp(signature, "SRAT", 4))
if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
> + return header;
> + }
> + entry += size;
> + }
> + return NULL;
> +}
> +
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/*
> + * According to ACPI table, filter the immvoable memory regions
> + * and store them in immovable_mem[].
> + */
> +void get_immovable_mem(void)
> +{
> + char *args = (char *)get_cmd_line_ptr();
> + struct acpi_table_header *table_header;
> + struct acpi_subtable_header *table;
> + struct acpi_srat_mem_affinity *ma;
> + unsigned long table_end;
> + int i = 0;
> +
> + if (!strstr(args, "movable_node") || strstr(args, "acpi=off"))
> + return;
> +
> + table_header = get_acpi_srat_table();
> + if (!table_header)
> + return;
> +
> + table_end = (unsigned long)table_header + table_header->length;
> +
> + table = (struct acpi_subtable_header *)
> + ((unsigned long)table_header + sizeof(struct acpi_table_srat));
> +
> + while (((unsigned long)table) + table->length < table_end) {
while (((unsigned long)table) +
sizeof(struct acpi_subtable_header) < table_end) {
> + if (table->type == 1) {
if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
> + ma = (struct acpi_srat_mem_affinity *)table;
> + if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
> + immovable_mem[i].start = ma->base_address;
> + immovable_mem[i].size = ma->length;
> + i++;
> + }
> +
> + if (i >= MAX_NUMNODES*2)
> + break;
> + }
> + table = (struct acpi_subtable_header *)
> + ((unsigned long)table + table->length);
> + }
> + num_immovable_mem =
Re: [PATCH] efi/libstub: Disable some warnings for x86{,_64}
On Mon, Oct 15, 2018 at 12:46 PM Nathan Chancellor
wrote:
>
> On Mon, Oct 15, 2018 at 12:40:36PM -0700, Nick Desaulniers wrote:
> > On Fri, Oct 12, 2018 at 6:06 PM Nathan Chancellor
> > wrote:
> > >
> > > When building the kernel with Clang, some disabled warnings appear
> > > because this Makefile overrides KBUILD_CFLAGS for x86{,_64}. Add them to
> > > this list so that the build is clean again.
> > >
> > > -Wpointer-sign was disabled for the whole kernel before the beginning
> > > of git history.
> > >
> > > -Waddress-of-packed-member was disabled for the whole kernel in
> > > commit bfb38988c51e ("kbuild: clang: Disable 'address-of-packed-member'
> > > warning") and for x86/boot/compressed in commit 20c6c1890455 ("x86/boot:
> > > Disable the address-of-packed-member compiler warning").
> > >
> > > -Wgnu was disabled for the whole kernel in commit 61163efae020 ("kbuild:
> > > LLVMLinux: Add Kbuild support for building kernel with Clang") and for
> > > x86/boot/compressed in commit 6c3b56b19730 ("x86/boot: Disable Clang
> > > warnings about GNU extensions").
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/112
> > > Signed-off-by: Nathan Chancellor
> > > ---
> > >
> > > Nick expressed concern that this Makefile is overwriting KBUILD_CFLAGS
> > > and suggested potentially rewriting the x86 portion of this Makefile to
> > > behave like the arm/arm64 one where problematic flags are filtered out.
> > > While that comes to fruition, it would be nice for this folder to behave
> > > like the rest of the kernel when it comes to this warnings so that the
> > > build is cleaner, thus this patch.
> > >
> > > drivers/firmware/efi/libstub/Makefile | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/Makefile
> > > b/drivers/firmware/efi/libstub/Makefile
> > > index c51627660dbb..d9845099635e 100644
> > > --- a/drivers/firmware/efi/libstub/Makefile
> > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > @@ -9,7 +9,10 @@ cflags-$(CONFIG_X86_32):= -march=i386
> > > cflags-$(CONFIG_X86_64):= -mcmodel=small
> > > cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \
> > >-fPIC -fno-strict-aliasing
> > > -mno-red-zone \
> > > - -mno-mmx -mno-sse -fshort-wchar
> > > + -mno-mmx -mno-sse -fshort-wchar \
> > > + -Wno-pointer-sign \
> >
> > Hi Nathan, thanks for this patch.
> >
> > Should this be:
> > $(call cc-disable-warning, pointer-sign)
> > as well?
> >
>
> According to commit 1d6bf3a9a546 ("kbuild: add -Wno-pointer-sign flag
> unconditionally") in -next, all supported compilers recognize the flag
> so I don't think it's necessary.
No problem.
Tested-by: Nick Desaulniers
>
> > > + $(call cc-disable-warning,
> > > address-of-packed-member) \
> > > + $(call cc-disable-warning, gnu)
> > >
> > > # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
> > > # disable the stackleak plugin
> > > --
> > > 2.19.1
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
> Thanks for the review!
> Nathan
--
Thanks,
~Nick Desaulniers
Re: [PATCH] efi/libstub: Disable some warnings for x86{,_64}
On Mon, Oct 15, 2018 at 12:40:36PM -0700, Nick Desaulniers wrote:
> On Fri, Oct 12, 2018 at 6:06 PM Nathan Chancellor
> wrote:
> >
> > When building the kernel with Clang, some disabled warnings appear
> > because this Makefile overrides KBUILD_CFLAGS for x86{,_64}. Add them to
> > this list so that the build is clean again.
> >
> > -Wpointer-sign was disabled for the whole kernel before the beginning
> > of git history.
> >
> > -Waddress-of-packed-member was disabled for the whole kernel in
> > commit bfb38988c51e ("kbuild: clang: Disable 'address-of-packed-member'
> > warning") and for x86/boot/compressed in commit 20c6c1890455 ("x86/boot:
> > Disable the address-of-packed-member compiler warning").
> >
> > -Wgnu was disabled for the whole kernel in commit 61163efae020 ("kbuild:
> > LLVMLinux: Add Kbuild support for building kernel with Clang") and for
> > x86/boot/compressed in commit 6c3b56b19730 ("x86/boot: Disable Clang
> > warnings about GNU extensions").
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/112
> > Signed-off-by: Nathan Chancellor
> > ---
> >
> > Nick expressed concern that this Makefile is overwriting KBUILD_CFLAGS
> > and suggested potentially rewriting the x86 portion of this Makefile to
> > behave like the arm/arm64 one where problematic flags are filtered out.
> > While that comes to fruition, it would be nice for this folder to behave
> > like the rest of the kernel when it comes to this warnings so that the
> > build is cleaner, thus this patch.
> >
> > drivers/firmware/efi/libstub/Makefile | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile
> > b/drivers/firmware/efi/libstub/Makefile
> > index c51627660dbb..d9845099635e 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -9,7 +9,10 @@ cflags-$(CONFIG_X86_32):= -march=i386
> > cflags-$(CONFIG_X86_64):= -mcmodel=small
> > cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \
> >-fPIC -fno-strict-aliasing -mno-red-zone
> > \
> > - -mno-mmx -mno-sse -fshort-wchar
> > + -mno-mmx -mno-sse -fshort-wchar \
> > + -Wno-pointer-sign \
>
> Hi Nathan, thanks for this patch.
>
> Should this be:
> $(call cc-disable-warning, pointer-sign)
> as well?
>
According to commit 1d6bf3a9a546 ("kbuild: add -Wno-pointer-sign flag
unconditionally") in -next, all supported compilers recognize the flag
so I don't think it's necessary.
> > + $(call cc-disable-warning,
> > address-of-packed-member) \
> > + $(call cc-disable-warning, gnu)
> >
> > # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
> > # disable the stackleak plugin
> > --
> > 2.19.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
Thanks for the review!
Nathan
Re: [PATCH] efi/libstub: Disable some warnings for x86{,_64}
On Fri, Oct 12, 2018 at 6:06 PM Nathan Chancellor
wrote:
>
> When building the kernel with Clang, some disabled warnings appear
> because this Makefile overrides KBUILD_CFLAGS for x86{,_64}. Add them to
> this list so that the build is clean again.
>
> -Wpointer-sign was disabled for the whole kernel before the beginning
> of git history.
>
> -Waddress-of-packed-member was disabled for the whole kernel in
> commit bfb38988c51e ("kbuild: clang: Disable 'address-of-packed-member'
> warning") and for x86/boot/compressed in commit 20c6c1890455 ("x86/boot:
> Disable the address-of-packed-member compiler warning").
>
> -Wgnu was disabled for the whole kernel in commit 61163efae020 ("kbuild:
> LLVMLinux: Add Kbuild support for building kernel with Clang") and for
> x86/boot/compressed in commit 6c3b56b19730 ("x86/boot: Disable Clang
> warnings about GNU extensions").
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/112
> Signed-off-by: Nathan Chancellor
> ---
>
> Nick expressed concern that this Makefile is overwriting KBUILD_CFLAGS
> and suggested potentially rewriting the x86 portion of this Makefile to
> behave like the arm/arm64 one where problematic flags are filtered out.
> While that comes to fruition, it would be nice for this folder to behave
> like the rest of the kernel when it comes to this warnings so that the
> build is cleaner, thus this patch.
>
> drivers/firmware/efi/libstub/Makefile | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile
> b/drivers/firmware/efi/libstub/Makefile
> index c51627660dbb..d9845099635e 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -9,7 +9,10 @@ cflags-$(CONFIG_X86_32):= -march=i386
> cflags-$(CONFIG_X86_64):= -mcmodel=small
> cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \
>-fPIC -fno-strict-aliasing -mno-red-zone \
> - -mno-mmx -mno-sse -fshort-wchar
> + -mno-mmx -mno-sse -fshort-wchar \
> + -Wno-pointer-sign \
Hi Nathan, thanks for this patch.
Should this be:
$(call cc-disable-warning, pointer-sign)
as well?
> + $(call cc-disable-warning,
> address-of-packed-member) \
> + $(call cc-disable-warning, gnu)
>
> # arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
> # disable the stackleak plugin
> --
> 2.19.1
>
--
Thanks,
~Nick Desaulniers
