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

gicv2 support #1235

Merged
merged 1 commit into from Nov 1, 2019

Conversation

@bchalios
Copy link
Contributor

bchalios commented Aug 30, 2019

Reason for This PR

This PR is meant to address #1196 for supporting GICv2 devices on aarch64 architecture.

Description of Changes

This makes the GIC-related code more flexible and implements a GICv2 device. It introduces a GICDevice Trait which needs to be implement by GIC device implementations. We implement the Trait for GICv3 and GICv2 devices. When creating the GIC device for the microVM we initially try to create a v3 device and if that fails, we fall back to creating a v2 device.

I have tried this on a Hikey970 board and it works, so far. I 'm trying this out on a RPi4 as well to double check everything is working. In any case, I'm not greatly familiar with the GIC architecture (I did some research for this PR), so any comments on what I 've done so far is more that welcome.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria. Where there are two options, keep one.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • Either this PR is linked to an issue, or, the reason for this PR is
    clearly provided.
  • The description of changes is clear and encompassing.
  • Either no docs need to be updated as part of this PR, or, the required
    doc changes are included in this PR. Docs in scope are all *.md files
    located either in the repository root, or in the docs/ directory.
  • Either no code has been touched, or, code-level documentation for touched
    code is included in this PR.
  • Either no API changes are included in this PR, or, the API changes are
    reflected in firecracker/swagger.yaml.
  • Either the changes in this PR have no user impact, or, the changes in
    this PR have user impact and have been added to the CHANGELOG.md file.
@bchalios bchalios force-pushed the cloudkernels:gicv2 branch 5 times, most recently from b135d55 to cecaaf3 Aug 30, 2019
@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Sep 1, 2019

Ok, I did a re-write of the PR which tries to do what was discussed in the issue comments, i.e. the GIC-related code tries to setup a GIC-v3 device and if it doesn't succeed it fails back to create a GIC-v2 device.

Moreover, the GIC code returns the version of the GIC chip that was created and that info is used in order to create the correct FDT later on.

Moreover, I tried to update the relevant tests to take these changes into account.

Disclaimer: my rust knowledge is very limited, so comments are more than welcome :)

@bchalios bchalios marked this pull request as ready for review Sep 1, 2019
@dianpopa dianpopa self-requested a review Sep 2, 2019
@bchalios bchalios force-pushed the cloudkernels:gicv2 branch 2 times, most recently from be1f019 to 2f55520 Sep 15, 2019
@bchalios bchalios changed the title WIP: gicv2 support gicv2 support Sep 15, 2019
@luxas

This comment has been minimized.

Copy link

luxas commented Sep 19, 2019

Thanks @bchalios! Could we get a review of this PR from any of the maintainers please?
cc @dianpopa @andreeaflorescu
Thanks in advance 👏!

@dianpopa

This comment has been minimized.

Copy link
Contributor

dianpopa commented Sep 19, 2019

Thanks @bchalios! Could we get a review of this PR from any of the maintainers please?
cc @dianpopa @andreeaflorescu
Thanks in advance 👏!

Yes I will definitely review this. It has been a busy period in terms of PR reviews but I will prioritize this and come back to you asap.

Copy link
Contributor

serban300 left a comment

I didn't have time to take a detailed look on this, and I don't know this area of our code very well, but at first sight I would prefer a more extensible and object oriented design.

I think it would make sense to have a class for GicV2 that contains all the gicv2 specific logic and a different class for GicV3 that contains all the gicv3 specific logic. Also they can implement a common trait. This way it would be easier to add support for gicv4 in the future for example. Also the code would probably be more readable.

@bchalios bchalios force-pushed the cloudkernels:gicv2 branch 4 times, most recently from 4978947 to a3771ec Sep 27, 2019
@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Sep 29, 2019

I didn't have time to take a detailed look on this, and I don't know this area of our code very well, but at first sight I would prefer a more extensible and object oriented design.

I think it would make sense to have a class for GicV2 that contains all the gicv2 specific logic and a different class for GicV3 that contains all the gicv3 specific logic. Also they can implement a common trait. This way it would be easier to add support for gicv4 in the future for example. Also the code would probably be more readable.

Thanks for your review @serban300!

I 've tried to refactor the PR to implement the feature in the way you suggested. Please, take a look and let me know if the changes make sense to you.

Essentially, I am using this PR as a way to, as well, practice Rust (I 've never written anything before in Rust), so please let me know if something strikes badly to you.

@serban300

This comment has been minimized.

Copy link
Contributor

serban300 commented Oct 1, 2019

I didn't have time to take a detailed look on this, and I don't know this area of our code very well, but at first sight I would prefer a more extensible and object oriented design.
I think it would make sense to have a class for GicV2 that contains all the gicv2 specific logic and a different class for GicV3 that contains all the gicv3 specific logic. Also they can implement a common trait. This way it would be easier to add support for gicv4 in the future for example. Also the code would probably be more readable.

Thanks for your review @serban300!

I 've tried to refactor the PR to implement the feature in the way you suggested. Please, take a look and let me know if the changes make sense to you.

Essentially, I am using this PR as a way to, as well, practice Rust (I 've never written anything before in Rust), so please let me know if something strikes badly to you.

Thanks for addressing the comments. I will try to take a look on the code as soon as possible. Worst case scenario I will do this at the beginning of the next week. I am super busy right now.

Copy link
Contributor

acatangiu left a comment

@bchalios the PR looks good functionally, but we'd like some more code deduplication if you have time/availability.

arch/src/aarch64/gic.rs Outdated Show resolved Hide resolved
arch/src/aarch64/gicv2.rs Outdated Show resolved Hide resolved
@bchalios bchalios force-pushed the cloudkernels:gicv2 branch from a3771ec to fd27ae3 Oct 14, 2019
@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Oct 15, 2019

@acatangiu I pushed a commit that tries to apply the requested changes. The new functionality is implemented through static functions, as I could not figure out a clean way to incorporate it in the common Trait. If you see any alternative, let me know!

Cheers,
Babis

create_gic_v2_node(fdt, gic_device)
}

fn create_gic_v2_node(fdt: &mut Vec<u8>, gic_device: &Box<GICDevice>) -> Result<()> {

This comment has been minimized.

Copy link
@serban300

serban300 Oct 16, 2019

Contributor

create_gic_v2_node and create_gic_v3_node look very similar. Looks like the only differences are:

append_property_string(fdt, "compatible", "arm,gic-400")?;
vs
append_property_string(fdt, "compatible", "arm,gic-v3")?;
and
ARCH_GIC_V2_MAINT_IRQ vs ARCH_GIC_V3_MAINT_IRQ

So I think we can have have only one method here. It could receives 2 paramaters: compatible: string and maint_irq: u32. Or it could get this parameters from the GicDevice. Whichever you think makes more sense. I think getting them from GicDevice would be cleaner.

This comment has been minimized.

Copy link
@bchalios

bchalios Oct 18, 2019

Author Contributor

I guess that is fine for now, but what happens when GICv4 (or any newer version) support comes in which would introduce more differences in the tree?

This comment has been minimized.

Copy link
@serban300

serban300 Oct 18, 2019

Contributor

I don't know. Depends on the differences. We can probably make this more extensible but I don't know if it's worth it at this point.

@@ -355,15 +357,46 @@ fn create_chosen_node(fdt: &mut Vec<u8>, cmdline: &CStr) -> Result<()> {
Ok(())
}

fn create_gic_node(fdt: &mut Vec<u8>, vcpu_count: u64) -> Result<()> {
fn create_gic_node(fdt: &mut Vec<u8>, gic_device: &Box<GICDevice>) -> Result<()> {
if gic_device.version() == kvm_bindings::kvm_device_type_KVM_DEV_TYPE_ARM_VGIC_V3 {

This comment has been minimized.

Copy link
@serban300

serban300 Oct 16, 2019

Contributor

I thoink that using a match block here would be more readable

}

/// Initialize a GIC device
pub fn init_device(vm: &VmFd, gic_device_type: u32) -> Result<DeviceFd> {

This comment has been minimized.

Copy link
@serban300

serban300 Oct 16, 2019

Contributor

I would move this method inside the GICDevice trait and leverage the version() method which is already defined there:

// version() needs to be static
fn version() -> u32;

pub fn init_device(vm: &VmFd) -> Result<DeviceFd> {
    let mut gic_device = kvm_bindings::kvm_create_device {
        type_: Self::version(),
        fd: 0,
        flags: 0,
    };

    let vgic_fd = vm.create_device(&mut gic_device).map_err(Error::CreateGIC);

    vgic_fd
}

Please try to see if you could also leverage this pattern for the other methods defined here: set_device_attribute, finalize_device. This way maybe also the new() method could become smaller and simpler.

This comment has been minimized.

Copy link
@bchalios

bchalios Oct 18, 2019

Author Contributor

hmmm, how can init_device (and set_device_attribute, finalize_device, for that matter) be parts of the Trait object? They are used to create the object itself. How can we call the Trait method on an object that has not yet been constructed?

This comment has been minimized.

Copy link
@serban300

serban300 Oct 18, 2019

Contributor

If the method is static you can call it even if the object hasn't been instantiated yet. For example something like this would work:

pub trait GICDevice {
    fn version() -> u32;

    fn init_device() -> bool {
        Self::version() == 1
    }
}

pub struct GICv2 {
    initialized: bool
}

impl GICDevice for GICv2 {
    fn version() -> u32 {
        1
    }

}

impl GICv2 {
    fn new() -> GICv2 {
        GICv2 {
            initialized: Self::init_device()
        }
    }
}

This comment has been minimized.

Copy link
@bchalios

bchalios Oct 19, 2019

Author Contributor

oh, great! I didn't know that. Thanks a lot.

This comment has been minimized.

Copy link
@bchalios

bchalios Oct 21, 2019

Author Contributor

Hmmm, I tried this out, but when I declare static Trait methods the compiler complains that the trait cannot be made into an object (meaning by the Box<GICDevice> I'm returning from the new method.

I have been looking around and it seems that there is no nice way around this issue. Please let me know, if I'm msising something

This comment has been minimized.

Copy link
@bchalios

bchalios Oct 22, 2019

Author Contributor

Ok, I dealt with it by marking the static methods of the Trait with where Self: Sized, e.g.:

pub trait GICDevice {
    fn init_device() -> bool where Self: Sized {
        true
    }
}

essentially, saying that these will not be called on the object itself. Does that make sense to you?

This comment has been minimized.

Copy link
@serban300

serban300 Oct 22, 2019

Contributor

Nice. Sounds good. It probably makes sense, but I have to see the entire commit anyway.

const KVM_VGIC_V2_CPU_SIZE: u64 = 0x2000;

/// Get the address of the GICv2 distributor.
fn get_dist_addr() -> u64 {

This comment has been minimized.

Copy link
@serban300

serban300 Oct 16, 2019

Contributor

This can be declared in the GICDevice trait and defined separately for GICv2 and GICv3, just like suggested for init_device(). Also please check if you can do something similar for all the other methods defined here. I would prefer to see them either declare in the GICDevice trait, or if they don't belong there you can define them directly in the GICv2 struct. The same comment for all the methods defined in the gicv3.rs file outside of the GICv3 struct.

This comment has been minimized.

Copy link
@bchalios

bchalios Oct 22, 2019

Author Contributor

I sort of disagree on that. This is an implementation-specific attribute. It could be that another GIC implementation does not have a distributor address attribute, which would not allow it to implement the Trait

This comment has been minimized.

Copy link
@serban300

serban300 Oct 22, 2019

Contributor

Ok. I'm not very familiar with ARM and GIC. Feel free to move the methods where you think that it makes sense. If they are GIC generic, move them to the trait. If not move them inside GICv2 or GICv3 struct.

const KVM_VGIC_V2_DIST_SIZE: u64 = 0x1000;
const KVM_VGIC_V2_CPU_SIZE: u64 = 0x2000;
Comment on lines 14 to 15

This comment has been minimized.

Copy link
@serban300

serban300 Oct 16, 2019

Contributor

Please move this constants in the impl GICv2 { block since they are GicV2 related. Please try to define all the methods / constants where they belong.

@bchalios bchalios force-pushed the cloudkernels:gicv2 branch from fd27ae3 to 0e5fa62 Oct 30, 2019
@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Oct 30, 2019

Ok, I think that I've addressed the requested changes. Please, take a look and let me know if the changes make sense

Copy link
Contributor

serban300 left a comment

Looks much better, but I still have some comments. Please take a look.

/// Trait for GIC devices.
pub trait GICDevice {
/// Returns the GIC version of the device
fn version(&self) -> u32;

This comment has been minimized.

Copy link
@serban300

serban300 Oct 31, 2019

Contributor

It would make sense for this method to be static as well:

    fn version() -> u32
    where
        Self: Sized;

This comment has been minimized.

Copy link
@bchalios

bchalios Oct 31, 2019

Author Contributor

Right, I couldn't do that before because it was used from the fdt code, but now yup.

Self: Sized,
{
let mut gic_device = kvm_bindings::kvm_create_device {
type_: version,

This comment has been minimized.

Copy link
@serban300

serban300 Oct 31, 2019

Contributor

If yo transform version() into a static method, you can use Self::version() here. And yo ucan remove the version parameter.

pub fn new(vm: &VmFd) -> Result<Box<dyn GICDevice>> {
let vgic_fd =
Self::init_device(vm, kvm_bindings::kvm_device_type_KVM_DEV_TYPE_ARM_VGIC_V2)?;

/* Setting up the distributor attribute.
We are placing the GIC below 1GB so we need to substract the size of the distributor. */
Self::set_device_attribute(
&vgic_fd,
kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ADDR,
u64::from(kvm_bindings::KVM_VGIC_V2_ADDR_TYPE_DIST),
&GICv2::get_dist_addr() as *const u64 as u64,
0,
)?;

/* Setting up the CPU attribute. */
Self::set_device_attribute(
&vgic_fd,
kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ADDR,
u64::from(kvm_bindings::KVM_VGIC_V2_ADDR_TYPE_CPU),
&GICv2::get_cpu_addr() as *const u64 as u64,
0,
)?;

Self::finalize_device(&vgic_fd)?;

let vgic_device = GICv2 {
fd: vgic_fd,
properties: [
GICv2::get_dist_addr(),
GICv2::get_dist_size(),
GICv2::get_cpu_addr(),
GICv2::get_cpu_size(),
],
};

Ok(Box::new(vgic_device))
}
Comment on lines 50 to 86

This comment has been minimized.

Copy link
@serban300

serban300 Oct 31, 2019

Contributor

I think that this logic can be nicely abstracted and moved in the trait:

  • init_device() is already a trait method
  • for the logic related to setting the device attributes we can have a trait method
    fn init_device_attributes(fd: &DeviceFd, vcpu_count: u64) -> Result<()>
    where
        Self: Sized;

and implement it differently for GICv2 vs GICv3. v2 impl for example:


  • finalize_device is already defined in the trait
  • the part that actually creates the device can also be abstracted in a method that is declared in the trait and defined separately for GICv2 and GICv3:

trait :

    fn create_device(fd: DeviceFd, vcpu_count: u64) -> Box<dyn GICDevice>
    where
        Self: Sized;

v2 impl:

fn create_device(fd: DeviceFd, vcpu_count: u64) -> Box<GICDevice> {
    Box::new(GICv2 {
        fd: fd,
        properties: [
            GICv2::get_dist_addr(),
            GICv2::get_dist_size(),
            GICv2::get_cpu_addr(),
            GICv2::get_cpu_size(),
        ],
    })
}

And then we can have new defined in the trait:

    fn new(vm: &VmFd, vcpu_count: u64) -> Result<Box<dyn GICDevice>>
    where
        Self: Sized,
    {
        let vgic_fd = Self::init_device(vm)?;

        Self::init_device_attributes(&vgic_fd, vcpu_count);

        Self::finalize_device(&vgic_fd)?;

        Ok(Self::create_device(vgic_fd, vcpu_count))
    }

The only problem here is that we would have to pass all the creation params for any GIC implementation. For example GICv2 doesn't need the vcpu_count but we have to pass it anyway. So it's not ideal. Also there would be some coupling. Which for the moment seems ok but I don't know how it will affect future GIC devices support.

Do you think it would make sense to refactor it like this ? Considering extensibility and future GIC devices support.

fn set_device_attribute(
fd: &DeviceFd,
group: u32,
attr: u64,
addr: u64,
flags: u32,
) -> Result<()>
where
Self: Sized,
{
let attr = kvm_bindings::kvm_device_attr {
group: group,
attr: attr,
addr: addr,
flags: flags,
};
fd.set_device_attr(&attr)
.map_err(Error::SetDeviceAttribute)?;

Ok(())
}
Comment on lines +55 to +88

This comment has been minimized.

Copy link
@serban300

serban300 Oct 31, 2019

Contributor

This method seems to only forward the parameters to fd.set_device_attr. I don't know if it brings a lot of value.

@bchalios bchalios force-pushed the cloudkernels:gicv2 branch from 43886ef to 901f0aa Oct 31, 2019
@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Oct 31, 2019

Ok, could you take a look now? I don't think we can avoid the problem with multiple arguments per version, since the external code creating the device cannot know what device is supported.

@bchalios bchalios force-pushed the cloudkernels:gicv2 branch 4 times, most recently from cb90776 to 5a78a6c Oct 31, 2019
Copy link
Contributor

serban300 left a comment

In general it looks good to me. I just have a couple of nits left.

let vgic_fd = vm.create_device(&mut gic_device).map_err(Error::CreateGIC);

vgic_fd
Comment on lines 64 to 66

This comment has been minimized.

Copy link
@serban300

serban300 Nov 1, 2019

Contributor

nit: here you can just do vm.create_device(&mut gic_device).map_err(Error::CreateGIC) . There's no need for the ssignment.

}
fn device_fd(&self) -> &DeviceFd {
Comment on lines 57 to 59

This comment has been minimized.

Copy link
@serban300

serban300 Nov 1, 2019

Contributor

nit: I think it would be nice to have a new line between the end of a definition and the beginning of the next one. Please fix this in all the structs.

This comment has been minimized.

Copy link
@bchalios

bchalios Nov 1, 2019

Author Contributor

I completely agree :) I just saw that at other places, for example vmm/src/device_manager/mmio.rs that it is not used an empty line.

@serban300 serban300 self-requested a review Nov 1, 2019
@bchalios bchalios force-pushed the cloudkernels:gicv2 branch 2 times, most recently from d52195a to c341967 Nov 1, 2019
@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Nov 1, 2019

Ok, done with the last comments, as well

Copy link
Contributor

serban300 left a comment

Everything looks ok. It's as good as approved on my side, but please sign your commits using git commit --ammend -s. Also I think it would make sense to reduce the number of commits. You can fixup everything in one commit.

@serban300 serban300 self-requested a review Nov 1, 2019
@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Nov 1, 2019

Everything looks ok. It's as good as approved on my side, but please sign your commits using git commit --ammend -s. Also I think it would make sense to reduce the number of commits. You can fixup everything in one commit.

Hmmm, I thought the commits were signed already. GitHub marks them as verified. Am I missing something?

@bchalios bchalios force-pushed the cloudkernels:gicv2 branch from c341967 to 378a6df Nov 1, 2019
@serban300

This comment has been minimized.

Copy link
Contributor

serban300 commented Nov 1, 2019

Everything looks ok. It's as good as approved on my side, but please sign your commits using git commit --ammend -s. Also I think it would make sense to reduce the number of commits. You can fixup everything in one commit.

Hmmm, I thought the commits were signed already. GitHub marks them as verified. Am I missing something?

I am not sure why github marks them as verified, maybe that's something else. But there should be a sign-off message at the end of each commit. Something like:

Signed-off-by: Serban Iorga <seriorga@amazon.com>

and git commit --ammend -s will add it automatically.

@bchalios

This comment has been minimized.

Copy link
Contributor Author

bchalios commented Nov 1, 2019

Everything looks ok. It's as good as approved on my side, but please sign your commits using git commit --ammend -s. Also I think it would make sense to reduce the number of commits. You can fixup everything in one commit.

Hmmm, I thought the commits were signed already. GitHub marks them as verified. Am I missing something?

I am not sure why github marks them as verified, maybe that's something else. But there should be a sign-off message at the end of each commit. Something like:

Signed-off-by: Serban Iorga <seriorga@amazon.com>

and git commit --ammend -s will add it automatically.

Ok, I 'm generally using git commit -S, which actually signs the commit with GPG but doesn't add the "Signed-off-by" bit in the commit message.

We refactor the GIC-related code, to define a GICDevice Trait and then
create a GICv3 object that implements this Trait. This way, it is more
straightforward to introduce an implementations for different GIC
versions.

Moreover, we implement the Trait for GICv2 interrupt controllers.

At the moment, the creation of the GICDevice tries to create a version 3
device. If that fails, we fallback to GICv2.

Signed-off-by: Babis Chalios <babis.chalios@gmail.com>
@bchalios bchalios force-pushed the cloudkernels:gicv2 branch from 378a6df to 4b488a6 Nov 1, 2019
@acatangiu acatangiu merged commit 763a80c into firecracker-microvm:master Nov 1, 2019
2 checks passed
2 checks passed
buildkite/firecracker-ci-aarch64/pr Build #1181 passed (17 minutes, 17 seconds)
Details
default All checks have passed. Check the results in the target URL.
Details
@luxas

This comment has been minimized.

Copy link

luxas commented Nov 4, 2019

Thanks @bchalios @serban300 @acatangiu 🎉!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.