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

plat: Migrate gic to drivers/ukintctlr #971

Closed
wants to merge 3 commits into from

Conversation

rares-miculescu
Copy link

@rares-miculescu rares-miculescu commented Jul 5, 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.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

GIC (Generic Interrupt Controller) is moved into drivers/ukintctlr/, and the API for the interrupt controller into llib/ukintctlr.

GitHub-Depends-On: #1023

@razvand razvand added the area/plat Unikraft Patform label Jul 10, 2023
Copy link
Contributor

@razvanvirtan razvanvirtan left a comment

Choose a reason for hiding this comment

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

Thanks for this, @rares-miculescu!
In order to remove the Draft state, please follow the steps in the PR prerequisites checklist and mark them as done. Also, see my comments regarding the commits in this PR, they also apply here.

drivers/gic/Makefile.uk Outdated Show resolved Hide resolved
@razvand razvand requested a review from StefanJum July 24, 2023 20:01
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thank you @rares-miculescu. Please sign-off you commits and add a / at the end of directory names (i.e. plat/drivers/).

@rares-miculescu rares-miculescu changed the title drivers: Move gic from plat/drivers to drivers plat: Migrate gic to drivers/ukintctlr Aug 10, 2023
Copy link
Member

@michpappas michpappas left a comment

Choose a reason for hiding this comment

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

@rares-miculescu besides the comments below, there are many files that need their headers to be updated:

  • drivers/ukrtc/pl031/pl031.c
  • plat/common/arm/generic_timer.c
  • plat/common/arm/lcpu.c
  • plat/common/arm/time.c
  • plat/common/arm/traps_arm64.c
  • plat/common/platform_bus.c
  • plat/drivers/virtio/virtio_mmio.c

In all these you need to update#include <gic/gic-v2.h> to #include <uk/intctlr/gic-v2.h> and so on.

Similarly in plat/kvm/arm/setup.c:

diff --git a/plat/kvm/arm/setup.c b/plat/kvm/arm/setup.c
index 452e35cf..bb60db6d 100644
--- a/plat/kvm/arm/setup.c
+++ b/plat/kvm/arm/setup.c
@@ -32,7 +32,7 @@
 #include <uk/rtc/pl031.h>
 #endif /* CONFIG_LIBUKRTC_PL031 */
 #include <uk/assert.h>
-#include <kvm/intctrl.h>
+#include <uk/intctlr.h>
 #include <arm/cpu.h>
 #include <arm/arm64/cpu.h>
 #include <arm/smccc.h>

In addition, please also add exportsyms.uk in both the driver and the library.

lib/ukintctlr/Config.uk Outdated Show resolved Hide resolved
lib/ukintctlr/Makefile.uk Show resolved Hide resolved
lib/ukintctlr/Makefile.uk Outdated Show resolved Hide resolved
lib/ukintctlr/arm/intctlr.c Outdated Show resolved Hide resolved
plat/kvm/irq.c Outdated Show resolved Hide resolved
drivers/ukintctlr/gic/include/uk/gic/gic-v3.h Outdated Show resolved Hide resolved
drivers/ukintctlr/gic/include/uk/gic/gic.h Outdated Show resolved Hide resolved
lib/ukintctlr/arm/intctlr.c Outdated Show resolved Hide resolved
plat/kvm/Config.uk Outdated Show resolved Hide resolved
plat/kvm/Makefile.uk Show resolved Hide resolved
@rares-miculescu rares-miculescu marked this pull request as ready for review August 24, 2023 14:07
@rares-miculescu rares-miculescu requested review from a team as code owners August 24, 2023 14:07
@unikraft-bot unikraft-bot added arch/arm arch/arm64 area/kconfig Part of the Unikraft KConfig option system area/lib Internal Unikraft Microlibrary area/makefile Part of the Unikraft Makefile build system lang/c Issues or PRs to do with C/C++ plat/common Common to all platforms plat/driver plat/driver/gic plat/driver/virtio labels Aug 24, 2023
@unikraft-bot unikraft-bot added the plat/kvm Unikraft for KVM label Aug 24, 2023
Copy link
Member

@michpappas michpappas left a comment

Choose a reason for hiding this comment

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

Hi @rares-miculescu just a few more changes, almost there!

Besides the comments below, please do:

  • Rebase to staging
  • Add exportsyms.uk to lib/ukintctlr. Except from the public functions, don't forget to add the gic global that is defined in arch/arm/intctlr.c
  • Enable libukintctlr for x86 in plat/kvm/Config.uk:
diff --git a/plat/kvm/Config.uk b/plat/kvm/Config.uk
index 2c497c62..ffa2ced8 100644
--- a/plat/kvm/Config.uk
+++ b/plat/kvm/Config.uk
@@ -10,6 +10,7 @@ menuconfig PLAT_KVM
        select ELF64_TO_32 if ARCH_X86_64
        select LIBUKRELOC if OPTIMIZE_PIE
        select LIBUKOFW if ARCH_ARM_64
+       select LIBUKINTCTLR if ARCH_X86_64
        help
                 Create a Unikraft image that runs as a KVM guest

For ARCH_ARM_64 we don't need to do that, as we select that library from drivers/ukintctlr/gic/Config.uk.

lib/ukintctlr/Makefile.uk Show resolved Hide resolved
lib/ukintctlr/Makefile.uk Outdated Show resolved Hide resolved
drivers/Makefile.uk Outdated Show resolved Hide resolved
drivers/ukintctlr/Config.uk Outdated Show resolved Hide resolved
drivers/ukintctlr/Makefile.uk Outdated Show resolved Hide resolved
drivers/ukintctlr/gic/Config.uk Outdated Show resolved Hide resolved
plat/common/arm/generic_timer.c Outdated Show resolved Hide resolved
Copy link
Member

@michpappas michpappas left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @rares-miculescu 👍🏼

The API still uses the old scheme (intctrl_ vs uk_intctlr) but this is out of scope until we finalize how we handle commits that affect multiple libraries. Hopefully that will happen in the next maintanier's call.

Reviewed-by: Michalis Pappas michalis@unikraft.io

drivers/ukintctlr/gic/Makefile.uk Outdated Show resolved Hide resolved
drivers/ukintctlr/gic/exportsyms.uk Outdated Show resolved Hide resolved
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good, thanks.
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Rares Miculescu added 3 commits August 26, 2023 16:32
Move the API from `plat/kvm/` into `lib/`.

Signed-off-by: Rares Miculescu <miculescur@gmail.com>
Migrate GIC to newly introduced drivers subsystem.

Signed-off-by: Rares Miculescu <miculescur@gmail.com>
Migrate GIC to newly introduced drivers subsystem.

Signed-off-by: Rares Miculescu <miculescur@gmail.com>
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
f186521 lib: Add ukintctlr as library
bfc3501 drivers: Move GIC to drivers/ukintctlr
5f3c068 plat: Move GIC from plat/drivers

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 27, 2023
Migrate GIC to newly introduced drivers subsystem.

Signed-off-by: Rares Miculescu <miculescur@gmail.com>
Reviewed-by: Michalis Pappas <michalis@unikraft.io>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #971
unikraft-bot pushed a commit that referenced this pull request Aug 27, 2023
Migrate GIC to newly introduced drivers subsystem.

Signed-off-by: Rares Miculescu <miculescur@gmail.com>
Reviewed-by: Michalis Pappas <michalis@unikraft.io>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #971
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 27, 2023
@rares-miculescu rares-miculescu deleted the move-gic branch September 12, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch/arm arch/arm64 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++ plat/common Common to all platforms plat/driver/gic plat/driver/virtio plat/driver plat/kvm Unikraft for KVM
Projects
Status: Done
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

7 participants