Skip to content

Conversation

@jiangliu
Copy link
Member

Add two enum to manage device resources.

The high level flow of resource management among the VMM, the device manager, and the device is as below:

  • the VMM creates a new device object.
  • the VMM asks the new device object for its resource requirements.
  • the VMM allocates resources for the device object according to resource requirements.
  • the VMM passes the allocated resources to the device object.
  • the VMM registers the new device onto corresponding device managers according the allocated resources.

@jiangliu
Copy link
Member Author

This is related to #8

@sameo sameo requested a review from a team September 11, 2019 09:05
@ghost ghost requested review from andreeaflorescu and sameo and removed request for a team September 11, 2019 09:05
/// KVM memslot index.
KvmMemSlot(u32),
}

Copy link

Choose a reason for hiding this comment

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

To summarize the discussion that took place in PR #8:

There is a proposal for using the same enum for both request and allocated resources. The enum would look like:

/// Resources flags
pub const MMIO_32BIT: u32 = 1;
pub const MMIO_64BIT: u32 = 2;

pub enum MsiIrqType {
    /// PCI MSI IRQ numbers.
    PciMsi,
    /// PCI MSIx IRQ numbers.
    PciMsix,
    /// Generic MSI IRQ numbers.
    GenericMsi,
}

pub enum Resource {
    PioAddress {
        base: Option<u16>,
        size: u16,
        align: u16,
    }

    MmioAddress {
        base: Option<u64>,
        flags: u64,
        size: u64,
        align: u64,
    }

    LegacyIrq {
        base: Option<u32>,
    },

    MsiIrq {
        msi_type: MsiIrqType,
        base: u32,
        size: u32,
    },

    MacAddresss(String),

    KvmMemSlot {
        base: Option<u32>,
        size: u32,
    },
}    

This would allow us to express the same set of requirements as the current proposal (fixed address range, fixed legacy irq, 32-bit address range (fixed or not)), and at the same time use the same definition for getting the requested resources from a Device and passing the reserved resources back to it.
I think it would make the API simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using Option to encode fixed resource request is a good idea, but using it to encode allocated resource may seem inconvenient to the resource consumer. We have such experience when implementing tempdir in vmm-sys-util crate.

Copy link

Choose a reason for hiding this comment

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

So would something like that:

/// Resources flags
pub const MMIO_32BIT: u32 = 1;
pub const MMIO_64BIT: u32 = 2;

/// Type of Message Singaled Interrupt
#[derive(Copy, Clone, PartialEq)]
pub enum MsiIrqType {
    /// PCI MSI IRQ numbers.
    PciMsi,
    /// PCI MSIx IRQ numbers.
    PciMsix,
    /// Generic MSI IRQ numbers.
    GenericMsi,
}

/// A Resource can be used by a device for requesting a specific resource,
/// and by the VMM to pass reserved resources back to a device.
pub enum Resource {
    /// Port IO (PIO) address range.
    PioRange {
        /// In order to request a PIO range that starts at a specific address,
        /// a device must set `base` to such address. Otherwise it should be set
        /// to u16::MAX.
        base: u16,
        /// Size for the allocated address range.
        size: u16,
        /// Alignment for the allocated range base address.
        align: u16,
    }

    /// Memory Mapped IO (MMIO) address range.
    MmioRange {
        /// In order to request an MMIO range that starts at a specific address,
        /// a device must set `base` to such address. Otherwise it should be set
        /// to u64::MAX.
        base: u64,
        /// flags is used by devices to express additional MMIO range resource
        /// requirements, like e.g. the fact that a range must live in the
        /// 32-bit address space.
        flags: u64,
        /// Size for the allocated address range.
        size: u64,
        /// Alignment for the allocated range base address.
        align: u64,
    }

    /// Legacy IRQ number
    LegacyIrq {
        /// In order to request for a specific legacy IRQ number, a device must
        /// set `base` to such IRQ number. Otherwise it should be set to
        /// u32::MAX.
        base: u32,
    },

    /// Message Signaled Interrupt
    MsiIrq {
        /// MSI IRQ type
        msi_type: MsiIrqType,
        /// base must be set to u32::MAX for a device resource request.
        base: u32,
        /// Number of IRQs in the MSI range.
        size: u32,
    },

    /// Network Interface Card MAC address.
    MacAddresss(String),

    KvmMemSlotRange {
        /// In order to request a KVM slot range that starts from a specific
        /// slot index, a device must set `index` to such slot index. Otherwise
        /// it should be set to u32::MAX.
        index: u32,
        /// Size for the allocated slot range.
        size: u32,
    },
}    

look better to you?

Copy link

Choose a reason for hiding this comment

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

@jiangliu Any feedback on the counter proposal?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use the same underline data structure to represent requests and assigned resources. On the other handle, it will be much more convenient to provide two sets of methods to create resource request objects and assigned resource objects, such as:
Resource::new_mmio() for assigned mmio resource and resource::new_mmio_request() for mmio request.

Copy link

@liujing2 liujing2 Oct 30, 2019

Choose a reason for hiding this comment

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

@jiangliu Assuming we have a Resource trait that devices would implement:

pub Trait DeviceResources: Send {
     fn get_resources(&self) -> Vec<Resource>;
     fn set_resources(&mut self, Vec<Resource>);
}

having one single set of API for allocate and create Resources would mean both the device and the allocator could use the same set of APIs. Which sounds simpler to me, but I may be missing something?

@sameo @jiangliu I think It's good to have such trait than implementing all request resources in VMM. According to the PR it seems we need to change the return type a little bit as follows. Do I misunderstand anything?

pub Trait DeviceResources: Send {
     // Device object returns the ResourceConstraint to VMM
     fn get_resources_requirement(&self) -> Vec<ResourceConstraint>;
     // Device object sets back the assigned Resource
     fn set_assigned_resources(&mut self, Vec<Resource>);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

According to my experience, set_assigned_resources() seems good at design but may not be the best way for implementation.
For example, the assigned resources are not available when creating the device, and will be assigned later. So the device driver must use field "Option" to store assigned resources. And you know, Option<> is not very friend.

We have tried the way to "set_assigned_resources()" but give up.

Choose a reason for hiding this comment

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

For example, the assigned resources are not available when creating the device, and will be assigned later. So the device driver must use field "Option" to store assigned resources.

Do you mean that when we create a device object, there is no resources available? How about setting to a MAX value as a hint? In this way, it is used to construct a right Device::ResourceConstraint

Copy link

Choose a reason for hiding this comment

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

For example, the assigned resources are not available when creating the device, and will be assigned later. So the device driver must use field "Option" to store assigned resources. And you know, Option<> is not very friend.

I agree with what you describe here, and it's better to let the device implementation assume that resources are created and available when we create it. But I still believe that there should be a way to asynchronously assign resources to a device, for device reprogramming for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

The PCI bar reprogram is not covered yet. We may solve it later when the vm-allocator becomes more solid.

@bonzini
Copy link
Member

bonzini commented Oct 23, 2019

How is a MAC address comparable to an I/O address?

@jiangliu
Copy link
Member Author

How is a MAC address comparable to an I/O address?

The idea is to use enum Resource to encapsulate all resources needed by device backend drivers. For some virtio-net devices, they must use assigned MAC addresses, so we abstract NIC MAC as an device resource type.

Introduce data structures to manage device resources, so we could
decouple resource allocator from resource consumers.

Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>
Add ResourceConstraint enum to describe devices's resource allocation
constraints, so we could build better flow among the VMM, the device
manager and the device object.

Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>
Signed-off-by: 守情 <liheng.xlh@alibaba-inc.com>
Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
@jiangliu
Copy link
Member Author

There are three components related to VMM resource management.

  1. The consumer side is vm-device/{Resource, ResourceConstraint}.
  2. The provider side is vm-allocator, an interval tree based allocator has been posted at Provide a specialized interval tree for resource management vm-allocator#3
  3. The resource manager to glue the provider and consumer. It manages all resources of the VMM. This part may be VMM specific and we haven't discussed about the part yet.

@jiangliu jiangliu force-pushed the resources branch 3 times, most recently from 1f908f4 to 9ef166e Compare October 29, 2019 03:41
@jiangliu jiangliu requested review from bonzini, chao-p and sameo October 29, 2019 03:45
@sameo
Copy link

sameo commented Oct 29, 2019

@jiangliu I sent a tiny PR against your resource branch for fixing a few comments and typos. Once the PR is merged, it'll LGTM.

sameo
sameo previously approved these changes Oct 29, 2019
@sameo
Copy link

sameo commented Nov 1, 2019

@chao-p Could you please have a look?

Copy link
Member

@chao-p chao-p left a comment

Choose a reason for hiding this comment

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

Since you changed 'resource requirement' to 'resource constraint', which sounds a good naming to me, but would you update your commit message as well? Besides that, overall looks good and ready to be merged.

@jiangliu jiangliu force-pushed the resources branch 2 times, most recently from 3ac25a3 to 04fa78a Compare November 6, 2019 16:33
@jiangliu jiangliu requested review from chao-p, liujing2 and sameo November 6, 2019 16:54
@jiangliu jiangliu merged commit c2cafa6 into rust-vmm:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants