Hi Andre, On 5/13/20 3:42 PM, Alexandru Elisei wrote: > Hi, > > On 5/13/20 9:17 AM, André Przywara wrote: >> On 12/05/2020 16:44, Alexandru Elisei wrote: >> >> Hi, >> >>> On 5/12/20 3:17 PM, André Przywara wrote: >>>> On 06/05/2020 14:51, Alexandru Elisei wrote: >>>> >>>> Hi, >>>> >>>>> On 4/6/20 3:06 PM, André Przywara wrote: >>>>> [..] >>>>>> Actually, looking closer: why do we need this in the first place? I >>>>>> removed this and struct pm_cap, and it still compiles. >>>>>> So can we lose those two structures at all? And move the discussion and >>>>>> implementation (for VirtIO 1.0?) to a later series? >>>>> I've answered both points in v2 of the series [1]. >>>>> >>>>> [1] https://www.spinics.net/lists/kvm/msg209601.html: >>>> From there: >>>>>> But more importantly: Do we actually need those definitions? We >>>>>> don't seem to use them, do we? >>>>>> And the u8 __pad[PCI_DEV_CFG_SIZE] below should provide the extended >>>>>> storage space a guest would expect? >>>>> Yes, we don't use them for the reasons I explained in the commit >>>>> message. I would rather keep them, because they are required by the >>>>> PCIE spec. >>>> I don't get the point of adding code / data structures that we don't >>>> need, especially if it has issues. I understand it's mandatory as per >>>> the spec, but just adding a struct here doesn't fix this or makes this >>>> better. >>> Sure, I can remove the unused structs, especially if they have issues. But I don't >>> see what issues they have, would you mind expanding on that? >> The best code is the one not written. ;-) > Truer words were never spoken. > > That settles it then, I'll remove the unused structs. Coming back to this. Without the PCIE capability, Linux will incorrectly set the size of the PCI configuration space for a device to the legacy PCI size (256 bytes instead of 4096). This is done in drivers/pci/probe.c::pci_setup_device, where dev->cfg_size = pci_cfg_size(dev). And pci_cfg_size() is: int pci_cfg_space_size(struct pci_dev *dev) { int pos; u32 status; u16 class; #ifdef CONFIG_PCI_IOV /* * Per the SR-IOV specification (rev 1.1, sec 3.5), VFs are required to * implement a PCIe capability and therefore must implement extended * config space. We can skip the NO_EXTCFG test below and the * reachability/aliasing test in pci_cfg_space_size_ext() by virtue of * the fact that the SR-IOV capability on the PF resides in extended * config space and must be accessible and non-aliased to have enabled * support for this VF. This is a micro performance optimization for * systems supporting many VFs. */ if (dev->is_virtfn) return PCI_CFG_SPACE_EXP_SIZE; #endif if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG) return PCI_CFG_SPACE_SIZE; class = dev->class >> 8; if (class == PCI_CLASS_BRIDGE_HOST) return pci_cfg_space_size_ext(dev); if (pci_is_pcie(dev)) return pci_cfg_space_size_ext(dev); pos = pci_find_capability(dev, PCI_CAP_ID_PCIX); if (!pos) return PCI_CFG_SPACE_SIZE; pci_read_config_dword(dev, pos + PCI_X_STATUS, &status); if (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)) return pci_cfg_space_size_ext(dev); return PCI_CFG_SPACE_SIZE; } And pci_is_pcie(dev) returns true if the device has a valid PCIE capability. Why do we care? The RTL8168 driver checks if the NIC is PCIE capable, and if not it prints the error message below and falls back to another (proprietary?) method of device configuration (in drivers/net/ethernet/realtek/r8169_main.c::rtl_csi_access_enable): [ 1.490530] r8169 0000:00:00.0 enp0s0: No native access to PCI extended config space, falling back to CSI [ 1.500201] r8169 0000:00:00.0 enp0s0: Link is Down I'll keep the PCIE cap and drop the power management cap. Thanks, Alex
