islet-project / islet

An on-device confidential computing platform
Apache License 2.0
91 stars 16 forks source link

Integrate realm and rd #312

Closed zpzigi754 closed 4 months ago

zpzigi754 commented 4 months ago

This PR contains the refactoring commits related to the earlier discussion (#230). Realm and Rd (Realm Descriptor) have almost the same purpose. It aims to keep only Rd which is defined by spec.

Summary of changes

-pub struct Realm {
-   id: usize,
-   pub vmid: u16,
-   pub state: State,
-   pub vcpus: Vec<Arc<Mutex<VCPU>>>,
-   pub page_table: Arc<Mutex<Box<dyn IPATranslation>>>,
-   pub measurements: [Measurement; MEASUREMENTS_SLOT_NR],
-}

pub struct Rd {
-   realm_id: usize,
+   vmid: u16,
    ...
+   s2_table: Arc<Mutex<Box<dyn IPATranslation>>>,
    hash_algo: u8,
+   pub measurements: [Measurement; MEASUREMENTS_SLOT_NR],
+   pub vcpus: Vec<Arc<Mutex<VCPU>>>,
}

Notes

Changes in the caller side

[before]
   let (s2tte, last_level) = get_realm(realm_id)
          .ok_or(Error::RmiErrorOthers(NotExistRealm))?
          .lock()
          .page_table
          .lock()
          .ipa_to_pte(GuestPhysAddr::from(ipa), level)
          .ok_or(error_code)?;

[after]
   let (s2tte, last_level) = rd
          .s2_table()
          .lock()
          .ipa_to_pte(GuestPhysAddr::from(ipa), level)
          .ok_or(error_code)?;
[before]
   let measurements = crate::realm::registry::get_realm(realmid)
        .ok_or(Error::RmiErrorOthers(NotExistRealm))?
        .lock()
        .measurements;

[after]
   let measurements = rd.measurements;

A caution

jinbpark commented 4 months ago

This refactoring is what I have been wanting to see! Thanks for bringing this up :)

zpzigi754 commented 4 months ago

Do you think it is better to leave the definition of realm, Rd, inside rmi? I think it is more appropriate to place it inside realm.

Good question! I also think that most of the data structures including Rd and Rec are better to be placed outside the current rmi module.

zpzigi754 commented 4 months ago

Do you have plan to integrate vcpu and rec too?

Partly yes, but not in the near time. I've refactored Realm in order to help verify Realm-related RMIs. So, the integration of vcpu and rec would be required before verifying Rec-related RMIs. The refactoring task itself could be done by another.