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.

Sign up for GitHub

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

Jump to bottom

dragonball: add pci root bus and root device #8564

Merged
merged 1 commit into from Dec 13, 2023
Merged

dragonball: add pci root bus and root device #8564

merged 1 commit into from Dec 13, 2023

Conversation

studychao
Copy link
Member

In order to follow up the PCI implementation in Dragonball, we need to add PCI root device and root bus support.

root device is a pseudo PCI root device to manage accessing to PCI configuration space.

root bus is mainly for emulating PCI root bridge and also create the PCI root bus with the given bus ID with the PCI root bridge.

fixes: #8563

Signed-off-by: Gerry Liu gerry@linux.alibaba.com
Signed-off-by: Zizheng Bian zizheng.bian@linux.alibaba.com
Signed-off-by: Shifang Feng fengshifang@linux.alibaba.com
Signed-off-by: Yang Su yang.su@linux.alibaba.com
Signed-off-by: Zha Bin zhabin@linux.alibaba.com
Signed-off-by: Xin Lin jingshan@linux.alibaba.com
Signed-off-by: Chao Wu chaowu@linux.alibaba.com

@katacontainersbot katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Dec 5, 2023
ZizhengBian
ZizhengBian reviewed Dec 6, 2023
View reviewed changes
src/dragonball/src/dbs_pci/src/device.rs
@@ -2,8 +2,6 @@
//
// SPDX-License-Identifier: Apache-2.0

use std::any::Any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's no compile warning check in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a bug in Dragonball CI and I have fixed it in #8599

src/dragonball/src/dbs_pci/src/lib.rs Outdated
@@ -30,22 +30,34 @@ mod bus;
mod configuration;
mod device;

pub use self::bus::PciBus;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we change the style like below？

mod xxx;
pub use xxx:yyy;

Copy link
Member Author

Choose a reason for hiding this comment

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

modified.

src/dragonball/src/dbs_pci/src/root_device.rs

use crate::fill_config_data;
use crate::{Error, PciBus, Result};
#[derive(PartialEq)]
Copy link
Contributor

Choose a reason for hiding this comment

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

add a blank line here

Copy link
Member Author

Choose a reason for hiding this comment

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

added

justxuewei
justxuewei reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Thanks @studychao! A few comments here.

src/dragonball/src/dbs_pci/src/root_device.rs Outdated
let state = self.state.read().unwrap();
let (b, d, f, o) = parse_mmio_address(offset);
if let Some(bus) = state.buses.get(&b) {
return bus.read_config(d, f, o | ((offset as u32) & 0x3), data);
Copy link
Member

Choose a reason for hiding this comment

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

Could you set a const for 0x3 to describe what it is? Or leave some comments here.

Copy link
Member Author

@studychao studychao Dec 11, 2023 •
edited

Choose a reason for hiding this comment

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

I found out that 0x3 is useless in this method because parse_mmio_address will use the lowest 12 bits, so I remove them from the code.

src/dragonball/src/dbs_pci/src/root_device.rs Outdated
let state = self.state.read().unwrap();
let (b, d, f, o) = parse_mmio_address(offset);
if let Some(bus) = state.buses.get(&b) {
return bus.write_config(d, f, o | (offset as u32 & 0x3), data);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/dragonball/src/dbs_pci/src/root_device.rs Outdated
let len = data.len();

// Only allow naturally aligned Dword, Word and Byte access.
if (len == 4 || len == 2 || len == 1) && offset as usize & (len - 1) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap this into an inline function? It is used three times.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, actually it is used four times lol

src/dragonball/src/dbs_pci/src/root_device.rs Outdated
let offset = offset.raw_value();
let len = data.len();

if (len == 4 || len == 2 || len == 1) && offset as usize & (len - 1) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/dragonball/src/dbs_pci/src/root_device.rs Outdated
let len = data.len();

// Only allow naturally aligned Dword, Word and Byte access.
if (len == 4 || len == 2 || len == 1) && offset as usize & (len - 1) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

src/dragonball/src/dbs_pci/src/root_device.rs Outdated
Comment on lines 286 to 301
let shift = (offset & 0x3) * 8;
if len == 4 {
state.io_addr = NativeEndian::read_u32(data);
} else if len == 2 {
state.io_addr &= !(0xffffu32 << shift);
state.io_addr |= u32::from(NativeEndian::read_u16(data)) << shift;
} else {
state.io_addr &= !(0xffu32 << shift);
Copy link
Member

Choose a reason for hiding this comment

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

There is a lot of hard code here.

Copy link
Member Author

Choose a reason for hiding this comment

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

comments all added.

src/dragonball/src/dbs_pci/src/root_device.rs
4..=7 => {
// Safe to unwrap because no legal to generate poisoned RwLock.
let state = self.state.read().unwrap();
if state.io_addr & 0x8000_0000 != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Hard code again.

Copy link
Member Author

Choose a reason for hiding this comment

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

comment added.

src/dragonball/src/dbs_pci/src/root_device.rs
bus_number,
device_number,
function_number,
register_number & !0x3u32,
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

document added.

src/dragonball/src/dbs_pci/src/root_device.rs Outdated
Comment on lines 360 to 383
(addr >> 20) & 0xff,
(addr >> 15) & 0x1f,
(addr >> 12) & 0x7,
addr & 0xfff,
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member Author

Choose a reason for hiding this comment

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

const added.

src/dragonball/src/dbs_pci/src/bus.rs Outdated
@@ -324,8 +322,8 @@ impl PartialEq for PciBus {

#[inline]
fn check_pci_cfg_valid(dev: u32, func: u32, offset: u32, data_len: usize) -> bool {
dev > 0x1f || func !=0 || offset >= 0x1000 || offset & (data_len - 1 ) as u32 & 0x3 != 0
}
dev > 0x1f || func != 0 || offset >= 0x1000 || offset & (data_len - 1) as u32 & 0x3 != 0
Copy link
Member

Choose a reason for hiding this comment

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

Could you leave some comments here?

liubogithub
liubogithub approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@liubogithub liubogithub left a comment

Choose a reason for hiding this comment

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

lgtm

@studychao
dragonball: add pci root bus and root device
b079e1a 
In order to follow up the PCI implementation in Dragonball, we need to
add PCI root device and root bus support.

root device is a pseudo PCI root device to manage accessing to PCI
configuration space.

root bus is mainly for emulating PCI root bridge and also create the PCI
root bus with the given bus ID with the PCI root bridge.

fixes: kata-containers#8563

Signed-off-by: Gerry Liu <gerry@linux.alibaba.com>
Signed-off-by: Zizheng Bian <zizheng.bian@linux.alibaba.com>
Signed-off-by: Shifang Feng <fengshifang@linux.alibaba.com>
Signed-off-by: Yang Su <yang.su@linux.alibaba.com>
Signed-off-by: Zha Bin <zhabin@linux.alibaba.com>
Signed-off-by: Xin Lin <jingshan@linux.alibaba.com>
Signed-off-by: Chao Wu <chaowu@linux.alibaba.com>
ZizhengBian
ZizhengBian approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@ZizhengBian ZizhengBian left a comment

Choose a reason for hiding this comment

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

LGTM

justxuewei
justxuewei approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

@studychao
Copy link
Member Author

/test

@studychao
Copy link
Member Author

/test-arm

@studychao studychao merged commit dfaf006 into kata-containers:main Dec 13, 2023
170 of 181 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liubogithub liubogithub liubogithub approved these changes

@quanweiZhou quanweiZhou quanweiZhou approved these changes

@justxuewei justxuewei justxuewei approved these changes

@ZizhengBian ZizhengBian ZizhengBian approved these changes

Assignees
No one assigned
Labels
ok-to-test runtime-rs size/huge Largest and most complex task (probably needs breaking into small pieces)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dragonball: pci root device and root bus support
6 participants
@studychao @liubogithub @quanweiZhou @justxuewei @ZizhengBian @katacontainersbot