On Wednesday 24 August 2022 09:25:15 Lorenzo Pieralisi wrote: > [...] > > > > In other words: what if PCI express capability is lower in the address > > > space (config) than the subsystem ID vendor capability ? > > > > Current code expects that if host controller driver sets both pcie_start > > and ssid_start, those values are correct, non-overlapping and can be > > handled correctly. > > > > And if offset to PCI express capability is lower than offset to SSID > > capability then there should not be any issue. First capability is > > correctly set into capabilities_pointer (via min function) and then > > pci_bridge_emul_conf_read() should handle it. > > I don't understand how the pointer to the SSID cap is set in this > specific case, I don't see any code doing that but that's most certainly > because I don't know in details the emul bridge internals. > > IIUC, in pci_bridge_emul_read_ssid(), we set the next cap pointer if > the PCI express capability is at an address higher (current case) than > the SSID capability otherwise we set it to 0 (end of the list). > > The other way around, I don't see the PCI express next cap pointer > being set anywhere (where, IIUC, it should be set to point to the > SSID cap) - I am not sure you are handling it. Ou, I see. This is really missing! It should be set when initializing pcie_conf.cap_id and pcie_conf.cap at this place: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/pci/pci-bridge-emul.c?h=v6.0-rc2#n379 I will fix it and send a new version of this patch. > That's the only question I have on this patch. > > Lorenzo > > > The whole my idea is to construct capabilities linked list structure > > correctly based on input requirements (e.g. fixed location of some > > capability, etc). > > > > > I am just trying to understand the patch, so forgive me if the question > > > is already addressed in the code. > > > > > > Thanks, > > > Lorenzo > > > > > > > + else > > > > + bridge->pcie_start = bridge->ssid_start + PCI_CAP_SSID_SIZEOF; > > > > + } > > > > + > > > > + bridge->conf.capabilities_pointer = min(bridge->ssid_start, bridge->pcie_start); > > > > > > > > if (bridge->conf.capabilities_pointer) > > > > bridge->conf.status |= cpu_to_le16(PCI_STATUS_CAP_LIST); > > > > @@ -459,15 +468,17 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where, > > > > read_op = bridge->ops->read_base; > > > > cfgspace = (__le32 *) &bridge->conf; > > > > behavior = bridge->pci_regs_behavior; > > > > - } else if (reg >= PCI_CAP_SSID_START && reg < PCI_CAP_SSID_END && bridge->subsystem_vendor_id) { > > > > + } else if (reg >= bridge->ssid_start && reg < bridge->ssid_start + PCI_CAP_SSID_SIZEOF && > > > > + bridge->subsystem_vendor_id) { > > > > /* Emulated PCI Bridge Subsystem Vendor ID capability */ > > > > - reg -= PCI_CAP_SSID_START; > > > > + reg -= bridge->ssid_start; > > > > read_op = pci_bridge_emul_read_ssid; > > > > cfgspace = NULL; > > > > behavior = NULL; > > > > - } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) { > > > > + } else if (reg >= bridge->pcie_start && reg < bridge->pcie_start + PCI_CAP_PCIE_SIZEOF && > > > > + bridge->has_pcie) { > > > > /* Our emulated PCIe capability */ > > > > - reg -= PCI_CAP_PCIE_START; > > > > + reg -= bridge->pcie_start; > > > > read_op = bridge->ops->read_pcie; > > > > cfgspace = (__le32 *) &bridge->pcie_conf; > > > > behavior = bridge->pcie_cap_regs_behavior; > > > > @@ -538,9 +549,10 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where, > > > > write_op = bridge->ops->write_base; > > > > cfgspace = (__le32 *) &bridge->conf; > > > > behavior = bridge->pci_regs_behavior; > > > > - } else if (reg >= PCI_CAP_PCIE_START && reg < PCI_CAP_PCIE_END && bridge->has_pcie) { > > > > + } else if (reg >= bridge->pcie_start && reg < bridge->pcie_start + PCI_CAP_PCIE_SIZEOF && > > > > + bridge->has_pcie) { > > > > /* Our emulated PCIe capability */ > > > > - reg -= PCI_CAP_PCIE_START; > > > > + reg -= bridge->pcie_start; > > > > write_op = bridge->ops->write_pcie; > > > > cfgspace = (__le32 *) &bridge->pcie_conf; > > > > behavior = bridge->pcie_cap_regs_behavior; > > > > diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h > > > > index 71392b67471d..2a0e59c7f0d9 100644 > > > > --- a/drivers/pci/pci-bridge-emul.h > > > > +++ b/drivers/pci/pci-bridge-emul.h > > > > @@ -131,6 +131,8 @@ struct pci_bridge_emul { > > > > struct pci_bridge_reg_behavior *pci_regs_behavior; > > > > struct pci_bridge_reg_behavior *pcie_cap_regs_behavior; > > > > void *data; > > > > + u8 pcie_start; > > > > + u8 ssid_start; > > > > bool has_pcie; > > > > u16 subsystem_vendor_id; > > > > u16 subsystem_id; > > > > -- > > > > 2.20.1 > > > >
