On Wed, 2022-01-26 at 14:23 +0100, Łukasz Gieryk wrote: > I'm sorry for the delayed response. We (I and the other Lukasz) somehow > had hoped that Knut, the original author of this patch, would have > responded.
Yes, sorry - this one flushed past me here for some reason, > > Let me address your questions, up to my best knowledge. > > > > -static pcibus_t pci_bar_address(PCIDevice *d, > > > - int reg, uint8_t type, pcibus_t size) > > > +static pcibus_t pci_config_get_bar_addr(PCIDevice *d, int reg, > > > + uint8_t type, pcibus_t size) > > > +{ > > > + pcibus_t new_addr; > > > + if (!pci_is_vf(d)) { > > > + int bar = pci_bar(d, reg); > > > + if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) { > > > + new_addr = pci_get_quad(d->config + bar); > > > + } else { > > > + new_addr = pci_get_long(d->config + bar); > > > + } > > > + } else { > > > + PCIDevice *pf = d->exp.sriov_vf.pf; > > > + uint16_t sriov_cap = pf->exp.sriov_cap; > > > + int bar = sriov_cap + PCI_SRIOV_BAR + reg * 4; > > > + uint16_t vf_offset = pci_get_word(pf->config + sriov_cap + > > > PCI_SRIOV_VF_OFFSET); > > > + uint16_t vf_stride = pci_get_word(pf->config + sriov_cap + > > > PCI_SRIOV_VF_STRIDE); > > > + uint32_t vf_num = (d->devfn - (pf->devfn + vf_offset)) / > > > vf_stride; > > > + > > > + if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) { > > > + new_addr = pci_get_quad(pf->config + bar); > > > + } else { > > > + new_addr = pci_get_long(pf->config + bar); > > > + } > > > + new_addr += vf_num * size; > > > + } > > > + if (reg != PCI_ROM_SLOT) { > > > + /* Preserve the rom enable bit */ > > > + new_addr &= ~(size - 1); > > > > This comment puzzles me. How does clearing low bits preserve > > any bits? Looks like this will clear low bits if any. > > > > I think the comment applies to (reg != PCI_ROM_SLOT), i.e., the bits are > cleared for BARs, but not for expansion ROM. I agree the placement of this > comment is slightly misleading. We will move it up and rephrase slightly. I agree - it's maybe better to just put the comment above the if(...) other than that I believe it is correct. Knut > > > > +pcibus_t pci_bar_address(PCIDevice *d, > > > + int reg, uint8_t type, pcibus_t size) > > > { > > > pcibus_t new_addr, last_addr; > > > - int bar = pci_bar(d, reg); > > > uint16_t cmd = pci_get_word(d->config + PCI_COMMAND); > > > Object *machine = qdev_get_machine(); > > > ObjectClass *oc = object_get_class(machine); > > > @@ -1309,7 +1363,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > > > if (!(cmd & PCI_COMMAND_IO)) { > > > return PCI_BAR_UNMAPPED; > > > } > > > - new_addr = pci_get_long(d->config + bar) & ~(size - 1); > > > + new_addr = pci_config_get_bar_addr(d, reg, type, size); > > > last_addr = new_addr + size - 1; > > > /* Check if 32 bit BAR wraps around explicitly. > > > * TODO: make priorities correct and remove this work around. > > > @@ -1324,11 +1378,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > > > if (!(cmd & PCI_COMMAND_MEMORY)) { > > > return PCI_BAR_UNMAPPED; > > > } > > > - if (type & PCI_BASE_ADDRESS_MEM_TYPE_64) { > > > - new_addr = pci_get_quad(d->config + bar); > > > - } else { > > > - new_addr = pci_get_long(d->config + bar); > > > - } > > > + new_addr = pci_config_get_bar_addr(d, reg, type, size); > > > /* the ROM slot has a specific enable bit */ > > > if (reg == PCI_ROM_SLOT && !(new_addr & PCI_ROM_ADDRESS_ENABLE)) { > > > > And in fact here we check the low bit and handle it specially. > > The code seems correct for me. The bit is preserved for ROM case. > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > index d7d73a31e4..182a225054 100644 > > > --- a/hw/pci/pcie.c > > > +++ b/hw/pci/pcie.c > > > @@ -446,6 +446,11 @@ void pcie_cap_slot_plug_cb(HotplugHandler > > > *hotplug_dev, > > > DeviceState *dev, > > > PCIDevice *pci_dev = PCI_DEVICE(dev); > > > uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP); > > > > > > + if(pci_is_vf(pci_dev)) { > > > + /* We don't want to change any state in hotplug_dev for SR/IOV > > > virtual > > > functions */ > > > + return; > > > + } > > > + > > > > Coding style violation here. And pls document the why not the what. > > E.g. IIRC the reason is that VFs don't have an express capability, > > right? > > I think the reason is that virtual functions don’t exist physically, so > they cannot be individually disconnected. Only PF should respond to > hotplug events, implicitly disconnecting (thus: destroying) all child > VFs. > > Anyway, we will update this comment to state *why* and add the missing > space. > > V4 with the mentioned changes will happen really soon. >