From b6ef09ecc582f6069f9186fcafaf6280d8770b8e Mon Sep 17 00:00:00 2001 From: Tinghao Zhang Date: Wed, 15 Jun 2022 22:24:46 +0800 Subject: [PATCH] devices: pci: don't reserve bridge window for hot added bridges We do not need to reserve bridge window for hot added bridges because guest will do that for us. This chunk of bridge window is also reserved by pcie root port, so no extra MMIO allocation is needed. So we config zeros sized window here for hot added bridges. BUG=b:199986018 TEST=./tools/presubmit Change-Id: I7e64b7596e1b5c060cf628a488e2399ae9c257fc Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3709794 Reviewed-by: Daniel Verkamp Commit-Queue: Daniel Verkamp Tested-by: kokoro --- devices/src/pci/pcie/pci_bridge.rs | 144 ++++++++++++++++------------ devices/src/pci/pcie/pcie_device.rs | 5 +- devices/src/pci/pcie/pcie_rp.rs | 5 +- devices/src/pci/pcie/pcie_switch.rs | 43 ++++++++- 4 files changed, 127 insertions(+), 70 deletions(-) diff --git a/devices/src/pci/pcie/pci_bridge.rs b/devices/src/pci/pcie/pci_bridge.rs index 33908eeb0a..9930ecbaae 100644 --- a/devices/src/pci/pcie/pci_bridge.rs +++ b/devices/src/pci/pcie/pci_bridge.rs @@ -333,17 +333,22 @@ impl PciDevice for PciBridge { let mut window_size: u64 = 0; let mut pref_window_base: u64 = u64::MAX; let mut pref_window_size: u64 = 0; + let hotplug_implemented = self.device.lock().hotplug_implemented(); + let hotplugged = self.device.lock().hotplugged(); - if self.device.lock().hotplug_implemented() { - // Bridge for children hotplug, get desired bridge window size and reserve - // it for guest OS use + if hotplug_implemented || hotplugged { + // If bridge is for children hotplug, get desired bridge window size and reserve + // it for guest OS use. + // If bridge is hotplugged into the system, get the desired bridge window size + // from host. let (win_size, pref_win_size) = self.device.lock().get_bridge_window_size(); window_size = win_size; pref_window_size = pref_win_size; } else { + // Bridge has children connected, get bridge window size from children let mut window_end: u64 = 0; let mut pref_window_end: u64 = 0; - // Bridge has children connected, get bridge window size from children + for &BarRange { addr, size, @@ -366,59 +371,21 @@ impl PciDevice for PciBridge { } } - if window_size == 0 { - // Allocate at least 2MB bridge winodw - window_size = BR_MEM_MINIMUM; - } - // if window_base isn't set, allocate a new one - if window_base == u64::MAX { - // align window_size to 1MB - if window_size & (BR_WINDOW_ALIGNMENT - 1) != 0 { - window_size = (window_size + BR_WINDOW_ALIGNMENT - 1) & BR_WINDOW_MASK; + if !hotplugged { + // Only static bridge needs to locate their window's position. Hotplugged bridge's + // window will be handled by guest kernel. + if window_size == 0 { + // Allocate at least 2MB bridge winodw + window_size = BR_MEM_MINIMUM; } - match resources.mmio_allocator(MmioType::Low).allocate_with_align( - window_size, - Alloc::PciBridgeWindow { - bus: address.bus, - dev: address.dev, - func: address.func, - }, - "pci_bridge_window".to_string(), - BR_WINDOW_ALIGNMENT, - ) { - Ok(addr) => window_base = addr, - Err(e) => warn!( - "{} failed to allocate bridge window: {}", - self.debug_label(), - e - ), - } - } else { - // align window_base to 1MB - if window_base & (BR_WINDOW_ALIGNMENT - 1) != 0 { - window_size += window_base - (window_base & BR_WINDOW_MASK); + // if window_base isn't set, allocate a new one + if window_base == u64::MAX { // align window_size to 1MB if window_size & (BR_WINDOW_ALIGNMENT - 1) != 0 { window_size = (window_size + BR_WINDOW_ALIGNMENT - 1) & BR_WINDOW_MASK; } - window_base &= BR_WINDOW_MASK; - } - } - - if pref_window_size == 0 { - // Allocate at least 2MB prefetch bridge window - pref_window_size = BR_MEM_MINIMUM; - } - // if pref_window_base isn't set, allocate a new one - if pref_window_base == u64::MAX { - // align pref_window_size to 1MB - if pref_window_size & (BR_WINDOW_ALIGNMENT - 1) != 0 { - pref_window_size = (pref_window_size + BR_WINDOW_ALIGNMENT - 1) & BR_WINDOW_MASK; - } - match resources - .mmio_allocator(MmioType::High) - .allocate_with_align( - pref_window_size, + match resources.mmio_allocator(MmioType::Low).allocate_with_align( + window_size, Alloc::PciBridgeWindow { bus: address.bus, dev: address.dev, @@ -427,23 +394,74 @@ impl PciDevice for PciBridge { "pci_bridge_window".to_string(), BR_WINDOW_ALIGNMENT, ) { - Ok(addr) => pref_window_base = addr, - Err(e) => warn!( - "{} failed to allocate bridge window: {}", - self.debug_label(), - e - ), + Ok(addr) => window_base = addr, + Err(e) => warn!( + "{} failed to allocate bridge window: {}", + self.debug_label(), + e + ), + } + } else { + // align window_base to 1MB + if window_base & (BR_WINDOW_ALIGNMENT - 1) != 0 { + window_size += window_base - (window_base & BR_WINDOW_MASK); + // align window_size to 1MB + if window_size & (BR_WINDOW_ALIGNMENT - 1) != 0 { + window_size = (window_size + BR_WINDOW_ALIGNMENT - 1) & BR_WINDOW_MASK; + } + window_base &= BR_WINDOW_MASK; + } } - } else { - // align pref_window_base to 1MB - if pref_window_base & (BR_WINDOW_ALIGNMENT - 1) != 0 { - pref_window_size += pref_window_base - (pref_window_base & BR_WINDOW_MASK); + + if pref_window_size == 0 { + // Allocate at least 2MB prefetch bridge window + pref_window_size = BR_MEM_MINIMUM; + } + // if pref_window_base isn't set, allocate a new one + if pref_window_base == u64::MAX { // align pref_window_size to 1MB if pref_window_size & (BR_WINDOW_ALIGNMENT - 1) != 0 { pref_window_size = (pref_window_size + BR_WINDOW_ALIGNMENT - 1) & BR_WINDOW_MASK; } - pref_window_base &= BR_WINDOW_MASK; + match resources + .mmio_allocator(MmioType::High) + .allocate_with_align( + pref_window_size, + Alloc::PciBridgeWindow { + bus: address.bus, + dev: address.dev, + func: address.func, + }, + "pci_bridge_window".to_string(), + BR_WINDOW_ALIGNMENT, + ) { + Ok(addr) => window_base = addr, + Err(e) => warn!( + "{} failed to allocate bridge window: {}", + self.debug_label(), + e + ), + } + } else { + // align pref_window_base to 1MB + if pref_window_base & (BR_WINDOW_ALIGNMENT - 1) != 0 { + pref_window_size += pref_window_base - (pref_window_base & BR_WINDOW_MASK); + // align pref_window_size to 1MB + if pref_window_size & (BR_WINDOW_ALIGNMENT - 1) != 0 { + pref_window_size = + (pref_window_size + BR_WINDOW_ALIGNMENT - 1) & BR_WINDOW_MASK; + } + pref_window_base &= BR_WINDOW_MASK; + } + } + } else { + // 0 is Ok here because guest will relocate the bridge window + if window_size > 0 { + window_base = 0; + } + if pref_window_size > 0 { + pref_window_base = 0; } } diff --git a/devices/src/pci/pcie/pcie_device.rs b/devices/src/pci/pcie/pcie_device.rs index 1939e44439..c5d493f465 100644 --- a/devices/src/pci/pcie/pcie_device.rs +++ b/devices/src/pci/pcie/pcie_device.rs @@ -20,7 +20,7 @@ pub trait PcieDevice: Send { ) -> std::result::Result; fn read_config(&self, reg_idx: usize, data: &mut u32); fn write_config(&mut self, reg_idx: usize, offset: u64, data: &[u8]); - fn clone_interrupt(&mut self, msix_config: Arc>); + fn clone_interrupt(&mut self, msi_config: Arc>); fn get_caps(&self) -> Vec>; fn set_capability_reg_idx(&mut self, id: PciCapabilityID, reg_idx: usize); fn get_bus_range(&self) -> Option { @@ -33,6 +33,9 @@ pub trait PcieDevice: Send { /// Return false, the children pci devices should be connected statically fn hotplug_implemented(&self) -> bool; + /// This function returns true if this pcie device is hotplugged into the system + fn hotplugged(&self) -> bool; + /// Get bridge window size to cover children's mmio size /// (u64, u64) -> (non_prefetchable window size, prefetchable_window_size) fn get_bridge_window_size(&self) -> (u64, u64); diff --git a/devices/src/pci/pcie/pcie_rp.rs b/devices/src/pci/pcie/pcie_rp.rs index 697e4021de..0f328afcfd 100644 --- a/devices/src/pci/pcie/pcie_rp.rs +++ b/devices/src/pci/pcie/pcie_rp.rs @@ -1,7 +1,6 @@ // Copyright 2021 The Chromium OS Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. - use std::collections::BTreeMap; use std::sync::Arc; use sync::Mutex; @@ -115,6 +114,10 @@ impl PcieDevice for PcieRootPort { self.pcie_port.hotplug_implemented() } + fn hotplugged(&self) -> bool { + false + } + fn get_bridge_window_size(&self) -> (u64, u64) { self.pcie_port.get_bridge_window_size() } diff --git a/devices/src/pci/pcie/pcie_switch.rs b/devices/src/pci/pcie/pcie_switch.rs index 22e775954e..6606ce3ecc 100644 --- a/devices/src/pci/pcie/pcie_switch.rs +++ b/devices/src/pci/pcie/pcie_switch.rs @@ -2,8 +2,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +use std::sync::Arc; +use sync::Mutex; + use crate::pci::pci_configuration::PciCapabilityID; -use crate::pci::{PciAddress, PciCapability, PciDeviceError}; +use crate::pci::{MsiConfig, PciAddress, PciCapability, PciDeviceError}; use crate::pci::pcie::pci_bridge::PciBridgeBusRange; use crate::pci::pcie::pcie_device::{PciPmcCap, PcieCap, PcieDevice}; @@ -17,11 +20,12 @@ const PCIE_DP_DID: u16 = 0x3510; pub struct PcieUpstreamPort { pcie_port: PciePort, + hotplugged: bool, } impl PcieUpstreamPort { /// Constructs a new PCIE upstream port - pub fn new(primary_bus_num: u8, secondary_bus_num: u8) -> Self { + pub fn new(primary_bus_num: u8, secondary_bus_num: u8, hotplugged: bool) -> Self { PcieUpstreamPort { pcie_port: PciePort::new( PCIE_UP_DID, @@ -30,12 +34,14 @@ impl PcieUpstreamPort { secondary_bus_num, false, ), + hotplugged, } } - pub fn new_from_host(pcie_host: PcieHostPort) -> Self { + pub fn new_from_host(pcie_host: PcieHostPort, hotplugged: bool) -> Self { PcieUpstreamPort { pcie_port: PciePort::new_from_host(pcie_host, false), + hotplugged, } } } @@ -56,6 +62,10 @@ impl PcieDevice for PcieUpstreamPort { self.pcie_port.allocate_address(resources) } + fn clone_interrupt(&mut self, msi_config: Arc>) { + self.pcie_port.clone_interrupt(msi_config); + } + fn read_config(&self, reg_idx: usize, data: &mut u32) { self.pcie_port.read_config(reg_idx, data); } @@ -79,10 +89,18 @@ impl PcieDevice for PcieUpstreamPort { self.pcie_port.get_bus_range() } + fn get_removed_devices(&self) -> Vec { + Vec::new() + } + fn hotplug_implemented(&self) -> bool { false } + fn hotplugged(&self) -> bool { + self.hotplugged + } + fn get_bridge_window_size(&self) -> (u64, u64) { self.pcie_port.get_bridge_window_size() } @@ -90,11 +108,12 @@ impl PcieDevice for PcieUpstreamPort { pub struct PcieDownstreamPort { pcie_port: PciePort, + hotplugged: bool, } impl PcieDownstreamPort { /// Constructs a new PCIE downstream port - pub fn new(primary_bus_num: u8, secondary_bus_num: u8) -> Self { + pub fn new(primary_bus_num: u8, secondary_bus_num: u8, hotplugged: bool) -> Self { PcieDownstreamPort { pcie_port: PciePort::new( PCIE_DP_DID, @@ -103,12 +122,14 @@ impl PcieDownstreamPort { secondary_bus_num, false, ), + hotplugged, } } - pub fn new_from_host(pcie_host: PcieHostPort) -> Self { + pub fn new_from_host(pcie_host: PcieHostPort, hotplugged: bool) -> Self { PcieDownstreamPort { pcie_port: PciePort::new_from_host(pcie_host, false), + hotplugged, } } } @@ -129,6 +150,10 @@ impl PcieDevice for PcieDownstreamPort { self.pcie_port.allocate_address(resources) } + fn clone_interrupt(&mut self, msi_config: Arc>) { + self.pcie_port.clone_interrupt(msi_config); + } + fn read_config(&self, reg_idx: usize, data: &mut u32) { self.pcie_port.read_config(reg_idx, data); } @@ -152,10 +177,18 @@ impl PcieDevice for PcieDownstreamPort { self.pcie_port.get_bus_range() } + fn get_removed_devices(&self) -> Vec { + Vec::new() + } + fn hotplug_implemented(&self) -> bool { false } + fn hotplugged(&self) -> bool { + self.hotplugged + } + fn get_bridge_window_size(&self) -> (u64, u64) { self.pcie_port.get_bridge_window_size() }