Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the ACPI code and change its location. #911

Closed
wants to merge 8 commits into from

Conversation

mogasergiu
Copy link
Member

@mogasergiu mogasergiu commented May 25, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.pl on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): x86_64
  • Platform(s): kvm
  • Application(s): N/A

Additional configuration

Description of changes

Re-write the ACPI structures definitions and, since ACPI is platform and architecture agnostic, move it to a location where it reflects that.

This PR depends on #772 #848 #908 #909 and #910, therefore it includes them.

NOTE

This is part of a larger set of Pull Requests. To make things easier and to ensure that things build as expected, it is recommended that testing is done based on #912, which includes everything. The splitting has been done to ease the review process.

@mogasergiu mogasergiu requested review from a team as code owners May 25, 2023 12:50
@unikraft-bot unikraft-bot added arch/arm arch/arm64 arch/x86_64 area/arch Unikraft Architecture area/include Part of include/uk area/kconfig Part of the Unikraft KConfig option system area/lib Internal Unikraft Microlibrary area/makefile Part of the Unikraft Makefile build system area/plat Unikraft Patform lang/c Issues or PRs to do with C/C++ lang/python Issues or PRs to do with Python lib/ukboot plat/common Common to all platforms plat/driver plat/driver/virtio plat/kvm Unikraft for KVM plat/xen Unikraft for Xen labels May 25, 2023
@razvand razvand assigned marcrittinghaus and unassigned nderjung May 31, 2023
@razvand razvand removed the request for review from a team May 31, 2023 04:41
mogasergiu added a commit to mogasergiu/unikraft that referenced this pull request Aug 10, 2023
mogasergiu added a commit to mogasergiu/unikraft that referenced this pull request Aug 10, 2023
@razvand razvand assigned razvand and unassigned marcrittinghaus Aug 11, 2023
Redefine ACPI structures so that they comply with the coding style
and fix Local SAPIC's wrong definition, as the UID string has at
least 1 byte.

Furthermore, redefine `MADTTypeEntry` as a general System Descriptor
sub-Table, since it is used by almost all of them.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Since all structures calculate their checksum in the same manner,
use only one general checksum verification function for all of them.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Since `MADT` is a System Descriptor Table itself, move it to `sdt.h`
and keep in `madt.h` only the `MADT` specific definitions.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Write the code in such a manner that the same set of functions can
accommodate any version of ACPI. Thus, remove `acpi10_*` functions
and define an ACPI table generic fetching function `acpi_get_table`.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Allow quick access to the `UEFI` System Tables through a field in
the `bootinfo` structure.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
First check for the presence of `RSDP` in the `UEFI` System Table's
Configuration Tables. If it does not exist, fallback on the legacy
BIOS ROM Area.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Add the definition for `FADT` and its related structures. Implement
a getter for it and add its signature in the list of `ACPI` fetched
tables.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Since `ACPI` is a platform and architecture agnostic specification,
move it to a place where reality reflects that.

Furthermore, add a corresponding configuration option and dependency
on it for x86 SMP builds.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
@unikraft-bot
Copy link
Member

⚠️ Checkpatch passed with warnings

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered warnings:

SHA commit checkpatch
3eb565a plat/common/x86: Re-define ACPI structures
fb7e0b5 plat/common/x86/acpi.c: Generalize checksum verification
de63164 plat/common/include/x86/acpi: Move `MADT` definition to sdt.h
c1e0d1a plat/common/acpi: Generalize `ACPI` initialization code
31d9268 plat/common/bootinfo: Add `UEFI` System Tables address in `bootinfo`
3b72ca1 plat/common/acpi: Allow getting `RSDP` through `UEFI` System Table ⚠️
dbb6cce plat/common/x86: Add the `FADT` among the fetched tables
e925cbc plat: Move everything `ACPI` to a platform common location

Truncated logs starting from first warning 3b72ca1:

WARNING:LONG_LINE: line over 80 characters
#46: FILE: plat/common/x86/acpi.c:154:
+	ct_count = ((struct uk_efi_sys_tbl *)bi->efi_st)->number_of_table_entries;

total: 0 errors, 1 warnings, 82 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0006-plat-common-acpi-Allow-getting-RSDP-through-UEFI-Sys.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

@mschlumpp
Copy link
Member

I currently can't compile this without also enabling PIE:

In file included from unikraft/lib/ukdebug/include/uk/assert.h:40,
                 from unikraft/plat/common/x86/lcpu.c:38:
unikraft/plat/common/x86/lcpu.c: In function ‘lcpu_arch_mp_init’:
unikraft/plat/common/x86/lcpu.c:216:19: error: ‘x86_start16_addr’ undeclared (first use in this function)
  216 |         UK_ASSERT(x86_start16_addr < 0x100000);
      |                   ^~~~~~~~~~~~~~~~
unikraft/include/uk/arch/lcpu.h:53:43: note: in definition of macro ‘unlikely’
   53 | #define unlikely(x) (__builtin_expect((!!(x)), 0))
      |                                           ^
unikraft/plat/common/x86/lcpu.c:216:9: note: in expansion of macro ‘UK_ASSERT’
  216 |         UK_ASSERT(x86_start16_addr < 0x100000);
      |         ^~~~~~~~~
unikraft/plat/common/x86/lcpu.c:216:19: note: each undeclared identifier is reported only once for each function it appears in
  216 |         UK_ASSERT(x86_start16_addr < 0x100000);
      |                   ^~~~~~~~~~~~~~~~
unikraft/include/uk/arch/lcpu.h:53:43: note: in definition of macro ‘unlikely’
   53 | #define unlikely(x) (__builtin_expect((!!(x)), 0))
      |                                           ^
unikraft/plat/common/x86/lcpu.c:216:9: note: in expansion of macro ‘UK_ASSERT’
  216 |         UK_ASSERT(x86_start16_addr < 0x100000);
      |         ^~~~~~~~~
unikraft/plat/common/x86/lcpu.c:217:43: error: ‘x86_start16_begin’ undeclared (first use in this function)
  217 |         memcpy((void *)x86_start16_addr, &x86_start16_begin, X86_START16_SIZE);
      |                                           ^~~~~~~~~~~~~~~~~
unikraft/plat/common/x86/lcpu.c:217:62: error: ‘X86_START16_SIZE’ undeclared (first use in this function)
  217 |         memcpy((void *)x86_start16_addr, &x86_start16_begin, X86_START16_SIZE);
      |                                                              ^~~~~~~~~~~~~~~~
unikraft/plat/common/x86/lcpu.c: In function ‘lcpu_arch_post_start’:
unikraft/plat/common/x86/lcpu.c:255:40: error: ‘x86_start16_addr’ undeclared (first use in this function)
  255 |                         apic_send_sipi(x86_start16_addr, lcpu->id);
      |                                        ^~~~~~~~~~~~~~~~

The defconfig:

CONFIG_PLAT_KVM=y
CONFIG_UKPLAT_MEMRNAME=y
CONFIG_UKPLAT_LCPU_MAXCOUNT=2
CONFIG_PAGING=y
CONFIG_LIBISRLIB=y
CONFIG_LIBSYSCALL_SHIM=y
CONFIG_LIBUKBOOT_BANNER_POWEREDBY_U8ANSI2=y
CONFIG_LIBUKCPIO=y
CONFIG_LIBUKDEBUG_PRINTK_INFO=y
CONFIG_LIBUKDEBUG_PRINTD=y
CONFIG_LIBUKDEBUG_PRINT_THREAD=y
CONFIG_LIBUKDEBUG_ANSI_COLOR=y
CONFIG_LIBUKDEBUG_TRACEPOINTS=y
CONFIG_LIBUKDEBUG_ALL_TRACEPOINTS=y
# CONFIG_LIBUKLIBPARAM is not set
CONFIG_LIBUKNETDEV=y
CONFIG_LIBUKNOFAULT=y
CONFIG_LIBVFSCORE=y
CONFIG_LIBVFSCORE_AUTOMOUNT_ROOTFS=y
CONFIG_OPTIMIZE_NONE=y

Copy link
Member

@mschlumpp mschlumpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Marco Schlumpp marco@unikraft.io

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Since all structures calculate their checksum in the same manner,
use only one general checksum verification function for all of them.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Reviewed-by: Dragos Petre <dragos.petre27@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #911
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Since `MADT` is a System Descriptor Table itself, move it to `sdt.h`
and keep in `madt.h` only the `MADT` specific definitions.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Reviewed-by: Dragos Petre <dragos.petre27@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #911
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Write the code in such a manner that the same set of functions can
accommodate any version of ACPI. Thus, remove `acpi10_*` functions
and define an ACPI table generic fetching function `acpi_get_table`.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Reviewed-by: Dragos Petre <dragos.petre27@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #911
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Allow quick access to the `UEFI` System Tables through a field in
the `bootinfo` structure.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Reviewed-by: Dragos Petre <dragos.petre27@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #911
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
First check for the presence of `RSDP` in the `UEFI` System Table's
Configuration Tables. If it does not exist, fallback on the legacy
BIOS ROM Area.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Reviewed-by: Dragos Petre <dragos.petre27@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #911
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Add the definition for `FADT` and its related structures. Implement
a getter for it and add its signature in the list of `ACPI` fetched
tables.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Reviewed-by: Dragos Petre <dragos.petre27@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #911
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Since `ACPI` is a platform and architecture agnostic specification,
move it to a place where reality reflects that.

Furthermore, add a corresponding configuration option and dependency
on it for x86 SMP builds.

Signed-off-by: Sergiu Moga <sergiu.moga@protonmail.com>
Reviewed-by: Dragos Petre <dragos.petre27@gmail.com>
Reviewed-by: Marco Schlumpp <marco@unikraft.io>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #911
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch/arm arch/arm64 arch/x86_64 area/arch Unikraft Architecture area/include Part of include/uk area/kconfig Part of the Unikraft KConfig option system area/lib Internal Unikraft Microlibrary area/makefile Part of the Unikraft Makefile build system area/plat Unikraft Patform ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lang/python Issues or PRs to do with Python lib/ukboot plat/common Common to all platforms plat/driver/virtio plat/driver plat/kvm Unikraft for KVM plat/xen Unikraft for Xen size/large Large PR, takes some time to review
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

7 participants