x86_64: move --pcie-ecam into --pci

A breaking change, but we don't think anyone is actively using
--pcie-ecam. Aligns with the new --pci argument for configure the PCI
CAM region and allows us to delete the custom argument parser.

Change-Id: Ia04dd99c1470b67d076c0f52f0c0c3eab88d5a0d
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/6042655
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Commit-Queue: Frederick Mayle <fmayle@google.com>

arch/src/lib.rs

@@ -68,8 +68,6 @@ use jail::FakeMinijailStub as Minijail;
 #[cfg(any(target_os = "android", target_os = "linux"))]
 use minijail::Minijail;
 use remain::sorted;
-#[cfg(target_arch = "x86_64")]
-use resources::AddressRange;
 use resources::SystemAllocator;
 use resources::SystemAllocatorConfig;
 use serde::de::Visitor;
@@ -356,6 +354,9 @@ pub struct PciConfig {
     /// region for PCI Configuration Access Mechanism
     #[cfg(any(target_arch = "arm", target_arch = "aarch64"))]
     pub cam: Option<MemoryRegionConfig>,
+    /// region for PCIe Enhanced Configuration Access Mechanism
+    #[cfg(target_arch = "x86_64")]
+    pub ecam: Option<MemoryRegionConfig>,
     /// region for non-prefetchable PCI device memory below 4G
     pub mem: Option<MemoryRegionConfig>,
 }
@@ -401,8 +402,6 @@ pub struct VmComponents {
     ))]
     pub normalized_cpu_capacities: BTreeMap<usize, u32>,
     pub pci_config: PciConfig,
-    #[cfg(target_arch = "x86_64")]
-    pub pcie_ecam: Option<AddressRange>,
     pub pflash_block_size: u32,
     pub pflash_image: Option<File>,
     pub pstore: Option<Pstore>,

src/crosvm/cmdline.rs

@@ -89,8 +89,6 @@ use crate::crosvm::config::parse_cpu_capacity;
 ))]
 use crate::crosvm::config::parse_cpu_frequencies;
 use crate::crosvm::config::parse_dynamic_power_coefficient;
-#[cfg(target_arch = "x86_64")]
-use crate::crosvm::config::parse_memory_region;
 use crate::crosvm::config::parse_mmio_address_range;
 use crate::crosvm::config::parse_pflash_parameters;
 use crate::crosvm::config::parse_serial_options;
@@ -1798,6 +1796,9 @@ pub struct RunCommand {
     ///
     /// Possible key values (aarch64 only):
     ///     cam=[start=INT,size=INT] - region for PCI Configuration Access Mechanism
+    ///
+    /// Possible key values (x86_64 only):
+    ///     ecam=[start=INT,size=INT] - region for PCIe Enhanced Configuration Access Mechanism
     pub pci: Option<PciConfig>,
 
     #[cfg(any(target_os = "android", target_os = "linux"))]
@@ -1814,17 +1815,6 @@ pub struct RunCommand {
     /// the pci mmio start address below 4G
     pub pci_start: Option<u64>,
 
-    #[cfg(target_arch = "x86_64")]
-    #[argh(
-        option,
-        arg_name = "mmio_base,mmio_length",
-        from_str_fn(parse_memory_region)
-    )]
-    #[serde(skip)] // TODO(b/255223604)
-    #[merge(strategy = overwrite_option)]
-    /// region for PCIe Enhanced Configuration Access Mechanism
-    pub pcie_ecam: Option<AddressRange>,
-
     #[argh(switch)]
     #[serde(skip)] // TODO(b/255223604)
     #[merge(strategy = overwrite_option)]
@@ -3621,7 +3611,6 @@ impl TryFrom<RunCommand> for super::config::Config {
             cfg.break_linux_pci_config_io = cmd.break_linux_pci_config_io.unwrap_or_default();
             cfg.enable_hwp = cmd.enable_hwp.unwrap_or_default();
             cfg.force_s2idle = cmd.s2idle.unwrap_or_default();
-            cfg.pcie_ecam = cmd.pcie_ecam;
             cfg.no_i8042 = cmd.no_i8042.unwrap_or_default();
             cfg.no_rtc = cmd.no_rtc.unwrap_or_default();
             cfg.smbios = cmd.smbios.unwrap_or_default();

src/crosvm/config.rs

@@ -83,14 +83,6 @@ cfg_if::cfg_if! {
     }
 }
 
-#[cfg(target_arch = "x86_64")]
-const ONE_MB: u64 = 1 << 20;
-#[cfg(target_arch = "x86_64")]
-const MB_ALIGNED: u64 = ONE_MB - 1;
-// the max bus number is 256 and each bus occupy 1MB, so the max pcie cfg mmio size = 256M
-#[cfg(target_arch = "x86_64")]
-const MAX_PCIE_ECAM_SIZE: u64 = ONE_MB * 256;
-
 // by default, if enabled, the balloon WS features will use 4 bins.
 #[cfg(feature = "balloon")]
 const VIRTIO_BALLOON_WS_DEFAULT_NUM_BINS: u8 = 4;
@@ -475,63 +467,6 @@ pub fn parse_serial_options(s: &str) -> Result<SerialParameters, String> {
     Ok(params)
 }
 
-#[cfg(target_arch = "x86_64")]
-pub fn parse_memory_region(value: &str) -> Result<AddressRange, String> {
-    let paras: Vec<&str> = value.split(',').collect();
-    if paras.len() != 2 {
-        return Err(invalid_value_err(
-            value,
-            "pcie-ecam must have exactly 2 parameters: ecam_base,ecam_size",
-        ));
-    }
-    let base = parse_hex_or_decimal(paras[0]).map_err(|_| {
-        invalid_value_err(
-            value,
-            "pcie-ecam, the first parameter base should be integer",
-        )
-    })?;
-    let mut len = parse_hex_or_decimal(paras[1]).map_err(|_| {
-        invalid_value_err(
-            value,
-            "pcie-ecam, the second parameter size should be integer",
-        )
-    })?;
-
-    if (base & MB_ALIGNED != 0) || (len & MB_ALIGNED != 0) {
-        return Err(invalid_value_err(
-            value,
-            "pcie-ecam, the base and len should be aligned to 1MB",
-        ));
-    }
-
-    if len > MAX_PCIE_ECAM_SIZE {
-        len = MAX_PCIE_ECAM_SIZE;
-    }
-
-    if base + len >= 0x1_0000_0000 {
-        return Err(invalid_value_err(
-            value,
-            "pcie-ecam, the end address couldn't beyond 4G",
-        ));
-    }
-
-    if base % len != 0 {
-        return Err(invalid_value_err(
-            value,
-            "pcie-ecam, base should be multiple of len",
-        ));
-    }
-
-    if let Some(range) = AddressRange::from_start_and_size(base, len) {
-        Ok(range)
-    } else {
-        Err(invalid_value_err(
-            value,
-            "pcie-ecam must be representable as AddressRange",
-        ))
-    }
-}
-
 pub fn parse_bus_id_addr(v: &str) -> Result<(u8, u8, u16, u16), String> {
     debug!("parse_bus_id_addr: {}", v);
     let mut ids = v.split(':');
@@ -842,8 +777,6 @@ pub struct Config {
     pub pci_config: PciConfig,
     #[cfg(feature = "pci-hotplug")]
     pub pci_hotplug_slots: Option<u8>,
-    #[cfg(target_arch = "x86_64")]
-    pub pcie_ecam: Option<AddressRange>,
     pub per_vm_core_scheduling: bool,
     pub pflash_parameters: Option<PflashParameters>,
     #[cfg(feature = "plugin")]
@@ -1079,8 +1012,6 @@ impl Default for Config {
             pci_config: Default::default(),
             #[cfg(feature = "pci-hotplug")]
             pci_hotplug_slots: None,
-            #[cfg(target_arch = "x86_64")]
-            pcie_ecam: None,
             per_vm_core_scheduling: false,
             pflash_parameters: None,
             #[cfg(feature = "plugin")]
@@ -2531,6 +2462,31 @@ mod tests {
         );
     }
 
+    #[cfg(target_arch = "x86_64")]
+    #[test]
+    fn parse_pci_ecam() {
+        assert_eq!(
+            config_from_args(&["--pci", "ecam=[start=0x123]", "/dev/null"]).pci_config,
+            PciConfig {
+                ecam: Some(arch::MemoryRegionConfig {
+                    start: 0x123,
+                    size: None,
+                }),
+                ..PciConfig::default()
+            }
+        );
+        assert_eq!(
+            config_from_args(&["--pci", "ecam=[start=0x123,size=0x456]", "/dev/null"]).pci_config,
+            PciConfig {
+                ecam: Some(arch::MemoryRegionConfig {
+                    start: 0x123,
+                    size: Some(0x456),
+                }),
+                ..PciConfig::default()
+            },
+        );
+    }
+
     #[test]
     fn parse_pci_mem() {
         assert_eq!(

src/crosvm/sys/linux.rs

@@ -1499,8 +1499,6 @@ fn setup_vm_components(cfg: &Config) -> Result<VmComponents> {
         force_s2idle: cfg.force_s2idle,
         pvm_fw: pvm_fw_image,
         pci_config: cfg.pci_config,
-        #[cfg(target_arch = "x86_64")]
-        pcie_ecam: cfg.pcie_ecam,
         dynamic_power_coefficient: cfg.dynamic_power_coefficient.clone(),
         boot_cpu: cfg.boot_cpu,
         #[cfg(any(target_arch = "arm", target_arch = "aarch64"))]

src/sys/windows.rs

@@ -2112,8 +2112,6 @@ fn setup_vm_components(cfg: &Config) -> Result<VmComponents> {
         pvm_fw: None,
         pci_config: cfg.pci_config,
         #[cfg(target_arch = "x86_64")]
-        pcie_ecam: cfg.pcie_ecam,
-        #[cfg(target_arch = "x86_64")]
         smbios: cfg.smbios.clone(),
         dynamic_power_coefficient: cfg.dynamic_power_coefficient.clone(),
         #[cfg(target_arch = "x86_64")]

x86_64/src/lib.rs

@@ -174,6 +174,8 @@ pub enum Error {
     CommandLineOverflow,
     #[error("failed to configure hotplugged pci device: {0}")]
     ConfigurePciDevice(arch::DeviceRegistrationError),
+    #[error("bad PCI ECAM configuration: {0}")]
+    ConfigurePciEcam(String),
     #[error("bad PCI mem configuration: {0}")]
     ConfigurePciMem(String),
     #[error("failed to configure segment registers: {0}")]
@@ -418,15 +420,35 @@ pub struct ArchMemoryLayout {
 
 pub fn create_arch_memory_layout(
     pci_config: &PciConfig,
-    pcie_ecam: Option<AddressRange>,
     has_protected_vm_firmware: bool,
 ) -> Result<ArchMemoryLayout> {
-    const DEFAULT_PCIE_CFG_MMIO: AddressRange = AddressRange {
-        start: DEFAULT_PCIE_CFG_MMIO_START,
-        end: DEFAULT_PCIE_CFG_MMIO_END,
+    // the max bus number is 256 and each bus occupy 1MB, so the max pcie cfg mmio size = 256M
+    const MAX_PCIE_ECAM_SIZE: u64 = 256 * MB;
+    let pcie_cfg_mmio = match pci_config.ecam {
+        Some(MemoryRegionConfig {
+            start,
+            size: Some(size),
+        }) => AddressRange::from_start_and_size(start, size.min(MAX_PCIE_ECAM_SIZE)).unwrap(),
+        Some(MemoryRegionConfig { start, size: None }) => {
+            AddressRange::from_start_and_end(start, DEFAULT_PCIE_CFG_MMIO_END)
+        }
+        None => {
+            AddressRange::from_start_and_end(DEFAULT_PCIE_CFG_MMIO_START, DEFAULT_PCIE_CFG_MMIO_END)
+        }
     };
-
-    let pcie_cfg_mmio = pcie_ecam.unwrap_or(DEFAULT_PCIE_CFG_MMIO);
+    if pcie_cfg_mmio.start % pcie_cfg_mmio.len().unwrap() != 0
+        || pcie_cfg_mmio.start % MB != 0
+        || pcie_cfg_mmio.len().unwrap() % MB != 0
+    {
+        return Err(Error::ConfigurePciEcam(
+            "base and len must be aligned to 1MB and base must be a multiple of len".to_string(),
+        ));
+    }
+    if pcie_cfg_mmio.end >= 0x1_0000_0000 {
+        return Err(Error::ConfigurePciEcam(
+            "end address can't go beyond 4G".to_string(),
+        ));
+    }
 
     let pci_mmio_before_32bit = match pci_config.mem {
         Some(MemoryRegionConfig {
@@ -761,7 +783,6 @@ impl arch::LinuxArch for X8664arch {
     ) -> std::result::Result<Self::ArchMemoryLayout, Self::Error> {
         create_arch_memory_layout(
             &components.pci_config,
-            components.pcie_ecam,
             components.hv_cfg.protection_type.runs_firmware(),
         )
     }
@@ -2371,13 +2392,16 @@ mod tests {
 
     fn setup() -> ArchMemoryLayout {
         let pci_config = PciConfig {
+            ecam: Some(MemoryRegionConfig {
+                start: 3 * GB,
+                size: Some(256 * MB),
+            }),
             mem: Some(MemoryRegionConfig {
                 start: 2 * GB,
                 size: None,
             }),
         };
-        let pcie_ecam = Some(AddressRange::from_start_and_size(3 * GB, 256 * MB).unwrap());
-        create_arch_memory_layout(&pci_config, pcie_ecam, false).unwrap()
+        create_arch_memory_layout(&pci_config, false).unwrap()
     }
 
     #[test]